Add support for validation on AzureName and ResourceGroup immutability #2260

super-harsh · 2022-05-03T20:48:57Z

What this PR does / why we need it:

We had an issue where changing AzureName and Resource groups would abandon the old resource and creates a new one. Which is solved using validation web-hook on UPDATE for immutable fields in this PR.

This PR includes:

Validation for AzureName immutability
Validation for Resource Group immutability
Helper method to handle more immutable fields in future
Edge case test for AzureName and ResourceGroup

… created

...internal/codegen/testdata/ArmResource/Arm_test_dependent_resource_and_ownership_azure.golden

Porges

see comment

codecov-commenter · 2022-05-03T21:37:17Z

Codecov Report

Merging #2260 (3794d9e) into main (de18cf8) will increase coverage by 0.04%.
The diff coverage is 23.49%.

@@            Coverage Diff             @@
##             main    #2260      +/-   ##
==========================================
+ Coverage   53.11%   53.15%   +0.04%     
==========================================
  Files         808      809       +1     
  Lines      248049   248769     +720     
==========================================
+ Hits       131744   132238     +494     
- Misses      97525    97700     +175     
- Partials    18780    18831      +51

Impacted Files	Coverage Δ
...ha1api20200801preview/role_assignment_types_gen.go	`26.82% <0.00%> (-0.31%)`	⬇️
...v1beta20200801preview/role_assignment_types_gen.go	`52.82% <0.00%> (-0.61%)`	⬇️
...tch/v1alpha1api20210101/batch_account_types_gen.go	`40.18% <0.00%> (-0.16%)`	⬇️
...pi/batch/v1beta20210101/batch_account_types_gen.go	`54.08% <0.00%> (-0.21%)`	⬇️
...alpha1api20201201/redis_firewall_rule_types_gen.go	`22.64% <0.00%> (-0.37%)`	⬇️
...alpha1api20201201/redis_linked_server_types_gen.go	`26.64% <0.00%> (-0.38%)`	⬇️
...lpha1api20201201/redis_patch_schedule_types_gen.go	`32.14% <0.00%> (-0.37%)`	⬇️
...2/api/cache/v1alpha1api20201201/redis_types_gen.go	`40.66% <0.00%> (-0.20%)`	⬇️
...api20210301/redis_enterprise_database_types_gen.go	`41.45% <0.00%> (-0.28%)`	⬇️
.../v1alpha1api20210301/redis_enterprise_types_gen.go	`35.81% <0.00%> (-0.33%)`	⬇️
... and 177 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de18cf8...3794d9e. Read the comment docs.

…-service-operator into feature/immutability

v2/api/authorization/v1alpha1api20200801preview/role_assignment_types_gen.go

v2/internal/controllers/edge_case_test.go

matthchr · 2022-05-04T01:42:14Z

v2/internal/controllers/edge_case_test.go

+	newAzureName := "test123"
+	old := acct.DeepCopy()
+	acct.Spec.AzureName = newAzureName
+	tc.PatchAndExpectError(old, acct)


Maybe instead of doing PatchAndExpectError we should just have a Patch that returns the error. Then you can actually assert that it has the right shape as well?

We already have Patch which expects error to be nil. Can make this one return error and then assert on if error is not nil

Totally agree that's the right approach

v2/tools/generator/internal/functions/kubernetes_admissions_validations.go

theunrepentantgeek · 2022-05-04T01:50:19Z

v2/pkg/genruntime/admissions.go

