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

[release-ocm-2.8] OCPBUGS-15813: Fix singular Ingress and API cluster VIPs removal #5337

Merged
merged 1 commit into from Jul 6, 2023
Merged

[release-ocm-2.8] OCPBUGS-15813: Fix singular Ingress and API cluster VIPs removal #5337

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 19 additions & 13 deletions internal/bminventory/inventory.go
View file
Open in desktop
Expand Up @@ -2339,12 +2339,19 @@ func (b *bareMetalInventory) updateClusterData(_ context.Context, cluster *commo
return nil
}

func wereClusterVipsUpdated(clusterVips []string, paramsVips []string) bool {
if len(clusterVips) != len(paramsVips) {
func wereClusterVipsUpdated(clusterVip string, paramVip *string, clusterVips []string, paramVips []string) bool {
if paramVip != nil && clusterVip != swag.StringValue(paramVip) {
return true
}

if paramVips == nil {
return false
}
if len(clusterVips) != len(paramVips) {
return true
}
for i := range clusterVips {
if clusterVips[i] != paramsVips[i] {
if clusterVips[i] != paramVips[i] {
return true
}
}
Expand All @@ -2362,17 +2369,16 @@ func (b *bareMetalInventory) updateVips(db *gorm.DB, params installer.V2UpdateCl
},
}

if params.ClusterUpdateParams.APIVips != nil && len(params.ClusterUpdateParams.APIVips) > 0 {
if wereClusterVipsUpdated(network.GetApiVips(cluster), network.GetApiVips(&paramVips)) {
apiVipUpdated = true
cluster.APIVips = params.ClusterUpdateParams.APIVips
}
if wereClusterVipsUpdated(cluster.APIVip, params.ClusterUpdateParams.APIVip, network.GetApiVips(cluster), network.GetApiVips(&paramVips)) {
apiVipUpdated = true
cluster.APIVip = swag.StringValue(params.ClusterUpdateParams.APIVip)
cluster.APIVips = params.ClusterUpdateParams.APIVips
}
if params.ClusterUpdateParams.IngressVips != nil && len(params.ClusterUpdateParams.IngressVips) > 0 {
if wereClusterVipsUpdated(network.GetIngressVips(cluster), network.GetIngressVips(&paramVips)) {
ingressVipUpdated = true
cluster.IngressVips = params.ClusterUpdateParams.IngressVips
}

if wereClusterVipsUpdated(cluster.IngressVip, params.ClusterUpdateParams.IngressVip, network.GetIngressVips(cluster), network.GetIngressVips(&paramVips)) {
ingressVipUpdated = true
cluster.IngressVip = swag.StringValue(params.ClusterUpdateParams.IngressVip)
cluster.IngressVips = params.ClusterUpdateParams.IngressVips
}

if apiVipUpdated || ingressVipUpdated {
Expand Down
82 changes: 82 additions & 0 deletions internal/bminventory/inventory_test.go
View file
Open in desktop
Expand Up @@ -3377,6 +3377,88 @@ var _ = Describe("cluster", func() {

})

It("Test VIP cleanup via singular APIVip and IngressVip", func() {
mockDetectAndStoreCollidingIPsForCluster(mockClusterApi, 2)
mockClusterApi.EXPECT().VerifyClusterUpdatability(createClusterIdMatcher(cluster)).Return(nil).Times(2)
mockClusterApi.EXPECT().SetConnectivityMajorityGroupsForCluster(gomock.Any(), gomock.Any()).Return(nil).Times(2)
mockClusterApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).Times(2)
mockHostApi.EXPECT().RefreshInventory(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(6)
mockHostApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(6)
mockHostApi.EXPECT().GetStagesByRole(gomock.Any(), gomock.Any()).Return(nil).Times(6)

apiVip := "1.2.3.100"
ingressVip := "1.2.3.101"
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
APIVip: swag.String(apiVip),
IngressVip: swag.String(ingressVip),
},
})
Expect(reply).Should(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
cluster = &common.Cluster{Cluster: *reply.(*installer.V2UpdateClusterCreated).Payload}
Expect(cluster.APIVip).To(Equal(apiVip))
Expect(network.GetApiVipById(cluster, 0)).To(Equal(apiVip))
Expect(cluster.IngressVip).To(Equal(ingressVip))
Expect(network.GetIngressVipById(cluster, 0)).To(Equal(ingressVip))

reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
APIVip: swag.String(""),
IngressVip: swag.String(""),
},
})
Expect(reply).Should(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
cluster = &common.Cluster{Cluster: *reply.(*installer.V2UpdateClusterCreated).Payload}
Expect(cluster.APIVip).To(Equal(""))
Expect(len(cluster.APIVips)).To(BeZero())
Expect(cluster.IngressVip).To(Equal(""))
Expect(len(cluster.IngressVips)).To(BeZero())
})

It("Test VIP cleanup via both singular and plural VIP values", func() {
mockDetectAndStoreCollidingIPsForCluster(mockClusterApi, 2)
mockClusterApi.EXPECT().VerifyClusterUpdatability(createClusterIdMatcher(cluster)).Return(nil).Times(2)
mockClusterApi.EXPECT().SetConnectivityMajorityGroupsForCluster(gomock.Any(), gomock.Any()).Return(nil).Times(2)
mockClusterApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).Times(2)
mockHostApi.EXPECT().RefreshInventory(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(6)
mockHostApi.EXPECT().RefreshStatus(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(6)
mockHostApi.EXPECT().GetStagesByRole(gomock.Any(), gomock.Any()).Return(nil).Times(6)

apiVip := "1.2.3.100"
ingressVip := "1.2.3.101"
reply := bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
APIVip: swag.String(apiVip),
IngressVip: swag.String(ingressVip),
},
})
Expect(reply).Should(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
cluster = &common.Cluster{Cluster: *reply.(*installer.V2UpdateClusterCreated).Payload}
Expect(cluster.APIVip).To(Equal(apiVip))
Expect(network.GetApiVipById(cluster, 0)).To(Equal(apiVip))
Expect(cluster.IngressVip).To(Equal(ingressVip))
Expect(network.GetIngressVipById(cluster, 0)).To(Equal(ingressVip))

reply = bm.V2UpdateCluster(ctx, installer.V2UpdateClusterParams{
ClusterID: clusterID,
ClusterUpdateParams: &models.V2ClusterUpdateParams{
APIVip: swag.String(""),
IngressVip: swag.String(""),
APIVips: nil,
IngressVips: nil,
},
})
Expect(reply).Should(BeAssignableToTypeOf(installer.NewV2UpdateClusterCreated()))
cluster = &common.Cluster{Cluster: *reply.(*installer.V2UpdateClusterCreated).Payload}
Expect(cluster.APIVip).To(Equal(""))
Expect(len(cluster.APIVips)).To(BeZero())
Expect(cluster.IngressVip).To(Equal(""))
Expect(len(cluster.IngressVips)).To(BeZero())
})

It("Two APIVips and Two ingressVips - both IPv4 - negative", func() {
apiVip := "1.2.3.100"
ingressVip := "1.2.3.101"
Expand Down
2 changes: 0 additions & 2 deletions internal/cluster/validations/validations.go
View file
Open in desktop
Expand Up @@ -380,7 +380,6 @@ func handleIngressVipUpdateBackwardsCompatibility(cluster *common.Cluster, param
if cluster.IngressVip != "" {
// IngressVip was cleared and IngressVips were not provided, clear both fields.
if swag.StringValue(params.IngressVip) == "" && len(params.IngressVips) == 0 {
params.IngressVip = nil
params.IngressVips = nil
}
// IngressVip was changed (but not cleared), IngressVips will be forcefully set to the value of IngressVips as a one-element list.
Expand All @@ -398,7 +397,6 @@ func handleApiVipUpdateBackwardsCompatibility(cluster *common.Cluster, params *m
if cluster.APIVip != "" {
// APIVip was cleared and APIVips were not provided, clear both fields.
if swag.StringValue(params.APIVip) == "" && len(params.APIVips) == 0 {
params.APIVip = nil
params.APIVips = nil
}
// APIVip was changed (but not cleared), APIVips will be forcefully set to the value of APIVip as a one-element list.
Expand Down
17 changes: 17 additions & 0 deletions internal/network/utils.go
View file
Open in desktop
Expand Up @@ -199,11 +199,17 @@ func GetClusterNetworkCidrs(cluster *common.Cluster) (ret []string) {
}

func GetApiVips(cluster *common.Cluster) (ret []string) {
if cluster.APIVips == nil {
return nil
}
ret = funk.Map(cluster.APIVips, func(x *models.APIVip) string { return string(x.IP) }).([]string)
return
}

func GetIngressVips(cluster *common.Cluster) (ret []string) {
if cluster.IngressVips == nil {
return nil
}
ret = funk.Map(cluster.IngressVips, func(x *models.IngressVip) string { return string(x.IP) }).([]string)
return
}
Expand Down Expand Up @@ -374,6 +380,12 @@ func UpdateVipsTables(db *gorm.DB, cluster *common.Cluster, apiVipUpdated bool,

if apiVipUpdated {
if err := db.Transaction(func(tx *gorm.DB) error {
if err := db.Model(&models.Cluster{}).Where("id = ?", *cluster.ID).
Update("api_vip", cluster.APIVip).Error; err != nil {
err = errors.Wrapf(err, "failed to delete api_vip of cluster %s", *cluster.ID)
return common.NewApiError(http.StatusInternalServerError, err)
}

if err := db.Where("cluster_id = ?", *cluster.ID).Delete(&models.APIVip{}).Error; err != nil {
err = errors.Wrapf(err, "failed to delete api vips of cluster %s", *cluster.ID)
return common.NewApiError(http.StatusInternalServerError, err)
Expand All @@ -393,6 +405,11 @@ func UpdateVipsTables(db *gorm.DB, cluster *common.Cluster, apiVipUpdated bool,

if ingressVipUpdated {
if err := db.Transaction(func(tx *gorm.DB) error {
if err := db.Model(&models.Cluster{}).Where("id = ?", *cluster.ID).
Update("ingress_vip", cluster.IngressVip).Error; err != nil {
err = errors.Wrapf(err, "failed to delete ingress_vip of cluster %s", *cluster.ID)
return common.NewApiError(http.StatusInternalServerError, err)
}
if err := db.Where("cluster_id = ?", *cluster.ID).Delete(&models.IngressVip{}).Error; err != nil {
err = errors.Wrapf(err, "failed to delete ingress vips of cluster %s", *cluster.ID)
return common.NewApiError(http.StatusInternalServerError, err)
Expand Down
13 changes: 13 additions & 0 deletions subsystem/operators_test.go
View file
Open in desktop
Expand Up @@ -199,6 +199,13 @@ var _ = Describe("Operators endpoint tests", func() {
registerNewCluster := func(openshiftVersion string, highAvailabilityMode string, operators []*models.OperatorCreateParams) *installer.V2RegisterClusterCreated {
var err error
var cluster *installer.V2RegisterClusterCreated
clusterCIDR := "10.128.0.0/14"
serviceCIDR := "172.30.0.0/16"

vipDhcpAllocation := swag.Bool(true)
if highAvailabilityMode == models.ClusterHighAvailabilityModeNone {
vipDhcpAllocation = swag.Bool(false)
}

cluster, err = userBMClient.Installer.V2RegisterCluster(ctx, &installer.V2RegisterClusterParams{
NewClusterParams: &models.ClusterCreateParams{
Expand All @@ -207,6 +214,12 @@ var _ = Describe("Operators endpoint tests", func() {
HighAvailabilityMode: swag.String(highAvailabilityMode),
PullSecret: swag.String(pullSecret),
OlmOperators: operators,
BaseDNSDomain: "example.com",
ClusterNetworks: []*models.ClusterNetwork{{Cidr: models.Subnet(clusterCIDR), HostPrefix: 23}},
ServiceNetworks: []*models.ServiceNetwork{{Cidr: models.Subnet(serviceCIDR)}},
SSHPublicKey: sshPublicKey,
VipDhcpAllocation: vipDhcpAllocation,
NetworkType: swag.String(models.ClusterNetworkTypeOVNKubernetes),
},
})

Expand Down