Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ Make sure IPAddressClaim has a cluster name label. #9883

Closed
wants to merge 1 commit into from
Closed

✨ Make sure IPAddressClaim has a cluster name label. #9883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions exp/ipam/internal/webhooks/ipaddressclaim.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/webhook"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
ipamv1 "sigs.k8s.io/cluster-api/exp/ipam/api/v1beta1"
)

Expand All @@ -54,6 +55,15 @@ func (webhook *IPAddressClaim) ValidateCreate(_ context.Context, obj runtime.Obj
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an IPAddressClaim but got a %T", obj))
}

// 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'")
}
Comment on lines +58 to +65
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who's responsible for adding this label? If users, I'd say this should be a field, like spec.Cluster like in other objects

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup got it. Sorry just thought my point somewhat fits to yours :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @fabriziopandini can add more thoughts in this regards

Copy link
Member

@schrej schrej Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is not needed, feel free to close it.

Copy link
Member

@schrej schrej Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if claim.Spec.PoolRef.APIGroup == nil {
return nil, field.Invalid(
field.NewPath("spec.poolRef.apiGroup"),
Expand Down
14 changes: 12 additions & 2 deletions exp/ipam/internal/webhooks/ipaddressclaim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/utils/ptr"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
ipamv1 "sigs.k8s.io/cluster-api/exp/ipam/api/v1beta1"
)

Expand All @@ -48,8 +49,12 @@ func TestIPAddressClaimValidateCreate(t *testing.T) {
expectErr bool
}{
{
name: "should accept a valid claim",
claim: getClaim(func(addr *ipamv1.IPAddressClaim) {}),
name: "should accept a valid claim",
claim: getClaim(func(addr *ipamv1.IPAddressClaim) {
addr.GetObjectMeta().SetLabels(map[string]string{
clusterv1.ClusterNameLabel: "test-cluster",
})
}),
expectErr: false,
},
{
Expand All @@ -59,6 +64,11 @@ func TestIPAddressClaimValidateCreate(t *testing.T) {
}),
expectErr: true,
},
{
name: "should reject an IPAddressClaim that doesn't have a cluster name label",
claim: getClaim(func(addr *ipamv1.IPAddressClaim) {}),
expectErr: true,
},
}

for i := range tests {
Expand Down
Loading