+	}
+
+	if oldObj.AzureName() != newObj.AzureName() {
+		errs = append(errs, errors.Errorf("updating 'AzureName()' is not allowed for '%s", oldObj.GetName()))


Why is AzureName written as though it's a method call?

Suggested change

errs = append(errs, errors.Errorf("updating 'AzureName()' is not allowed for '%s", oldObj.GetName()))

errs = append(errs, errors.Errorf("updating 'AzureName( is not allowed for '%s", oldObj.GetName()))

Also, does oldObj.GetName() return a fully qualified GVK or only a partial name?

Just the partial name.. Have also added added kind as well now. Or do you think GVK is better?

We probably dont need version as this error is for all versions always I think? But G + K is good.

theunrepentantgeek · 2022-05-04T01:52:32Z

...nal/codegen/testdata/ArmResource/Arm_oneof_resource_conversion_on_arm_type_only_azure.golden

+}
+
+// validateImmutableProperties validates all immutable properties
+func (resource *FakeResource) validateImmutableProperties(old runtime.Object) error {


We suppress some pipeline stages for these golden tests as they focus on property structure and the functions are noise - perhaps validation should be suppressed too? #unsure

We already have validateResourceReferences in the golden tests though

theunrepentantgeek · 2022-05-04T01:59:00Z

v2/pkg/genruntime/admissions.go

@@ -26,3 +28,22 @@ type Defaulter interface {
 	// CustomDefault performs custom defaults that are run in addition to the code generated defaults.
 	CustomDefault()
 }
+
+// ValidateImmutableProperties function validates the update on immutable properties.
+func ValidateImmutableProperties(oldObj MetaObject, newObj MetaObject) error {


I'm not sure "Immutable" is quite the right term for these properties, but I'm having trouble coming up with a reasonable alternative.

…-service-operator into feature/immutability

matthchr

Approved but take a look at my test comments as I think there's 1 more test you should have.

v2/internal/controllers/edge_case_test.go

theunrepentantgeek · 2022-05-05T02:12:07Z

v2/tools/generator/internal/functions/kubernetes_admissions_validations.go

+		ReceiverType: &dst.StarExpr{
+			X: receiverType,
+		},


There's a helper function in astbuilder for this.

Updated to use astbuilder.Deference

v2/tools/generator/internal/functions/kubernetes_admissions_validations.go

theunrepentantgeek · 2022-05-05T02:17:46Z

v2/tools/generator/internal/functions/kubernetes_admissions_validations.go

+	cast := astbuilder.TypeAssert(obj, dst.NewIdent("old"), astbuilder.Dereference(receiver.AsType(codeGenerationContext)))
+	checkAssert := astbuilder.ReturnIfNotOk(astbuilder.Nil())
+
+	returnStmt := astbuilder.Returns(


Suggest configuring this to have a blank line before the return statement.

…lidations.go Co-authored-by: Bevan Arps <[email protected]>

…-service-operator into feature/immutability

v2/internal/controllers/edge_case_test.go

…lyCreated test

Porges · 2022-05-08T22:42:41Z

Somewhat fixes #1443, but we still need support for more (custom) fields there.

…-service-operator into feature/immutability

Harshdeep Singh added 4 commits May 3, 2022 22:24

Add support for setting up validation webhook for immutable probs

db7630d

Add resource creation check to ignore validation when resource is not…

35cba03

… created

Update generated code

1a04f74

Add immutability edge case tests

0fce028

super-harsh requested review from davefellows, theunrepentantgeek, Porges, matthchr and babbageclunk as code owners May 3, 2022 20:48

Harshdeep Singh and others added 2 commits May 4, 2022 02:28

Update golden test files

0fdfae6

Merge branch 'main' into feature/immutability

677a100

Porges reviewed May 3, 2022

View reviewed changes

...internal/codegen/testdata/ArmResource/Arm_test_dependent_resource_and_ownership_azure.golden Outdated Show resolved Hide resolved

Porges requested changes May 3, 2022

View reviewed changes

Harshdeep Singh added 2 commits May 4, 2022 04:51

Add validations in genruntime package

268ebd9

Merge branch 'feature/immutability' of https://github.com/Azure/azure…

7b26fe7

…-service-operator into feature/immutability

Porges approved these changes May 4, 2022

View reviewed changes

Merge branch 'main' into feature/immutability

7387fde

matthchr reviewed May 4, 2022

View reviewed changes

theunrepentantgeek reviewed May 4, 2022

View reviewed changes

Harshdeep Singh added 2 commits May 5, 2022 02:41

Add test cases for rg update special case; Refactor indentation

630e6fd

Merge branch 'feature/immutability' of https://github.com/Azure/azure…

68904c4

…-service-operator into feature/immutability

matthchr approved these changes May 4, 2022

View reviewed changes

Harshdeep Singh added 2 commits May 5, 2022 03:32

gofmt edge_case_test.go

ab0f590

Add new test case; Improve logging

099be95

theunrepentantgeek reviewed May 5, 2022

View reviewed changes

super-harsh and others added 4 commits May 5, 2022 19:40

Update v2/tools/generator/internal/functions/kubernetes_admissions_va…

7edf5c0

…lidations.go Co-authored-by: Bevan Arps <[email protected]>

Use astbuilder for starExpr; Add extra line before return

dcd76d0

Update golden tests and generated code

af29577

Merge branch 'feature/immutability' of https://github.com/Azure/azure…

9cabf1d

…-service-operator into feature/immutability

matthchr reviewed May 5, 2022

View reviewed changes

v2/internal/controllers/edge_case_test.go Outdated Show resolved Hide resolved

Harshdeep Singh and others added 2 commits May 5, 2022 22:00

Update the name assignment in Test_AzureName_IsMutableIfNotSuccessful…

3e78c41

…lyCreated test

Merge branch 'main' into feature/immutability

3794d9e

Harshdeep Singh and others added 4 commits May 9, 2022 19:03

Update ValidateImmutableProperties name to validateWriteOnceProperties

cdeae97

Merge branch 'feature/immutability' of https://github.com/Azure/azure…

6addb2a

…-service-operator into feature/immutability

Merge branch 'main' into feature/immutability

2c15dac

Merge branch 'main' into feature/immutability

823c5c9

super-harsh merged commit 3264b31 into main May 9, 2022

super-harsh deleted the feature/immutability branch May 9, 2022 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for validation on AzureName and ResourceGroup immutability #2260

Add support for validation on AzureName and ResourceGroup immutability #2260

super-harsh commented May 3, 2022 •

edited

Loading

Porges left a comment

codecov-commenter commented May 3, 2022 •

edited

Loading

matthchr May 4, 2022

super-harsh May 4, 2022

matthchr May 4, 2022

theunrepentantgeek May 4, 2022

theunrepentantgeek May 4, 2022

super-harsh May 4, 2022 •

edited

Loading

matthchr May 5, 2022

theunrepentantgeek May 4, 2022

super-harsh May 4, 2022

theunrepentantgeek May 4, 2022

matthchr left a comment

theunrepentantgeek May 5, 2022

super-harsh May 5, 2022

theunrepentantgeek May 5, 2022

super-harsh May 5, 2022

Porges commented May 8, 2022 •

edited

Loading

	errs = append(errs, errors.Errorf("updating 'AzureName()' is not allowed for '%s", oldObj.GetName()))
	errs = append(errs, errors.Errorf("updating 'AzureName( is not allowed for '%s", oldObj.GetName()))

Add support for validation on AzureName and ResourceGroup immutability #2260

Add support for validation on AzureName and ResourceGroup immutability #2260

Conversation

super-harsh commented May 3, 2022 • edited Loading

Porges left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 3, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

super-harsh May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Porges commented May 8, 2022 • edited Loading

super-harsh commented May 3, 2022 •

edited

Loading

codecov-commenter commented May 3, 2022 •

edited

Loading

super-harsh May 4, 2022 •

edited

Loading

Porges commented May 8, 2022 •

edited

Loading