Skip to content

Commit

Permalink
Fix logic for Authorized IP ranges
Browse files Browse the repository at this point in the history 
(cherry picked from commit 08974fc3330e57b06dba4fe574f1c9e3b1782341)
  • Loading branch information
@mjura
mjura committed Jul 19, 2024
1 parent 3bbd47e commit 7aea327
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 19 deletions.
1 change: 1 addition & 0 deletions controller/aks-cluster-config-handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ func (h *Handler) buildUpstreamClusterState(ctx context.Context, credentials *ak

// set API server access profile
upstreamSpec.PrivateCluster = to.Ptr(false)
upstreamSpec.AuthorizedIPRanges = utils.ConvertToPointerOfSlice([]*string{})
if clusterState.Properties.APIServerAccessProfile != nil {
if clusterState.Properties.APIServerAccessProfile.EnablePrivateCluster != nil {
upstreamSpec.PrivateCluster = clusterState.Properties.APIServerAccessProfile.EnablePrivateCluster
Expand Down
23 changes: 16 additions & 7 deletions pkg/aks/update.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func UpdateCluster(ctx context.Context, cred *Credentials, clusterClient service
ctx,
spec.ResourceGroup,
spec.ClusterName,
updateCluster(*desiredCluster, actualCluster.ManagedCluster),
updateCluster(*desiredCluster, actualCluster.ManagedCluster, spec.Imported),
nil,
)
if err != nil {
Expand All @@ -64,7 +64,7 @@ func UpdateCluster(ctx context.Context, cred *Credentials, clusterClient service
return err
}

func updateCluster(desiredCluster armcontainerservice.ManagedCluster, actualCluster armcontainerservice.ManagedCluster) armcontainerservice.ManagedCluster {
func updateCluster(desiredCluster armcontainerservice.ManagedCluster, actualCluster armcontainerservice.ManagedCluster, imported bool) armcontainerservice.ManagedCluster {
if !validateUpdate(desiredCluster, actualCluster) {
logrus.Warn("Not all cluster properties can be updated.")
}
Expand Down Expand Up @@ -108,14 +108,23 @@ func updateCluster(desiredCluster armcontainerservice.ManagedCluster, actualClus
}
}

// If the cluster is imported, we may not have the authorized IP ranges set on the spec.
if desiredCluster.Properties.APIServerAccessProfile != nil && desiredCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges != nil {
for i := range desiredCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges {
ipRange := (desiredCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges)[i]

if !hasAuthorizedIPRange(String(ipRange), actualCluster.Properties.APIServerAccessProfile) {
actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges = append(actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges, ipRange)
if !imported {
actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges = desiredCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges
} else {
for i := range desiredCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges {
ipRange := (desiredCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges)[i]

if !hasAuthorizedIPRange(String(ipRange), actualCluster.Properties.APIServerAccessProfile) {
actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges = append(actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges, ipRange)
}
}
}
} else {
if !imported && actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges != nil {
actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges = []*string{}
}
}

// Linux profile
Expand Down
94 changes: 82 additions & 12 deletions pkg/aks/update_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ var _ = Describe("updateCluster", func() {
desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster)
updatedCluster := updateCluster(*desiredCluster, *actualCluster, false)
Expect(updatedCluster.Properties.KubernetesVersion).To(Equal(clusterSpec.KubernetesVersion))
Expect(updatedCluster.Properties.AddonProfiles).To(HaveKey("test-addon"))
Expect(updatedCluster.Properties.AddonProfiles).To(HaveKey("httpApplicationRouting"))
Expand Down Expand Up @@ -113,7 +113,7 @@ var _ = Describe("updateCluster", func() {
desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster)
updatedCluster := updateCluster(*desiredCluster, *actualCluster, false)
Expect(updatedCluster.Properties.KubernetesVersion).To(BeNil())
})

Expand All @@ -126,36 +126,106 @@ var _ = Describe("updateCluster", func() {
desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster)
updatedCluster := updateCluster(*desiredCluster, *actualCluster, false)
agentPoolProfiles := updatedCluster.Properties.AgentPoolProfiles
Expect(agentPoolProfiles).To(HaveLen(1))
})

It("shouldn't set authorized IP ranges if not specified in cluster spec", func() {
It("should clear authorized IP ranges for non imported clusters", func() {
actualCluster.Properties.APIServerAccessProfile = &armcontainerservice.ManagedClusterAPIServerAccessProfile{
AuthorizedIPRanges: []*string{to.Ptr("test-ip-range"), to.Ptr("test-ip-range-2")},
}

clusterSpec.AuthorizedIPRanges = nil
desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster)
updatedCluster := updateCluster(*desiredCluster, *actualCluster, false)
Expect(updatedCluster.Properties.APIServerAccessProfile).ToNot(BeNil())
Expect(updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges).ToNot(BeNil())
authorizedIPranges := updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges
Expect(authorizedIPranges).To(HaveLen(0))

clusterSpec.AuthorizedIPRanges = to.Ptr([]string{})
desiredCluster, err = createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster = updateCluster(*desiredCluster, *actualCluster, false)
Expect(updatedCluster.Properties.APIServerAccessProfile).ToNot(BeNil())
Expect(updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges).ToNot(BeNil())
authorizedIPranges = updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges
Expect(authorizedIPranges).To(HaveLen(0))
})

It("shoudn't add new authorized IP range if it already exists ", func() {
It("should overwrite authorized IP range for non imported clusters", func() {
clusterSpec.AuthorizedIPRanges = to.Ptr([]string{"test-ip-range-new1", "test-ip-range-new2"})

actualCluster.Properties.APIServerAccessProfile = &armcontainerservice.ManagedClusterAPIServerAccessProfile{
AuthorizedIPRanges: []*string{to.Ptr("test-ip-range")},
AuthorizedIPRanges: []*string{to.Ptr("test-ip-range"), to.Ptr("test-ip-range-2")},
}

desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster)
updatedCluster := updateCluster(*desiredCluster, *actualCluster, false)
Expect(updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges).To(Equal(actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges))
Expect(updatedCluster.Properties.APIServerAccessProfile).ToNot(BeNil())
authorizedIPranges := updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges
Expect(authorizedIPranges).To(HaveLen(1))
Expect(authorizedIPranges).To(HaveLen(2))
Expect(*authorizedIPranges[0]).To(Equal("test-ip-range-new1"))
Expect(*authorizedIPranges[1]).To(Equal("test-ip-range-new2"))

clusterSpec.AuthorizedIPRanges = to.Ptr([]string{"test-ip-range-new1", "test-ip-range-new2"})
})

It("should merge authorized IP range if it already exists for imported cluster", func() {
clusterSpec.AuthorizedIPRanges = to.Ptr([]string{"test-ip-range-new1", "test-ip-range-new2", "test-ip-range"})

actualCluster.Properties.APIServerAccessProfile = &armcontainerservice.ManagedClusterAPIServerAccessProfile{
AuthorizedIPRanges: []*string{to.Ptr("test-ip-range"), to.Ptr("test-ip-range-2")},
}

desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster, true)
Expect(updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges).To(Equal(actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges))
Expect(updatedCluster.Properties.APIServerAccessProfile).ToNot(BeNil())
authorizedIPranges := updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges
Expect(authorizedIPranges).To(HaveLen(4))
Expect(*authorizedIPranges[0]).To(Equal("test-ip-range"))
Expect(*authorizedIPranges[1]).To(Equal("test-ip-range-2"))
Expect(*authorizedIPranges[2]).To(Equal("test-ip-range-new1"))
Expect(*authorizedIPranges[3]).To(Equal("test-ip-range-new2"))
})

It("shoudn't change authorized IP range if it is empty or nil to imported cluster ", func() {
actualCluster.Properties.APIServerAccessProfile = &armcontainerservice.ManagedClusterAPIServerAccessProfile{
AuthorizedIPRanges: []*string{to.Ptr("test-ip-range"), to.Ptr("test-ip-range-2")},
}
clusterSpec.AuthorizedIPRanges = nil
desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster, true)
Expect(updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges).To(Equal(actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges))
Expect(updatedCluster.Properties.APIServerAccessProfile).ToNot(BeNil())
authorizedIPranges := updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges
Expect(authorizedIPranges).To(HaveLen(2))
Expect(*authorizedIPranges[0]).To(Equal("test-ip-range"))
Expect(*authorizedIPranges[1]).To(Equal("test-ip-range-2"))

clusterSpec.AuthorizedIPRanges = to.Ptr([]string{})
desiredCluster, err = createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster = updateCluster(*desiredCluster, *actualCluster, true)
Expect(updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges).To(Equal(actualCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges))
Expect(updatedCluster.Properties.APIServerAccessProfile).ToNot(BeNil())
authorizedIPranges = updatedCluster.Properties.APIServerAccessProfile.AuthorizedIPRanges
Expect(authorizedIPranges).To(HaveLen(2))
Expect(*authorizedIPranges[0]).To(Equal("test-ip-range"))
Expect(*authorizedIPranges[1]).To(Equal("test-ip-range-2"))
})

It("shouldn't update linux profile if it's not specified", func() {
Expand All @@ -164,15 +234,15 @@ var _ = Describe("updateCluster", func() {
desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster)
updatedCluster := updateCluster(*desiredCluster, *actualCluster, false)
Expect(updatedCluster.Properties.LinuxProfile).To(BeNil())
})

It("shouldn't update service principal if phase is active or updating", func() {
desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "active")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster)
updatedCluster := updateCluster(*desiredCluster, *actualCluster, false)
Expect(updatedCluster.Properties.ServicePrincipalProfile).To(BeNil())
})

Expand All @@ -181,7 +251,7 @@ var _ = Describe("updateCluster", func() {
desiredCluster, err := createManagedCluster(ctx, cred, workplacesClientMock, clusterSpec, "phase")
Expect(err).ToNot(HaveOccurred())

updatedCluster := updateCluster(*desiredCluster, *actualCluster)
updatedCluster := updateCluster(*desiredCluster, *actualCluster, false)
Expect(updatedCluster.Tags).To(HaveLen(0))
})
})
Expand Down

0 comments on commit 7aea327

Please sign in to comment.