cluster-api-provider-aws
cluster-api-provider-aws copied to clipboard
WIP: Update AWSMachine webhook validate logic on update to be consistent
Update AWSMachine webhook validate logic on update to be consistent with validation on creation.
What type of PR is this? /kind bug
What this PR does / why we need it:
Makes the AWSMachine webhook validation on update consistent with on create. Also fixes a mistake introduced in the validateNonRootVolumes
logic introduced by #2468.
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 #3109 & #3105
Special notes for your reviewer:
Checklist:
- [ ] squashed commits
- [ ] includes documentation
- [ ] adds unit tests
- [ ] adds or updates e2e tests
Hi @cnmcavoy. 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 /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test
label.
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/test-infra repository.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vincepri for approval by writing /assign @vincepri
in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve
in a comment
Approvers can cancel approval by writing /approve cancel
in a comment
Need to address https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/3109#issuecomment-1244145506 first.
/ok-to-test
@cnmcavoy is this still WIP? Would you be able to continue with this PR?
@cnmcavoy is this still WIP? Would you be able to continue with this PR?
I have an open question here: https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/3109#issuecomment-1244145506
If we don't want to address, this can be unmarked as WIP and merged.
I added logic to the AWSMachineTemplate so that the validation behavior is consistent with the existing on create behavior of AWSMachine's as well.
/test pull-cluster-api-provider-aws-e2e-blocking /test pull-cluster-api-provider-aws-e2e-eks
@cnmcavoy I think the update part is still not done right, as discussed in the original issue?
@Ankitasw I realize there description of this change is lacking and the issue it fixes is rather brief as well. Here's a summary of the changes in the PR:
The existing AWSMachine webhook validates onCreate
that:
- Cloud Init is configured correctly
- [immutable] Ignition is configured correctly
- [immutable] root volumes are configured with valid settings
- [immutable] non-root volumes are configured with valid settings
- [immutable] ssh key name is valid
- additional security groups are configured with valid IDs or filters
- additional aws tags are valid
The existing AWSMachine webhook validates onUpdate
that:
- Cloud Init is configured correctly
- additional aws tags are valid
- immutable spec fields are immutable
My change addresses the missing validation of the security groups onUpdate
for AWSMachine. It also fixes the bug identified in #3105, where the wrong volume was compared in one of the validation non-root volumes.
Additionally, the existing AWSMachineTemplate webhook validates that onCreate
that:
- [immutable] Ignition is configured correctly
- [immutable] root volumes are configured with valid settings
- [immutable] non-root volumes are configured with valid settings
The existing AWSMachineTemplate webhook validates onUpdate
that:
- Modifying the secret backend store from the default (
""
) to the SSM Parameter store is not a distinct update from the default (""
) state for older crd versions. - immutable spec fields are immutable
My change addresses all of the missing validation from the AWSMachineTemplate onCreate
webhook which is present in the AWSMachine onCreate
webhook, and introduces new validation onCreate
for the AWSMachineTemplate to cover:
- Cloud Init is configured correctly
- ssh key name is valid
- additional security groups are configured with valid IDs or filters
- additional aws tags are valid
There shouldn't be any changes to the API contract in this PR, only enforcement of the existing contract. Is there anything that is incorrect or I left out in this? Please let me know if so.
/test pull-cluster-api-provider-aws-e2e-blocking
/easycla
/lgtm /approve
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: Ankitasw
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [Ankitasw]
Approvers can indicate their approval by writing /approve
in a comment
Approvers can cancel approval by writing /approve cancel
in a comment