cluster-api-provider-aws icon indicating copy to clipboard operation
cluster-api-provider-aws copied to clipboard

WIP: Update AWSMachine webhook validate logic on update to be consistent

Open cnmcavoy opened this issue 2 years ago • 4 comments

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

cnmcavoy avatar Sep 12 '22 18:09 cnmcavoy

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.

k8s-ci-robot avatar Sep 12 '22 18:09 k8s-ci-robot

[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.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Sep 12 '22 18:09 k8s-ci-robot

Need to address https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/3109#issuecomment-1244145506 first.

cnmcavoy avatar Sep 12 '22 18:09 cnmcavoy

/ok-to-test

Skarlso avatar Sep 12 '22 19:09 Skarlso

@cnmcavoy is this still WIP? Would you be able to continue with this PR?

Ankitasw avatar Dec 05 '22 17:12 Ankitasw

@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.

cnmcavoy avatar Dec 12 '22 16:12 cnmcavoy

I added logic to the AWSMachineTemplate so that the validation behavior is consistent with the existing on create behavior of AWSMachine's as well.

cnmcavoy avatar Jan 09 '23 19:01 cnmcavoy

/test pull-cluster-api-provider-aws-e2e-blocking /test pull-cluster-api-provider-aws-e2e-eks

Ankitasw avatar Feb 27 '23 07:02 Ankitasw

@cnmcavoy I think the update part is still not done right, as discussed in the original issue?

Ankitasw avatar Feb 27 '23 11:02 Ankitasw

@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.

cnmcavoy avatar Feb 27 '23 18:02 cnmcavoy

/test pull-cluster-api-provider-aws-e2e-blocking

Ankitasw avatar Feb 28 '23 13:02 Ankitasw

/easycla

Ankitasw avatar Feb 28 '23 14:02 Ankitasw

/lgtm /approve

Ankitasw avatar Feb 28 '23 14:02 Ankitasw

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Feb 28 '23 14:02 k8s-ci-robot