feat: validate AzureCluster update for location property #522

jackfrancis · 2020-04-10T22:26:25Z

What this PR does / why we need it:

This PR adds CRUD validations as webhooks to the AzureCluster resource type.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #497

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

k8s-ci-robot · 2020-04-10T22:26:26Z

@jackfrancis: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 · 2020-04-10T22:26:32Z

Welcome @jackfrancis!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-04-10T22:26:33Z

Hi @jackfrancis. 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 · 2020-04-10T22:26:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jackfrancis
To complete the pull request process, please assign cecilerobertmichon
You can assign the PR to them by writing /assign @cecilerobertmichon in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

devigned · 2020-04-10T22:57:01Z

@jackfrancis please add a release note.

nader-ziada

Thanks @jackfrancis for your PR 👍

you still need to update the webhook config, check the machine webhook for an example

cluster-api-provider-azure/config/webhook/manifests.yaml

Line 3 in 59e909e

apiVersion: admissionregistration.k8s.io/v1beta1

api/v1alpha3/azurecluster_webhook.go

nader-ziada · 2020-04-13T12:38:13Z

api/v1alpha3/azurecluster_webhook_test.go

+	}
+}
+
+func createAzureCluster(t *testing.T, location string) *AzureCluster {


the input t is not used

nader-ziada · 2020-04-13T12:41:31Z

api/v1alpha3/azurecluster_webhook_test.go

+		},
+	}
+	for _, tc := range tests {
+		tc := tc


I don't think this is needed

You are correct, it is not needed because t.Run is running synchronously. I usually advocate for this style as it's often missed when someone goes to make a test parallel and then runs into the pernicious issue of closure capture.

https://play.golang.org/p/GGrFU73Na8S

Added the parallel invocation to settle the debate :)

nader-ziada · 2020-04-13T12:42:37Z

api/v1alpha3/azurecluster_webhook_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.cluster.ValidateUpdate(tc.updated)
+			if tc.wantErr {
+				g.Expect(err).To(HaveOccurred())


can we also check that the returned error is as expected

serbrech

I would recommend to separate the validation library from the webhook, and use the library in the webhook instead.

CecileRobertMichon · 2020-05-04T20:43:28Z

/ok-to-test

CecileRobertMichon · 2020-05-05T20:24:34Z

@jackfrancis you need to run make generate

from the test log above: generated files are out of date, run make generate

Also did you see @serbrech's comment above? See azuremachine_validation for an example. I think we can mirror the same separation here.

jackfrancis · 2020-05-05T20:41:14Z

api/v1alpha3/azurecluster_webhook.go

+		return fmt.Errorf("update object is not a AzureCluster type")
+	}
+
+	if azureCluster.Spec.Location != r.Spec.Location {


@CecileRobertMichon @serbrech do we really want to create a re-usable function to validate that two strings aren't equal?

I think @serbrech's point is that the _webhook file shouldn't contain the actual logic for validation but instead each object should have its own _validation file with functions that can each be unit tested separately. Right now it might be just a string comparison because you only setup the initial skeleton for Cluster update validation but as we add more validations the logic will become more complex. Might as well start things the right way so others can be guided to add further validations in the right place. See how ValidateCronJob is implemented here for an example.

@serbrech please correct me if I'm wrong.

+1 to what Cecile said, just to setup the structure for future validations

k8s-ci-robot · 2020-05-12T21:32:13Z

@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 · 2020-05-26T21:50:33Z

@jackfrancis a lot of this surface area has changed and conflicts with #604, I think the easiest way might be to close this PR and start fresh. The immutable fields can be validated as part of https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/api/v1alpha3/azurecluster_validation.go#L37 by passing in the old AzureCluster object as part of validateUpdate(). Feel free to unassign if you don't think you'll have time to get to this in the near term.

jackfrancis · 2020-05-27T18:20:15Z

Happy to start over!

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 10, 2020

k8s-ci-robot requested review from awesomenix and nader-ziada April 10, 2020 22:26

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 10, 2020

nader-ziada reviewed Apr 13, 2020

View reviewed changes

serbrech reviewed Apr 13, 2020

View reviewed changes

jackfrancis added 3 commits May 4, 2020 13:21

chore: validate AzureCluster

7a6d269

test: address test feedback

11e2685

test: remove unused t reference in createAzureCluster func

d65fdfb

jackfrancis force-pushed the azurecluster-validate branch from bae65c5 to d65fdfb Compare May 4, 2020 20:26

jackfrancis changed the title ~~[WIP] chore: validate AzureCluster~~ feat: validate AzureCluster update for location property May 4, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2020

jackfrancis mentioned this pull request May 4, 2020

Add validations for AzureCluster Updates #497

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 4, 2020

chore: generated code

1da7831

jackfrancis commented May 5, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2020

jackfrancis closed this May 27, 2020

jackfrancis deleted the azurecluster-validate branch December 9, 2022 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: validate AzureCluster update for location property #522

feat: validate AzureCluster update for location property #522

jackfrancis commented Apr 10, 2020 •

edited by CecileRobertMichon

Loading

k8s-ci-robot commented Apr 10, 2020

k8s-ci-robot commented Apr 10, 2020

k8s-ci-robot commented Apr 10, 2020

k8s-ci-robot commented Apr 10, 2020

devigned commented Apr 10, 2020

nader-ziada left a comment

nader-ziada Apr 13, 2020

nader-ziada Apr 13, 2020

devigned Apr 13, 2020 •

edited

Loading

jackfrancis May 4, 2020

nader-ziada Apr 13, 2020

serbrech left a comment

CecileRobertMichon commented May 4, 2020

CecileRobertMichon commented May 5, 2020

jackfrancis May 5, 2020

CecileRobertMichon May 6, 2020

nader-ziada May 7, 2020

k8s-ci-robot commented May 12, 2020

CecileRobertMichon commented May 26, 2020

jackfrancis commented May 27, 2020

feat: validate AzureCluster update for location property #522

feat: validate AzureCluster update for location property #522

Conversation

jackfrancis commented Apr 10, 2020 • edited by CecileRobertMichon Loading

k8s-ci-robot commented Apr 10, 2020

k8s-ci-robot commented Apr 10, 2020

k8s-ci-robot commented Apr 10, 2020

k8s-ci-robot commented Apr 10, 2020

devigned commented Apr 10, 2020

nader-ziada left a comment

Choose a reason for hiding this comment

nader-ziada Apr 13, 2020

Choose a reason for hiding this comment

nader-ziada Apr 13, 2020

Choose a reason for hiding this comment

devigned Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

jackfrancis May 4, 2020

Choose a reason for hiding this comment

nader-ziada Apr 13, 2020

Choose a reason for hiding this comment

serbrech left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented May 4, 2020

CecileRobertMichon commented May 5, 2020

jackfrancis May 5, 2020

Choose a reason for hiding this comment

CecileRobertMichon May 6, 2020

Choose a reason for hiding this comment

nader-ziada May 7, 2020

Choose a reason for hiding this comment

k8s-ci-robot commented May 12, 2020

CecileRobertMichon commented May 26, 2020

jackfrancis commented May 27, 2020

jackfrancis commented Apr 10, 2020 •

edited by CecileRobertMichon

Loading

devigned Apr 13, 2020 •

edited

Loading