Provide ability to set annotations on worker nodes

[shyamradhakrishnan]

User Story

As an operator I would like to have the ability to set annotations on worker nodes created by CAPI. This is similar to the ability to set labels which is being currently worked on.

Detailed Description

In many cases, annotations are used to set metadata on nodes. For example in OCI, compartment id can be set on nodes which will help in discovery of nodes by cloud provider, as well as lookup vnics. Other providers may also have such use cases.

Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

/kind feature

[sbueringer]

For example in OCI, compartment id can be set on nodes which will help in discovery of nodes by cloud provider, as well as lookup vnics.

I'm fine in general with the requirement.

Just a question about this specific case. I don't exactly know what a compartment is, but I wonder if it's the kind of metadata that in some clouds is provided by the cloud-provider running inside of the workload cluster.

[fabriziopandini]

/triage accepted
/help

I think we should implement label propagation first, and then build on top of it

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/triage accepted
/help

I think we should implement label propagation first, and then build on top of it

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[kfox1111]

taints is important too. should that be another issue too or wrapped up with labels or annotations?

[musaprg]

Label propagation has been supported by #7173. It would be a good time to step onto annotations. (AFAIK it's not implemented for now)

We may have two questions that need to have consensus:

Q1. Should we only allow annotations with predefined prefixes?
Q2. Which annotation value should have precedence if the specified one on the Machine object exists on the corresponding Node object?

For labels, CAPI only allows propagating managed labels. To achieve the original motivation of this PR, I suppose we anyway need to allow arbitrary annotations on Machine objects to be propagated.

In terms of Q2, for labels, ones on Machine objects always have precedence. IMO it would make sense to follow the same policy for annotations as well.

I'm interested in working on implementation and ready to prepare a PR.

[fabriziopandini]

/triage backlog

[k8s-ci-robot]

@fabriziopandini: The label(s) triage/backlog cannot be applied, because the repository doesn't have them.

In response to this:

/triage backlog

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fabriziopandini]

/priority backlog

[nrb]

#7731 has been finished for quite some time, and #11650 is currently in flight to sync labels from Machines to Nodes.

@fabriziopandini @sbueringer I understand your time is limited - what would you like to see in order to move this forward?

I think your questions are good & warrant some discussion, @musaprg. Do you still have bandwidth to collaborate on this?

[nrb]

/assign

[musaprg]

Yes, I'm still keen to work on this.

I've looked through #11650. We could take the same approach for propagating annotations.

[nrb]

/assign @musaprg

[blind3dd]

is this regarding potential CAPI feature that would inject annotations for instance with webhooks?

[nrb]

@blind3dd Yeah, syncing annotations from a CAPI Machine to the corresponding Node.

[nrb]

@sbueringer @fabriziopandini @musaprg

I've taken an initial pass at some code, but before I go further, I want to check with folks to get on the same page.

Currently, CAPI handles a specific set known of labels; anything outside of the set is ignored until #11650 merges. Even after that PR merges, users can still provide a set of known domains to sync, rather than any and all labels found on the Machine.

For annotations, right now the only annotations that are copied to a Machine are those defined in the MachineSet.Spec.Template.Annotations field (code).

When syncing a Machine's metadata to its Node, CAPI sets a small set of annotations for its own bookkeeping (code), but otherwise all labels are unaffected.

This leads to the questions from above, which we'll need to answer; maybe they should be in a CAEP?

Q1. Should we only allow annotations with predefined prefixes?

From my perspective, it should mirror the behavior that #11650 establishes for labels. I'll leave out the specific implementation details since it's not merged yet, but I think it would be best to treat labels and annotations as similarly as possible.

Q2. Which annotation value should have precedence if the specified one on the Machine object exists on the corresponding Node object?

This one's trickier. Should we perhaps make use of the SSA owner logic, perhaps? My understanding is that labels propagated from CAPI objects to their dependents will have the field's manager set to the relevant controller (MachineSet example)?

If we're able to say that annotations originating from the Machine are owned by CAPI, we could then leave any other annotations from external systems alone and avoid the precedence decision.

If we can't do that, I'm inclined to say that the Machine's value should win if there's a conflict.

[sbueringer]

is this regarding potential CAPI feature that would inject annotations for instance with webhooks?

No we would not inject annotations with a webhook. We would implement the same Machine => Node propagation of annotations that we already have for labels (https://cluster-api.sigs.k8s.io/reference/api/metadata-propagation#machine)

[sbueringer]

@nrb

From my perspective, it should mirror the behavior that #11650 establishes for labels. I'll leave out the specific implementation details since it's not merged yet, but I think it would be best to treat labels and annotations as similarly as possible.

tl;dr yes, details:

I would also treat annotations in a very similar way to labels.

For labels we ended up with syncing roughly the following labels per default (I'm ignoring the nuances/differences on how we match each of these):

• node-role.kubernetes.io
• node-restriction.kubernetes.io
• node.cluster.x-k8s.io

The first two are well-known labels from upstream k/k.

I think we should only sync node.cluster.x-k8s.io annotations per default (we can always add more if there are some well-known annotations that would make sense to sync per default)

Like for labels, I think we should add a flag that allows to specify additional annotations to sync.

This leads to the questions from above, which we'll need to answer; maybe they should be in a CAEP?

Probably it makes sense to generalize the "Label Sync Between Machines and underlying Kubernetes Nodes" proposal to also include annotations (compared to copy&pasting most of the proposal). Would be good to get @fabriziopandini's take on that though.

Q2. Which annotation value should have precedence if the specified one on the Machine object exists on the corresponding Node object?

I would implement it the exact same way as we did for labels. We tried to use SSA at the time, but then realized it doesn't work: #8417.

If we're able to say that annotations originating from the Machine are owned by CAPI, we could then leave any other annotations from external systems alone and avoid the precedence decision.

This should be what the current implementation achieves - just without SSA.

IIRC overall this comes down to that the annotations on the Machine take precedence if there a annotation key conflict, and I think that's okay (after all we're restricting the annotations that are synced, we're not just syncing everything)

[fabriziopandini]

I agree with @sbueringer
Might be worth clarify a little bit two points:

CAPI is authoritative for labels it syncs, there is no co-ownership or conflicts like in SSA (see code). That means that:

• if someone removes from the node a label managed by CAPI, CAPI will set it back
• if someone changes the value for a label managed by CAPI, CAPI will revert this change
• If CAPI removes a label it manages, the label will go away
  I think we should implement the same for annotations

I hope we should only implement propagation of annotation from Machine to Node (extending this proposal), and keep propagation of metadata across CAPI object as it is today (this proposal)

Last but not least, a reminder that above proposals are not yet implemented for MachinePools