Skip to content

Commit

Permalink
Fix bug where owner.armId could be changed (#4193)
Browse files Browse the repository at this point in the history
It was never intended to allow changes to owner.armId, this protection
was missed when adding owner.armId support.

Even though this is technically a breaking change in the strictest
sense, it's unlikely to break users as updating ARM ID, while not
explicitly rejected, didn't actually work upstream in Azure for most
cases.

Fixes #4181
  • Loading branch information
matthchr authored Aug 19, 2024
1 parent 1c9a80c commit 2afdf16
Show file tree
Hide file tree
Showing 3 changed files with 729 additions and 6 deletions.
39 changes: 37 additions & 2 deletions v2/internal/controllers/edge_case_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,42 @@ func Test_Owner_IsImmutableOnceSuccessfullyCreated(t *testing.T) {
acct.Spec.Owner = testcommon.AsOwner(rg2)
err := tc.PatchAndExpectError(old, acct)

tc.Expect(err).ToNot(BeNil())
tc.Expect(err).To(MatchError(ContainSubstring("updating 'spec.owner.name' is not allowed")))
tc.Expect(old.Owner().Name).ToNot(BeIdenticalTo(rg2.Name))

// Delete the account
tc.DeleteResourceAndWait(acct)
}

func Test_OwnerARMID_IsImmutableOnceSuccessfullyCreated(t *testing.T) {
t.Parallel()

tc := globalTestContext.ForTest(t)

rg := tc.CreateTestResourceGroupAndWait()

// Ensure that the RG has an ARM ID set
tc.Expect(rg.Status.Id).ToNot(BeNil())
tc.Expect(to.Value(rg.Status.Id)).ToNot(BeEmpty())

acct := newStorageAccount(tc, rg)
// Manually set the ARM ID of the owner:
acct.Spec.Owner.Name = ""
acct.Spec.Owner.ARMID = to.Value(rg.Status.Id)
tc.CreateResourcesAndWait(acct)

rg2 := tc.CreateTestResourceGroupAndWait()

// Ensure that the RG has an ARM ID set
tc.Expect(rg2.Status.Id).ToNot(BeNil())
tc.Expect(to.Value(rg2.Status.Id)).ToNot(BeEmpty())

// Patch the account to change Owner
old := acct.DeepCopy()
acct.Spec.Owner.ARMID = to.Value(rg2.Status.Id)
err := tc.PatchAndExpectError(old, acct)

tc.Expect(err).To(MatchError(ContainSubstring("updating 'spec.owner.armId' is not allowed")))
tc.Expect(old.Owner().Name).ToNot(BeIdenticalTo(rg2.Name))

// Delete the account
Expand All @@ -242,7 +277,7 @@ func Test_AzureName_IsImmutable_IfAzureHasBeenCommunicatedWith(t *testing.T) {
acct.Spec.AzureName = tc.NoSpaceNamer.GenerateName("stor")
err := tc.PatchAndExpectError(old, acct)
tc.Expect(err).To(HaveOccurred())
tc.Expect(err.Error()).To(ContainSubstring("updating 'AzureName' is not allowed"))
tc.Expect(err.Error()).To(ContainSubstring("updating 'spec.azureName' is not allowed"))

// Delete the account
tc.DeleteResourceAndWait(acct)
Expand Down
Loading

0 comments on commit 2afdf16

Please sign in to comment.