From 32621ea9103336060a64b74c2e1a685b0dfd5008 Mon Sep 17 00:00:00 2001 From: Nir Magnezi Date: Tue, 17 Oct 2023 11:03:51 +0300 Subject: [PATCH] [release-ocm-2.8] MGMT-15306: Fix UpdateCluster for requests that include VIPS and UMA (#5554) This is a combination of 2 commits. This is the 1st commit message: MGMT-14416: VipDhcpAllocation from update params should take precedence (#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 (#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. --- internal/bminventory/inventory_test.go | 222 +++++++++++++++++++- internal/cluster/validations/validations.go | 93 +++++--- 2 files changed, 277 insertions(+), 38 deletions(-) diff --git a/internal/bminventory/inventory_test.go b/internal/bminventory/inventory_test.go index 0f3bcc9abc5..fc82fb08595 100644 --- a/internal/bminventory/inventory_test.go +++ b/internal/bminventory/inventory_test.go @@ -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) @@ -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) @@ -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() { @@ -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() { diff --git a/internal/cluster/validations/validations.go b/internal/cluster/validations/validations.go index 1d27edc539d..c36fe9a6146 100644 --- a/internal/cluster/validations/validations.go +++ b/internal/cluster/validations/validations.go @@ -379,8 +379,8 @@ func HandleApiVipBackwardsCompatibility(clusterId strfmt.UUID, apiVip string, ap func handleIngressVipUpdateBackwardsCompatibility(cluster *common.Cluster, params *models.V2ClusterUpdateParams) error { if cluster.IngressVip != "" { // IngressVip was cleared and IngressVips were not provided, clear both fields. - if swag.StringValue(params.IngressVip) == "" && len(params.IngressVips) == 0 { - params.IngressVips = nil + if params.IngressVip != nil && swag.StringValue(params.IngressVip) == "" && params.IngressVips == nil { + params.IngressVips = []*models.IngressVip{} } // IngressVip was changed (but not cleared), IngressVips will be forcefully set to the value of IngressVips as a one-element list. if params.IngressVip != nil && swag.StringValue(params.IngressVip) != "" && swag.StringValue(params.IngressVip) != cluster.IngressVip { @@ -396,8 +396,8 @@ func handleIngressVipUpdateBackwardsCompatibility(cluster *common.Cluster, param func handleApiVipUpdateBackwardsCompatibility(cluster *common.Cluster, params *models.V2ClusterUpdateParams) error { if cluster.APIVip != "" { // APIVip was cleared and APIVips were not provided, clear both fields. - if swag.StringValue(params.APIVip) == "" && len(params.APIVips) == 0 { - params.APIVips = nil + if params.APIVip != nil && swag.StringValue(params.APIVip) == "" && params.APIVips == nil { + params.APIVips = []*models.APIVip{} } // APIVip was changed (but not cleared), APIVips will be forcefully set to the value of APIVip as a one-element list. if params.APIVip != nil && swag.StringValue(params.APIVip) != "" && swag.StringValue(params.APIVip) != cluster.APIVip { @@ -470,6 +470,43 @@ func ValidateClusterCreateIPAddresses(ipV6Supported bool, clusterId strfmt.UUID, return validateVIPAddresses(ipV6Supported, targetConfiguration) } +func validateVIPsWithUMA(cluster *common.Cluster, params *models.V2ClusterUpdateParams, vipDhcpAllocation bool) error { + var ( + apiVip string + ingressVip string + apiVips []*models.APIVip + ingressVips []*models.IngressVip + ) + + if swag.BoolValue(cluster.VipDhcpAllocation) { + return ValidateVIPsWereNotSetUserManagedNetworking( + apiVip, ingressVip, apiVips, ingressVips, vipDhcpAllocation, + ) + } + + apiVip = cluster.APIVip + ingressVip = cluster.IngressVip + apiVips = cluster.APIVips + ingressVips = cluster.IngressVips + + if params.APIVip != nil { + apiVip = swag.StringValue(params.APIVip) + } + if params.IngressVip != nil { + ingressVip = swag.StringValue(params.IngressVip) + } + if params.APIVips != nil { + apiVips = params.APIVips + } + if params.IngressVips != nil { + ingressVips = params.IngressVips + } + + return ValidateVIPsWereNotSetUserManagedNetworking( + apiVip, ingressVip, apiVips, ingressVips, vipDhcpAllocation, + ) +} + func ValidateClusterUpdateVIPAddresses(ipV6Supported bool, cluster *common.Cluster, params *models.V2ClusterUpdateParams) error { var err error targetConfiguration := common.Cluster{} @@ -501,38 +538,34 @@ func ValidateClusterUpdateVIPAddresses(ipV6Supported bool, cluster *common.Clust return common.NewApiError(http.StatusBadRequest, err) } - targetConfiguration.UserManagedNetworking = swag.Bool(false) - if params.UserManagedNetworking != nil { - if swag.BoolValue(params.UserManagedNetworking) { - err = ValidateVIPsWereNotSetUserManagedNetworking(cluster.APIVip, cluster.IngressVip, - cluster.APIVips, cluster.IngressVips, swag.BoolValue(cluster.VipDhcpAllocation)) - if err != nil { - // reformat error to match order of actions - errParts := strings.Split(err.Error(), " cannot be set with ") - err = errors.Errorf("%s cannot be set with %s", errParts[1], errParts[0]) - return common.NewApiError(http.StatusBadRequest, err) - } + if params.UserManagedNetworking != nil && swag.BoolValue(params.UserManagedNetworking) { + vipDhcpAllocation := swag.BoolValue(cluster.VipDhcpAllocation) + if params.VipDhcpAllocation != nil { // VipDhcpAllocation from update params should take precedence + vipDhcpAllocation = swag.BoolValue(params.VipDhcpAllocation) } - targetConfiguration.UserManagedNetworking = params.UserManagedNetworking - } - targetConfiguration.VipDhcpAllocation = swag.Bool(false) - if params.VipDhcpAllocation != nil { - targetConfiguration.VipDhcpAllocation = params.VipDhcpAllocation - } - targetConfiguration.ID = cluster.ID - if params.APIVip != nil { - targetConfiguration.APIVip = *params.APIVip - } + if err = validateVIPsWithUMA(cluster, params, vipDhcpAllocation); err != nil { + // reformat error to match order of actions + errParts := strings.Split(err.Error(), " cannot be set with ") + err = errors.Errorf("%s cannot be set with %s", errParts[1], errParts[0]) + return common.NewApiError(http.StatusBadRequest, err) + } - if params.IngressVip != nil { - targetConfiguration.IngressVip = *params.IngressVip + if cluster.VipDhcpAllocation != nil && swag.BoolValue(cluster.VipDhcpAllocation) { // override VIPs that were allocated via DHCP + params.APIVip = swag.String("") + params.IngressVip = swag.String("") + params.APIVips = []*models.APIVip{} + params.IngressVips = []*models.IngressVip{} + } } + targetConfiguration.ID = cluster.ID + targetConfiguration.VipDhcpAllocation = params.VipDhcpAllocation + targetConfiguration.APIVip = swag.StringValue(params.APIVip) targetConfiguration.APIVips = params.APIVips + targetConfiguration.IngressVip = swag.StringValue(params.IngressVip) targetConfiguration.IngressVips = params.IngressVips targetConfiguration.UserManagedNetworking = params.UserManagedNetworking - targetConfiguration.VipDhcpAllocation = params.VipDhcpAllocation targetConfiguration.HighAvailabilityMode = cluster.HighAvailabilityMode targetConfiguration.ClusterNetworks = params.ClusterNetworks targetConfiguration.ServiceNetworks = params.ServiceNetworks @@ -787,7 +820,7 @@ func ValidateVIPsWereNotSetDhcpMode(apiVip string, ingressVip string, apiVips [] err := errors.New("Setting API VIP is forbidden when cluster is in vip-dhcp-allocation mode") return err } - if apiVips != nil { + if len(apiVips) > 0 { err := errors.New("Setting API VIPs is forbidden when cluster is in vip-dhcp-allocation mode") return err } @@ -795,7 +828,7 @@ func ValidateVIPsWereNotSetDhcpMode(apiVip string, ingressVip string, apiVips [] err := errors.New("Setting Ingress VIP is forbidden when cluster is in vip-dhcp-allocation mode") return err } - if ingressVips != nil { + if len(ingressVips) > 0 { err := errors.New("Setting Ingress VIPs is forbidden when cluster is in vip-dhcp-allocation mode") return err }