✨ Allow changing DNSNameservers in subnet config for OpenstackCluster #2511
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
|
|
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @pbasov! |
|
Hi @pbasov. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I absolutely hated making DNSNameservers mutable in the webhook, it was tricky because subnets is a list (even though we support only ONE per network). Surely there's a better way to manage mutability for the config. |
k8s-ci-robot
added
cncf-cla: yes
|
So yeah, why are we doing these webhook gymnastics when kubebuilder has immutable fields? |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Likely because no-one has taken the time to refactor it yet |
s3rj1k
force-pushed
the
dns-update
branch
3 times, most recently
from
d5ed5f7 to
aa1e818
Compare
k8s-ci-robot
added
size/L
s3rj1k
force-pushed
the
dns-update
branch
2 times, most recently
from
c87b8b0 to
a9f9c62
Compare
k8s-ci-robot
added
size/XL
| var needsUpdate bool | ||
| if len(desiredNameservers) != len(currentNameservers) { | ||
| needsUpdate = true | ||
| } else { | ||
| needsUpdate = !equality.Semantic.DeepEqual(currentNameservers, desiredNameservers) | ||
| } |
There was a problem hiding this comment.
Probably not that big of a deal but I'm curious why don't we just drop needsUpdate and use the equality all the time. Is this an optimization or is it simply something I don't know? :)
There was a problem hiding this comment.
yea, len is much quicker vs equality.Semantic.DeepEqual but for sure we can drop that optimization and keep code more simple
There was a problem hiding this comment.
nit: Given that they're just slices of strings I'd personally do something a bit less voodoo than Semantic.DeepEqual like slices.Sort(); slices.Equal().
There was a problem hiding this comment.
equality.Semantic.DeepEqual is basically what most of people use in K8s land (before it was regular reflect.DeepEqual), but sure we can switch to sort and equal approach
There was a problem hiding this comment.
equality.Semantic.DeepEqual is used in multiple places, so I guess it makes sense to have a wrapper function to compare slices, something like this
func EqualIgnoreOrder[T cmp.Ordered](a, b []T) bool {
if len(a) != len(b) {
return false
}
if len(a) == 0 {
return true
}
aCopy := slices.Clone(a)
bCopy := slices.Clone(b)
slices.Sort(aCopy)
slices.Sort(bCopy)
return slices.Equal(aCopy, bCopy)
}
There was a problem hiding this comment.
This is just a nit, and mostly because I've spent time in the past reading Semantic.DeepEqual trying to determine the precise behaviour of its edge cases.
I only mention it at all because my personal preference would be to use something more explicit, especially when the data structure is simple as here. But this works and the PR has great test coverage, so there's no pressing need to update this.
There was a problem hiding this comment.
I am fine replacing if we bring similar helper in, I would hate to replace 1 func call with len,copy,sort,equal checks in multiple places, this can be followup if that makes sense
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
k8s-ci-robot
added
approved
k8s-ci-robot
removed
lgtm
There was a problem hiding this comment.
Thanks for this, and especially for the amazing test coverage 👌
I'm not 100% confident of my assertion that order of DNSNameservers doesn't matter. It feels like one of those things that shouldn't matter, but might in practise. Let me know if I'm wrong about that.
Strong preference for an extra unit test covering an ordering-only difference, with the behaviour asserted being whatever we decide it should be.
Suggestion inline for a significant simplification of the webhook. Let me know if that makes sense.
| var needsUpdate bool | ||
| if len(desiredNameservers) != len(currentNameservers) { | ||
| needsUpdate = true | ||
| } else { | ||
| needsUpdate = !equality.Semantic.DeepEqual(currentNameservers, desiredNameservers) | ||
| } |
There was a problem hiding this comment.
nit: Given that they're just slices of strings I'd personally do something a bit less voodoo than Semantic.DeepEqual like slices.Sort(); slices.Equal().
| DNSNameservers: []string{"8.8.8.8", "8.8.4.4"}, | ||
| }, nil) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Please can we have another test where they're the same, but in a different order? I think we should not update in this case.
There was a problem hiding this comment.
We think that order probably does matter, so please can we have another test where the set of nameservers is identical but the order is different? We should assert that they're updated to be in the specified order.
| // Build maps of subnets by CIDR | ||
| oldSubnetMap := make(map[string]infrav1.SubnetSpec) | ||
| newSubnetMap := make(map[string]infrav1.SubnetSpec) |
There was a problem hiding this comment.
Suggestion:
You only need oldSubnetMap here, but make it contain pointers to infrav1.SubnetSpec instead so you can modifiy the originals in place. That removes the need for some of the gymnastics below.
| } | ||
|
|
||
| // Check if all new subnets have matching old subnets with the same CIDR | ||
| for _, newSubnet := range newObj.Spec.ManagedSubnets { |
There was a problem hiding this comment.
| for _, newSubnet := range newObj.Spec.ManagedSubnets { | |
| for i := range newObj.Spec.ManagedSubnets { | |
| newSubnet := &newObj.Spec.ManagedSubnets[i] |
We've now got pointers to the old and new original objects in place.
| // Create modified copies of the subnets with DNSNameservers cleared | ||
| oldSubnets := make([]infrav1.SubnetSpec, 0, len(oldObj.Spec.ManagedSubnets)) | ||
| newSubnets := make([]infrav1.SubnetSpec, 0, len(newObj.Spec.ManagedSubnets)) | ||
| for _, subnet := range oldObj.Spec.ManagedSubnets { | ||
| subnetCopy := subnet | ||
| subnetCopy.DNSNameservers = nil | ||
| oldSubnets = append(oldSubnets, subnetCopy) | ||
|
|
||
| if newSubnet, exists := newSubnetMap[subnet.CIDR]; exists { | ||
| newSubnetCopy := newSubnet | ||
| newSubnetCopy.DNSNameservers = nil | ||
| newSubnets = append(newSubnets, newSubnetCopy) | ||
| } | ||
| } | ||
|
|
||
| oldObj.Spec.ManagedSubnets = oldSubnets | ||
| newObj.Spec.ManagedSubnets = newSubnets |
There was a problem hiding this comment.
We don't need this any more.
| // Check if AllocationPools have changed | ||
| if !equality.Semantic.DeepEqual(oldSubnet.AllocationPools, newSubnet.AllocationPools) { | ||
| allErrs = append(allErrs, field.Forbidden( | ||
| field.NewPath("spec", "managedSubnets").Child("allocationPools"), | ||
| "cannot modify allocation pools in existing subnet", | ||
| )) | ||
| } | ||
|
|
||
| newSubnetMap[newSubnet.CIDR] = newSubnet |
There was a problem hiding this comment.
| // Check if AllocationPools have changed | |
| if !equality.Semantic.DeepEqual(oldSubnet.AllocationPools, newSubnet.AllocationPools) { | |
| allErrs = append(allErrs, field.Forbidden( | |
| field.NewPath("spec", "managedSubnets").Child("allocationPools"), | |
| "cannot modify allocation pools in existing subnet", | |
| )) | |
| } | |
| newSubnetMap[newSubnet.CIDR] = newSubnet | |
| // DNSNameservers is mutable | |
| oldSubnet.DNSNameservers = nil | |
| newSubnet.DNSNameservers = nil |
There was a problem hiding this comment.
suggest to change to something like this, what do you think?
// Allow changes only to DNSNameservers in ManagedSubnets spec
if newObj.Spec.ManagedSubnets != nil && oldObj.Spec.ManagedSubnets != nil {
// Check subnet count hasn't changed
if len(oldObj.Spec.ManagedSubnets) != len(newObj.Spec.ManagedSubnets) {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "managedSubnets"), "cannot add or remove subnets"))
return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
}
// Build map of old subnets by CIDR
oldSubnetMap := make(map[string]infrav1.SubnetSpec, len(oldObj.Spec.ManagedSubnets))
for _, subnet := range oldObj.Spec.ManagedSubnets {
oldSubnetMap[subnet.CIDR] = subnet
}
// Validate each new subnet
for i, newSubnet := range newObj.Spec.ManagedSubnets {
oldSubnet, exists := oldSubnetMap[newSubnet.CIDR]
if !exists {
allErrs = append(allErrs, field.Forbidden(
field.NewPath("spec", "managedSubnets").Index(i).Child("cidr"),
fmt.Sprintf("cannot change subnet CIDR to %s", newSubnet.CIDR),
))
continue
}
// Check AllocationPools haven't changed
if !equality.Semantic.DeepEqual(oldSubnet.AllocationPools, newSubnet.AllocationPools) {
allErrs = append(allErrs, field.Forbidden(
field.NewPath("spec", "managedSubnets").Index(i).Child("allocationPools"),
"cannot modify allocation pools in existing subnet",
))
}
}
// If there are errors, return early without modifying objects
if len(allErrs) > 0 {
return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
}
// Clear DNSNameservers from both objects
for i := range oldObj.Spec.ManagedSubnets {
oldObj.Spec.ManagedSubnets[i].DNSNameservers = nil
}
for i := range newObj.Spec.ManagedSubnets {
newObj.Spec.ManagedSubnets[i].DNSNameservers = nil
}
}
There was a problem hiding this comment.
I was trying to get rid of the AllocationPools check. I think this is fragile because if we added another field to this struct we'd need to also check it hasn't changed. The chances of that being forgotten are high.
There was a problem hiding this comment.
Should be solved by 7b5086d, thanks
|
/test pull-cluster-api-provider-openstack-e2e-test |
|
This looks great. Can you squash the commits and I'll approve. |
Co-authored-by: s3rj1k <[email protected]> Co-authored-by: Matthew Booth <[email protected]> Signed-off-by: s3rj1k <[email protected]>
Done, thanks for helping out and reviewing |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lentzi90, mdbooth, mnaser The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
What this PR does / why we need it:
Allows changing DNSNameservers in subnet config for OpenstackCluster after the initial deployment
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 #2468
/hold