standarize strict enforcement for immutable properties #2718
Conversation
k8s-ci-robot
added
release-note-none
|
/hold under active dev |
k8s-ci-robot
added
the
do-not-merge/hold
exp/api/v1beta1/webhook.go
Outdated
| immutableMessage = "field is immutable" | ||
| ) | ||
|
|
||
| func ensureStringSlicesAreEqual(a []string, b []string) bool { |
There was a problem hiding this comment.
(acknowledging that this was just copy-pasted and not changed in this PR)
It looks like this will interpret something like ["a", "a", "b"] and ["a", "b", "b"] as equal, which may not be expected. If some other mechanism is preventing duplicate elements or otherwise handling this appropriately then I don't think this is an issue.
There was a problem hiding this comment.
yeah I don't know what's going on here, I'm just going to fix this
exp/api/v1beta1/webhook.go
Outdated
| @@ -0,0 +1,117 @@ | |||
| /* | |||
| Copyright 2022 The Kubernetes Authors. | |||
There was a problem hiding this comment.
Would webhook_helper.go or webhook_util.go be a more accurate filename?
There was a problem hiding this comment.
With the expanded scope, moved all of this into util/webhook/validator.go where we get util/helper naming vibes for free.
k8s-ci-robot
added
the
needs-rebase
jackfrancis
force-pushed
the
validation-more-types
branch
from
08e4176 to
31e5ac8
Compare
k8s-ci-robot
removed
the
needs-rebase
jackfrancis
force-pushed
the
validation-more-types
branch
from
31e5ac8 to
5e9873a
Compare
k8s-ci-robot
added
size/XL
|
[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
release-note
jackfrancis
changed the title
We should keep behavior changes separate from refactor changes, especially validation behavior which is considered a breaking change for the user API fields and should have its own release note. Please split those into a separate PR with an explicit release note about which fields are now immutable/required. |
k8s-ci-robot
added
the
needs-rebase
|
@jackfrancis: 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. |
|
@CecileRobertMichon the part of the work that simply generalizes immutability checks into a common set of convenience funcs is here: #2741 |
|
/test ls |
|
@jackfrancis: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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. |
|
Opened #2795 instead |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR standardizes the way we validate immutable fields via webhook, and refactors existing, one-off validation implementations to re-use common funcs.
Created a new file
util/webhook/validator.goto host these exportable funcs so that we can easily re-use them acrossexp/api/andapi/.There are intentional, material changes in this PR to properly handle property immutability for AKS cluster properties that are immutable. The following
AzureManagedControlPlaneproperties are now being checked for strict immutability:Spec.SSHPublicKeySpec.DNSServiceIPSpec.NetworkPluginSpec.NetworkPolicySpec.LoadBalancerSKUAnd the following
AzureManagedMachinePoolproperties:Spec.OSDiskSizeGBSpec.MaxPodsSpec.OsDiskTypeWhich issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: