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 }