✨ Make sure IPAddressClaim has a cluster name label. #9883
Conversation
k8s-ci-robot
added
area/ipam
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/S
mcbenjemaa
changed the title
| // Ensures the IPAddressClaim has a cluster name label, to pause IPAddressClaim when the root cluster is paused | ||
| // and essentially prevent clusterctl move from failing. | ||
| if claim.GetObjectMeta().GetLabels()[clusterv1.ClusterNameLabel] == "" { | ||
| return nil, field.Invalid( | ||
| field.NewPath("metadata.labels"), | ||
| claim.GetObjectMeta().GetLabels(), | ||
| "the IPAddressClaim needs to have the cluster name label 'cluster.x-k8s.io/cluster-name'") | ||
| } |
There was a problem hiding this comment.
Who's responsible for adding this label? If users, I'd say this should be a field, like spec.Cluster like in other objects
There was a problem hiding this comment.
Are we sure we want to enforce that an IPAddressClaim has to belong to a cluster?
Today it's perfectly fine to use this CRD independent of a Cluster, which I could imagine can be quite useful in some cases.
(One rare use case that we have in CAPV e2e tests is that we basically have a separate cluster running once that we can use for IPAM purposes. This is entirely separate of mgmt clusters and the "reserved" IP addresses are then used in e2e mgmt clusters)
There was a problem hiding this comment.
Agreed, it seems this should have been discussed in the related issue. I was more commenting on the implementation of this constraint rather than looking at the semantics of the CRD; I haven't used these CRDs much, so I'll leave it to the folks responsible for them.
cc @rvanderp3
There was a problem hiding this comment.
Yup got it. Sorry just thought my point somewhat fits to yours :)
There was a problem hiding this comment.
Maybe @fabriziopandini can add more thoughts in this regards
There was a problem hiding this comment.
I agree with Stefan, I don't think we should enforce this. Providers should set the label when creating claims that are part of a cluster, but we shouldn't make it mandatory for every actor.
We (I) still need to create a proper IPAM contract description, which should include this as a requirement.
There was a problem hiding this comment.
If a label is required to link the claim to a cluster, the label should be a field, can be optional in this case; but we shouldn't rely on labels as a good API to expose to users in this context
There was a problem hiding this comment.
if this is not needed, feel free to close it.
There was a problem hiding this comment.
I agree, and so do the k8s api guidelines I think, that labels (and annotations) shouldn't be used if a property can be used instead.
Since the label is set on a lot of resources, which makes sense for querying them with clusterctl, the immediate thought is to use the label instead of a property to avoid duplicating data.
#10182
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: fa9ba2b9e7cc911c4eee6f46b7f487b02162c375
|
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
PR needs rebase. 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. |
k8s-ci-robot
added
the
needs-rebase
|
/close Given
|
|
@sbueringer: Closed this PR. In response to this:
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. |
What this PR does / why we need it:
The
clusterctl movefails because the IPAddressClaim is not paused so the controller in the target cluster will create IPAddress resouces.To pause the IPAddressClaim, we need to set the cluster label name.
This PR adds validation to the webhook of IPAddressClaim to ensure the cluster name label is always set.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Fixes #9478
/area ipam