Support arbitrary ARM ID owners #3245

matthchr · 2023-09-01T21:40:05Z

This closes #2357.

Nearly all resources now support being owned by an arbitrary ARM ID. Only database user resources (MySQLUser, PostgreSQLUser) do not allow ARM ID ownership. This is because those resources must extract certain connection information from their parents and so must have access to their parents via the Kubernetes API.

ARM ID ownership behaves the following way:

Resources owned by an ARM ID will continue to attempt to reconcile if that ID doesn't exist. When the owning resource cannot be found, the resource will report a Ready=false with a "Warning" and additional details. If possible, avoid deleting the owning ARM resource without also deleting the resources in Kubernetes.
The credential being used for the resource must have the same subscription ID as the owning ARM resource. This means if your global credential is for Subscription A you cannot have ARM ID owners from subscription B unless you also create a serviceoperator.azure.com/credential-from for subscription B.
Resources that set both owner.name and owner.armID will be rejected by webhook.
Resources that attempt to set an ARM ID of the "wrong kind" (should be a storage account, gave it a virtual machine) will be rejected by webhook.

Documentation will come in a follow-up PR.

If applicable:

this PR contains documentation
this PR contains tests

v2/internal/resolver/resource_hierarchy.go

codecov-commenter · 2023-09-02T02:58:31Z

Codecov Report

Merging #3245 (3ccb5ed) into main (863737a) will decrease coverage by 0.10%.
The diff coverage is 47.57%.

@@            Coverage Diff             @@
##             main    #3245      +/-   ##
==========================================
- Coverage   54.46%   54.36%   -0.10%     
==========================================
  Files        1446     1446              
  Lines      616161   616614     +453     
==========================================
- Hits       335584   335227     -357     
- Misses     225588   226405     +817     
+ Partials    54989    54982       -7

Files Changed	Coverage Δ
...ta20220501storage/configuration_store_types_gen.go	`54.51% <0.00%> (+0.15%)`	⬆️
...0200801previewstorage/role_assignment_types_gen.go	`53.67% <0.00%> (+0.48%)`	⬆️
...h/v1beta20210101storage/batch_account_types_gen.go	`57.07% <0.00%> (+0.19%)`	⬆️
...pi20201201storage/redis_firewall_rule_types_gen.go	`43.16% <0.00%> (+0.94%)`	⬆️
...pi20201201storage/redis_linked_server_types_gen.go	`46.37% <0.00%> (+0.55%)`	⬆️
...i20201201storage/redis_patch_schedule_types_gen.go	`47.74% <0.00%> (+0.45%)`	⬆️
.../api/cache/v1api20201201storage/redis_types_gen.go	`59.71% <0.00%> (+0.18%)`	⬆️
...0301storage/redis_enterprise_database_types_gen.go	`55.00% <0.00%> (+0.34%)`	⬆️
...v1api20210301storage/redis_enterprise_types_gen.go	`54.85% <0.00%> (+0.39%)`	⬆️
...he/v1beta20201201/redis_firewall_rule_types_gen.go	`23.61% <0.00%> (-0.34%)`	⬇️
... and 287 more

... and 272 files with indirect coverage changes

matthchr · 2023-09-02T04:23:57Z

/ok-to-test sha=3ccb5ed

theunrepentantgeek

I think I understand what's going on here; will come back for another read through but don't want to block.

theunrepentantgeek · 2023-09-03T23:09:38Z

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

+		func(old runtime.Object) (admission.Warnings, error) {
+			return resource.validateOwnerReference()
+		},


Does this need to be wrapped? or, could you use resource.validateOwnerReference directly?

It's wrapped to discard the old parameter because it's not needed but I wanted to share a single impl for update/create. Since the signature matches for create it's called directly:

// createValidations validates the creation of the resource func (zone *DnsZone) createValidations() []func() (admission.Warnings, error) { return []func() (admission.Warnings, error){zone.validateResourceReferences, zone.validateOwnerReference} }

but for update because the signature doesn't match exactly it's wrapped. This is the same behavior as w/ validateResourceReferences so I think it makes sense to do the same thing here. LMK if you see a cleaner way.

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

theunrepentantgeek · 2023-09-03T23:23:42Z

v2/pkg/genruntime/core/errors.go

+func (e *SubscriptionMismatch) Error() string {
+	return e.cause.Error()
+}


Does SubscriptionMismatch need a custom error message as well?

I've changed how this is implemented so that it makes more sense. The error format is now part of this error type rather than just hardcoded in the file that called NewSubscriptionMismatchError

theunrepentantgeek · 2023-09-03T23:25:07Z

v2/pkg/genruntime/kubernetes_resource.go

