Skip to content

Commit

Permalink
NO-ISSUE: When comparing VIPs, compare only IPs (#4882)
Browse files Browse the repository at this point in the history 
This PR changes logic of comparing complex VIP structs so that only the
value of IP address is compared and not the cluster ID. This is in order
to simplify the UpdateCluster logic in the ClusterDeployment controller
when we need to decide if the Spec of AgentClusterInstall has been
updated.

Previously used reflect.DeepEqual was comparing both IP and ClusterID,
whereas the newly introduced comparison function compares slices by
comparing only their respective IP fields.
  • Loading branch information
@mkowalski
mkowalski authored Jan 11, 2023
1 parent 86b487f commit 79584fa
Show file tree
Hide file tree
Showing 3 changed files with 254 additions and 2 deletions.
4 changes: 2 additions & 2 deletions internal/controller/controllers/clusterdeployments_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ func (r *ClusterDeploymentsReconciler) updateNetworkParams(clusterDeployment *hi
apiVipsEntriesToArray(clusterInstall.Spec.APIVIPs))

if clusterInstall.Spec.APIVIP != cluster.APIVip ||
!reflect.DeepEqual(desiredApiVips, cluster.APIVips) {
!network.AreApiVipsIdentical(desiredApiVips, cluster.APIVips) {

params.APIVip = swag.String(clusterInstall.Spec.APIVIP)
params.APIVips = desiredApiVips
Expand All @@ -875,7 +875,7 @@ func (r *ClusterDeploymentsReconciler) updateNetworkParams(clusterDeployment *hi
ingressVipsEntriesToArray(clusterInstall.Spec.IngressVIPs))

if clusterInstall.Spec.IngressVIP != cluster.IngressVip ||
!reflect.DeepEqual(desiredIngressVips, cluster.IngressVips) {
!network.AreIngressVipsIdentical(desiredIngressVips, cluster.IngressVips) {

params.IngressVip = swag.String(clusterInstall.Spec.IngressVIP)
params.IngressVips = desiredIngressVips
Expand Down
12 changes: 12 additions & 0 deletions internal/network/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,10 @@ func GetIPv6Networks(inventory *models.Inventory) (ret []CidrInfo, err error) {
}

func areListsEquivalent(len1, len2 int, areItemsEquivalent func(int, int) bool) bool {
// Be aware! This function does not care about order of items on the list. For the sake of
// comparison done here, [1, 2] and [2, 1] are equal. If order of entries is important, please
// use another function.

if len1 != len2 {
return false
}
Expand Down Expand Up @@ -352,6 +356,14 @@ func AreClusterNetworksIdentical(n1, n2 []*models.ClusterNetwork) bool {
return areListsEquivalent(len(n1), len(n2), func(i, j int) bool { return n1[i].Cidr == n2[j].Cidr && n1[i].HostPrefix == n2[j].HostPrefix })
}

func AreApiVipsIdentical(n1, n2 []*models.APIVip) bool {
return areListsEquivalent(len(n1), len(n2), func(i, j int) bool { return n1[i].IP == n2[j].IP })
}

func AreIngressVipsIdentical(n1, n2 []*models.IngressVip) bool {
return areListsEquivalent(len(n1), len(n2), func(i, j int) bool { return n1[i].IP == n2[j].IP })
}

func UpdateVipsTables(db *gorm.DB, cluster *common.Cluster, apiVipUpdated bool, ingressVipUpdated bool) error {
var err error

Expand Down
240 changes: 240 additions & 0 deletions internal/network/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,246 @@ var _ = Describe("ArClusterNetworksIdentical", func() {
}
})

var _ = Describe("AreApiVipsIdentical", func() {
tests := []struct {
name string
n1, n2 []*models.APIVip
expectedResult bool
}{
{
name: "Both nil",
expectedResult: true,
},
{
name: "One nil, one empty",
n1: []*models.APIVip{},
expectedResult: true,
},
{
name: "Both empty",
n1: []*models.APIVip{},
n2: []*models.APIVip{},
expectedResult: true,
},
{
name: "Identical, ignore cluster id",
n1: []*models.APIVip{
{
IP: "1.2.3.0",
ClusterID: "id",
},
{
IP: "5.6.7.0",
},
},
n2: []*models.APIVip{
{
IP: "1.2.3.0",
},
{
IP: "5.6.7.0",
},
},
expectedResult: true,
},
{
// In this comparison we don't care about the order of entries, we only care that a set
// built from all the items is equal. If a consumer cares about of order of entries,
// another comparison function should be used.
name: "Identical in different order, ignore cluster id",
n1: []*models.APIVip{
{
IP: "1.2.3.0",
ClusterID: "id",
},
{
IP: "5.6.7.0",
},
},
n2: []*models.APIVip{
{
IP: "5.6.7.0",
},
{
IP: "1.2.3.0",
},
},
expectedResult: true,
},
{
name: "Different length",
n1: []*models.APIVip{
{
IP: "1.2.3.0",
ClusterID: "id",
},
{
IP: "5.6.7.0",
},
},
n2: []*models.APIVip{
{
IP: "5.6.7.0",
},
{
IP: "1.2.3.0",
},
{
IP: "2.2.3.0",
},
},
expectedResult: false,
},
{
name: "Different contents",
n1: []*models.APIVip{
{
IP: "1.2.3.0",
ClusterID: "id",
},
{
IP: "5.6.7.0",
},
},
n2: []*models.APIVip{
{
IP: "5.6.7.0",
},
{
IP: "2.2.3.0",
},
},
expectedResult: false,
},
}
for i := range tests {
t := tests[i]
It(t.name, func() {
Expect(AreApiVipsIdentical(t.n1, t.n2)).To(Equal(t.expectedResult))
})
}
})

var _ = Describe("AreIngressVipsIdentical", func() {
tests := []struct {
name string
n1, n2 []*models.IngressVip
expectedResult bool
}{
{
name: "Both nil",
expectedResult: true,
},
{
name: "One nil, one empty",
n1: []*models.IngressVip{},
expectedResult: true,
},
{
name: "Both empty",
n1: []*models.IngressVip{},
n2: []*models.IngressVip{},
expectedResult: true,
},
{
name: "Identical, ignore cluster id",
n1: []*models.IngressVip{
{
IP: "1.2.3.0",
ClusterID: "id",
},
{
IP: "5.6.7.0",
},
},
n2: []*models.IngressVip{
{
IP: "1.2.3.0",
},
{
IP: "5.6.7.0",
},
},
expectedResult: true,
},
{
// In this comparison we don't care about the order of entries, we only care that a set
// built from all the items is equal. If a consumer cares about of order of entries,
// another comparison function should be used.
name: "Identical in different order, ignore cluster id",
n1: []*models.IngressVip{
{
IP: "1.2.3.0",
ClusterID: "id",
},
{
IP: "5.6.7.0",
},
},
n2: []*models.IngressVip{
{
IP: "5.6.7.0",
},
{
IP: "1.2.3.0",
},
},
expectedResult: true,
},
{
name: "Different length",
n1: []*models.IngressVip{
{
IP: "1.2.3.0",
ClusterID: "id",
},
{
IP: "5.6.7.0",
},
},
n2: []*models.IngressVip{
{
IP: "5.6.7.0",
},
{
IP: "1.2.3.0",
},
{
IP: "2.2.3.0",
},
},
expectedResult: false,
},
{
name: "Different contents",
n1: []*models.IngressVip{
{
IP: "1.2.3.0",
ClusterID: "id",
},
{
IP: "5.6.7.0",
},
},
n2: []*models.IngressVip{
{
IP: "5.6.7.0",
},
{
IP: "2.2.3.0",
},
},
expectedResult: false,
},
}
for i := range tests {
t := tests[i]
It(t.name, func() {
Expect(AreIngressVipsIdentical(t.n1, t.n2)).To(Equal(t.expectedResult))
})
}
})

var _ = Describe("GetVips", func() {
var cluster *common.Cluster
var ApiVips []string
Expand Down

0 comments on commit 79584fa

Please sign in to comment.