Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[v2.9] Fix logic for Authorized IP ranges #594

Merged
merged 1 commit into from
Jul 25, 2024
Merged

[v2.9] Fix logic for Authorized IP ranges #594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions controller/aks-cluster-config-handler.go
View file
Open in desktop
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
Open in desktop
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: is there a case where authorized IP ranges are set on the spec and if so what happens in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For imported clusters it is not possible, always this setting will be overwritten with downstream cluster

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
Open in desktop
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