Skip to content

Commit

Permalink
[release-ocm-2.8] MGMT-15306: Fix UpdateCluster for requests that inc…
Browse files Browse the repository at this point in the history 
…lude VIPS and UMA (openshift#5554)

This is a combination of 2 commits.

This is the 1st commit message:

MGMT-14416: VipDhcpAllocation from update params should take precedence (openshift#5209)

This change fixes a case where UserManagedNetworking and
VipDhcpAllocation are being changed at the same API update.

The ValidateClusterUpdateVIPAddresses func will now use VipDhcpAllocation
from update params, if relevant, before taking the DB cluster
VipDhcpAllocation into account.

Additionally, an additional error surfaced while fixing the aforementioned
issue, displaying a "User Managed Networking cannot be set with API VIP" to the user.

This change will also handle such a scenario by clearing up API and Ingress VIPs
that were set by DHCP, in case the user switched to 'User Managed Networking'.

This is the commit message #2:

MGMT-15306: Fix UpdateCluster for requests that include VIPS and UMA (openshift#5547)

This fix handles cluster updates that patch UMN and VIPs at the same request.
There was an issue in which the code overrode an empty VIPs array (which is
how VIPs should get deleted) with a nil, indicating no update to this field.

This change also includes tests that validate those exact scenarios.
  • Loading branch information
Nir Magnezi authored Oct 17, 2023
1 parent d318938 commit 32621ea
Show file tree
Hide file tree
Showing 2 changed files with 277 additions and 38 deletions.
222 changes: 214 additions & 8 deletions internal/bminventory/inventory_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4177,6 +4177,106 @@ var _ = Describe("cluster", func() {

Context("UserManagedNetworking", func() {

It("handle UserManagedNetworking and Singular VIPs update at the same request", func() {
mockClusterUpdatability(3)
mockDetectAndStoreCollidingIPsForCluster(mockClusterApi, 3)
mockHostApi.EXPECT().RefreshInventory(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockHostApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockHostApi.EXPECT().GetStagesByRole(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockClusterApi.EXPECT().SetConnectivityMajorityGroupsForCluster(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockClusterApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
mockProviderRegistry.EXPECT().SetPlatformUsages(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()

apiVip := "10.11.12.15"
ingressVip := "10.11.12.16"

By("Set User Managed Networking: false + Add APIVip and IngressVip")
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(false),
APIVip: swag.String(apiVip),
IngressVip: swag.String(ingressVip),
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

By("Set UserManagedNetworking true + Remove APIVip and IngressVip")
reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(true),
APIVip: swag.String(""),
IngressVip: swag.String(""),
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

By("Set User Managed Networking: false + Add APIVip and IngressVip again")
reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(false),
APIVip: swag.String(apiVip),
IngressVip: swag.String(ingressVip),
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
})

It("handle UserManagedNetworking and VIP arrays update at the same request", func() {
mockClusterUpdatability(3)
mockDetectAndStoreCollidingIPsForCluster(mockClusterApi, 3)
mockHostApi.EXPECT().RefreshInventory(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockHostApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockHostApi.EXPECT().GetStagesByRole(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockClusterApi.EXPECT().SetConnectivityMajorityGroupsForCluster(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockClusterApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
mockProviderRegistry.EXPECT().SetPlatformUsages(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()

apiVip := "10.11.12.15"
ingressVip := "10.11.12.16"

By("Set User Managed Networking: false + Add APIVip and IngressVip")
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(false),
APIVip: swag.String(apiVip),
APIVips: []*models.APIVip{{IP: models.IP(apiVip)}},
IngressVip: swag.String(ingressVip),
IngressVips: []*models.IngressVip{{IP: models.IP(ingressVip)}},
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

By("Set UserManagedNetworking true + Remove APIVip and IngressVip")
reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(true),
APIVip: swag.String(""),
APIVips: []*models.APIVip{},
IngressVip: swag.String(""),
IngressVips: []*models.IngressVip{},
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

By("Set User Managed Networking: false + Add APIVip and IngressVip again")
reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(false),
APIVip: swag.String(apiVip),
APIVips: []*models.APIVip{{IP: models.IP(apiVip)}},
IngressVip: swag.String(ingressVip),
IngressVips: []*models.IngressVip{{IP: models.IP(ingressVip)}},
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
})

It("Fail to set cluster VIPs when UserManagedNetworking true was set", func() {
mockClusterUpdatability(3)
mockDetectAndStoreCollidingIPsForCluster(mockClusterApi, 2)
Expand Down Expand Up @@ -4254,6 +4354,113 @@ var _ = Describe("cluster", func() {
verifyApiErrorString(reply, http.StatusBadRequest, "User Managed Networking cannot be set with API VIP")
})

It("Update both UserManagedNetworking and VipDhcpAllocation at the same update", func() {
mockClusterUpdatability(4)
mockDetectAndStoreCollidingIPsForCluster(mockClusterApi, 4)
mockHostApi.EXPECT().RefreshInventory(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockHostApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockHostApi.EXPECT().GetStagesByRole(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockClusterApi.EXPECT().SetConnectivityMajorityGroupsForCluster(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockClusterApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
mockProviderRegistry.EXPECT().SetPlatformUsages(models.PlatformTypeNone, gomock.Any(), mockUsage)

By("Set User Managed Networking: false")
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(false),
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

By("Set API VIP and Ingress VIP")
reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
APIVip: swag.String("10.11.12.15"),
IngressVip: swag.String("10.11.12.16"),
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

By("set UserManagedNetworking: false, and VipDhcpAllocation: true")
reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(false),
VipDhcpAllocation: swag.Bool(true),
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

By("set UserManagedNetworking: true, and VipDhcpAllocation: false")
reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(true),
VipDhcpAllocation: swag.Bool(false),
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
})

It("Test that DHCP VIPs were cleared when switching to UserManagedNetworking: true", func() {
mockClusterUpdatability(2)
mockDetectAndStoreCollidingIPsForCluster(mockClusterApi, 2)
mockHostApi.EXPECT().RefreshInventory(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockHostApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockHostApi.EXPECT().GetStagesByRole(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockClusterApi.EXPECT().SetConnectivityMajorityGroupsForCluster(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockClusterApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
mockProviderRegistry.EXPECT().SetPlatformUsages(models.PlatformTypeNone, gomock.Any(), mockUsage)

apiVip := "10.11.12.15"
ingressVip := "10.11.12.16"

By("Set User Managed Networking: false and VipDhcpAllocation: true")
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(false),
VipDhcpAllocation: swag.Bool(true),
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

By("Update VIPS in DB cluster to mimic DHCP allocation")
cluster, err := common.GetClusterFromDB(db, clusterID, common.SkipEagerLoading)
Expect(err).ToNot(HaveOccurred())
cluster.APIVip = apiVip
cluster.APIVips = []*models.APIVip{{IP: models.IP(apiVip)}}
cluster.IngressVip = ingressVip
cluster.IngressVips = []*models.IngressVip{{IP: models.IP(ingressVip)}}
db.Save(&cluster)

By("Verify VIPs updates")
replay := bm.V2GetCluster(ctx, installer.V2GetClusterParams{ClusterID: clusterID}).(*installer.V2GetClusterOK)
Expect(replay.Payload.APIVip).Should(Equal(apiVip))
Expect(len(replay.Payload.APIVips)).Should(Equal(1))
Expect(replay.Payload.IngressVip).Should(Equal(ingressVip))
Expect(len(replay.Payload.IngressVips)).Should(Equal(1))

By("Set User Managed Networking: true and VipDhcpAllocation: false")
reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(true),
VipDhcpAllocation: swag.Bool(false),
},
})
Expect(reply).To(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))

By("Verify that DHCP VIPs were cleared")
replay = bm.V2GetCluster(ctx, installer.V2GetClusterParams{ClusterID: clusterID}).(*installer.V2GetClusterOK)
Expect(replay.Payload.APIVip).Should(Equal(""))
Expect(len(replay.Payload.APIVips)).Should(BeZero())
Expect(replay.Payload.IngressVip).Should(Equal(""))
Expect(len(replay.Payload.IngressVips)).Should(BeZero())
})

It("success", func() {
mockClusterUpdatability(1)
mockSuccess(1)
Expand Down Expand Up @@ -4303,7 +4510,7 @@ var _ = Describe("cluster", func() {
},
})

verifyApiErrorString(reply, http.StatusBadRequest, "VIP DHCP Allocation cannot be set with User Managed Networking")
verifyApiErrorString(reply, http.StatusBadRequest, "User Managed Networking cannot be set with VIP DHCP Allocation")
})

It("Fail with DHCP when UserManagedNetworking was set", func() {
Expand All @@ -4329,30 +4536,29 @@ var _ = Describe("cluster", func() {
IngressVip: swag.String("10.35.20.10"),
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "API VIP cannot be set with User Managed Networking")
verifyApiErrorString(reply, http.StatusBadRequest, "User Managed Networking cannot be set with API VIP")
})

It("Fail with API and Ingress VIPs", func() {
By("Fail VIPs with User Managed Networking")
By("Fail API VIP with User Managed Networking")
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(true),
APIVip: swag.String("10.35.20.10"),
IngressVip: swag.String("10.35.30.10"),
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "API VIP cannot be set with User Managed Networking")
verifyApiErrorString(reply, http.StatusBadRequest, "User Managed Networking cannot be set with API VIP")

By("Fail API VIP with no Ingress VIP")
By("Fail Ingress VIP with no Ingress VIP")
reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
UserManagedNetworking: swag.Bool(true),
APIVip: swag.String("10.35.20.10"),
IngressVip: swag.String("10.35.20.10"),
},
})
verifyApiErrorString(reply, http.StatusBadRequest, "configuration must include the same number of apiVIPs (got 1) and ingressVIPs (got 0)")
verifyApiErrorString(reply, http.StatusBadRequest, "User Managed Networking cannot be set with Ingress VIP")
})

It("Don't fail with Machine CIDR", func() {
Expand Down
Loading

0 comments on commit 32621ea

Please sign in to comment.