+
+// GetResourceTypeAndProvider returns the provider and the array of resource types which represent the resource.
+// For example: Microsoft.Compute/virtualMachineScaleSets would return ("Microsoft.Compute", []string{"virtualMachineScaleSets"}, nil)
+func GetResourceTypeAndProvider(res ARMMetaObject) (string, []string, error) {


I think we already have this code somewhere. We should either reuse the original, or change the other case to use this one. Maybe as a separate PR though.

Not sure if you saw but I just moved it from where it was elsewhere. Previously it was unexpected in resource_hierarchy.go. Unless you're saying it's already in two different places in the code? One in resource_hierarchy.go and one someplace else?

v2/internal/resolver/resolver.go

theunrepentantgeek · 2023-09-03T23:47:52Z

v2/internal/resolver/resolver.go

-// to have an owner (for example, ResourceGroup), returns nil.
-func (r *Resolver) ResolveOwner(ctx context.Context, obj genruntime.ARMOwnedMetaObject) (genruntime.ARMMetaObject, error) {
+// to have an owner (for example, ResourceGroup) or the owner points to a raw ARM ID, returns nil.
+func (r *Resolver) ResolveOwner(ctx context.Context, obj genruntime.ARMOwnedMetaObject) (OwnerDetails, error) {


Seems odd that ResolveOwner returns nil even if it has an owner, just because it's an ARM ID.

Maybe rename to ResolveClusterOwner() or ResolveKubernetesOwner()?

Ah it doesn't actually return nil anymore. I refactored the method and added the OwnerDetails return type because I also didn't like this. Now it always has a return and that return has the Result set to either:

OwnerFoundKubernetes (if the owner was found in k8s)

OwnerFoundARM (if the owner was found in ARM)

OwnerNotExpected (if the owner was not found but also not expected)

an error (if an owner was not found but was expected).

I've fixed this comment.

v2/internal/resolver/resource_hierarchy.go

matthchr · 2023-09-04T06:13:46Z

/ok-to-test sha=0363c94

This closes Azure#2357. Nearly all resources now support being owned by an arbitrary ARM ID. Only database user resources (MySQLUser, PostgreSQLUser) do not allow ARM ID ownership. This is because those resources must extract certain connection information from their parents and so must have access to their parents via the Kubernetes API. ARM ID ownership behaves the following way: * Resources owned by an ARM ID will continue to attempt to reconcile if that ID doesn't exist. When the owning resource cannot be found, the resource will report a Ready=false with a "Warning" and additional details. If possible, avoid deleting the owning ARM resource without also deleting the resources in Kubernetes. * The credential being used for the resource must have the same subscription ID as the owning ARM resource. This means if your global credential is for Subscription A you cannot have ARM ID owners from subscription B unless you also create a serviceoperator.azure.com/credential-from for subscription B.

matthchr · 2023-09-04T17:01:51Z

/ok-to-test sha=255bc20

matthchr added this to the v2.3.0 milestone Sep 1, 2023

matthchr self-assigned this Sep 1, 2023

matthchr requested review from davefellows, theunrepentantgeek, babbageclunk and super-harsh as code owners September 1, 2023 21:40

matthchr commented Sep 1, 2023

View reviewed changes

v2/internal/resolver/resource_hierarchy.go Outdated Show resolved Hide resolved

matthchr force-pushed the feature/arbitrary-armid-owner branch 2 times, most recently from ab49bd8 to 3ccb5ed Compare September 2, 2023 02:26

theunrepentantgeek approved these changes Sep 3, 2023

View reviewed changes

matthchr force-pushed the feature/arbitrary-armid-owner branch from 3ccb5ed to 0363c94 Compare September 4, 2023 06:13

matthchr force-pushed the feature/arbitrary-armid-owner branch from 0363c94 to 255bc20 Compare September 4, 2023 17:01

matthchr merged commit d1c7f88 into Azure:main Sep 4, 2023
8 checks passed

matthchr deleted the feature/arbitrary-armid-owner branch September 4, 2023 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support arbitrary ARM ID owners #3245

Support arbitrary ARM ID owners #3245

matthchr commented Sep 1, 2023 •

edited

codecov-commenter commented Sep 2, 2023

matthchr commented Sep 2, 2023

theunrepentantgeek left a comment

theunrepentantgeek Sep 3, 2023

matthchr Sep 4, 2023

theunrepentantgeek Sep 3, 2023

matthchr Sep 4, 2023

theunrepentantgeek Sep 3, 2023

matthchr Sep 4, 2023

theunrepentantgeek Sep 3, 2023

matthchr Sep 4, 2023

matthchr Sep 4, 2023

matthchr commented Sep 4, 2023

matthchr commented Sep 4, 2023

Support arbitrary ARM ID owners #3245

Support arbitrary ARM ID owners #3245

Conversation

matthchr commented Sep 1, 2023 • edited

codecov-commenter commented Sep 2, 2023

Codecov Report

matthchr commented Sep 2, 2023

theunrepentantgeek 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

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 commented Sep 4, 2023

matthchr commented Sep 4, 2023

matthchr commented Sep 1, 2023 •

edited