disable AKS control plane create webhook when paused #3341
Conversation
k8s-ci-robot
added
release-note
|
[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/M
|
This is for my own understanding;
|
I believe something like what you outline is possible. I'm not sure that a user creating and then breaking its own CAPI cluster in this way would qualify as an attack vector. The existing create webhook is meant as a user convenience of last resort, to remind the user if this data is pre-populated at cluster creation time, that that data configuration is invalid (the AKS service will be authoritative for populating the apiserver endpoint during its own creation lifecycle, not the user) and not allowed by a user. |
jackfrancis
force-pushed
the
aks-host-port-webhook-paused
branch
from
2215645
to
595c27d
Compare
|
/cherry-pick release-1.8 |
|
@CecileRobertMichon: once the present PR merges, I will cherry-pick it on top of release-1.8 in a new PR and assign it to you. 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. |
|
/retest |
| Namespace: m.Namespace, | ||
| Name: m.Name, |
There was a problem hiding this comment.
Is this making a similar assumption to the one that's being addressed in #3322? The owner references would probably be the easiest place to check for the Cluster's namespace/name, but that isn't set until after the AzureManagedControlPlane has been created. Otherwise it seems like we'd have to list all Clusters in all namespaces and see if any match this AzureManagedControlPlane and are paused.
There was a problem hiding this comment.
It's definitely true that the name of an AzureManagedControlPlane can be different from the name of the referencing cluster.
I think we can probably introspect each Cluster for Spec.ControlPlaneRef.Name == the name of the AzureManagedControlPlane; is that what you're thinking?
There was a problem hiding this comment.
Yeah, I was thinking of exactly that. Not sure there's any other way to correlate those right when the object is created.
There was a problem hiding this comment.
One other thought I had was to list with a field selector on the spec.controlPlaneRef.* fields but it looks like those aren't supported.
There was a problem hiding this comment.
And do we know for sure that clusterctl move will always create the Cluster before the AzureManagedCluster? The CAPI webhooks appear to allow either to be created first. If we have to account for when no matching Cluster exists yet here, I'm thinking we'd probably want to skip the controlPlaneEndpoint checks in that case to make sure we're not blocking a valid clusterctl move?
There was a problem hiding this comment.
Indeed maybe the most practical solution here is to remove these Create checks entirely. They were implemented defensively to protect the user from accidentally doing something wrong, but the outcome of "wrong" in this case is worst-case scenario a never-actually-manageable cluster, but best-case scenario the AKS controllers overwrite those user-provided values with the correct ones from the AKS API anyways, and so it literally has not effect. (We'd need to look into this to know for sure which outcome is the actual one. But either of those is probably not sufficiently catastrophic or likely to happen to justify all this additional webhook complexity.)
Thoughts?
There was a problem hiding this comment.
Indeed maybe the most practical solution here is to remove these Create checks entirely.
I tend to agree
the AKS controllers overwrite those user-provided values with the correct ones from the AKS API anyways
are we sure this is still the case? I seem to remember we changed it to only set them when empty
There was a problem hiding this comment.
Indeed UT tells the tale:
I'll submit a PR that removes create webhooks for this and AzureCluster and we can discuss there.
|
/close |
|
@CecileRobertMichon: 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. |
|
@jackfrancis kubernetes-sigs/cluster-api#8322 should help us do something like this in the future |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR disables the create webhook for AKS control plane data when the cluster is paused, which is the cluster state that a "clusterctl move" operation yields.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Contributes to #3303
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: