From 160d6c209d7cd240612142b597a8146ecc825561 Mon Sep 17 00:00:00 2001 From: Maksim Zhylinski Date: Tue, 25 Jan 2022 21:28:31 +0100 Subject: [PATCH 1/4] Compare ports ingoring order and considering protocol defaults --- pkg/cluster/cluster.go | 42 ++++++++- pkg/cluster/cluster_test.go | 169 ++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index 967f9d530..570bce4c1 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -516,7 +516,7 @@ func (c *Cluster) compareContainers(description string, setA, setB []v1.Containe newCheck("new statefulset %s's %s (index %d) name does not match the current one", func(a, b v1.Container) bool { return a.Name != b.Name }), newCheck("new statefulset %s's %s (index %d) ports do not match the current one", - func(a, b v1.Container) bool { return !reflect.DeepEqual(a.Ports, b.Ports) }), + func(a, b v1.Container) bool { return !comparePorts(a.Ports, b.Ports) }), newCheck("new statefulset %s's %s (index %d) resources do not match the current ones", func(a, b v1.Container) bool { return !compareResources(&a.Resources, &b.Resources) }), newCheck("new statefulset %s's %s (index %d) environment does not match the current one", @@ -627,6 +627,46 @@ func compareSpiloConfiguration(configa, configb string) bool { return reflect.DeepEqual(oa, ob) } +func areProtocolsEqual(a, b v1.Protocol) bool { + return a == b || + (a == "" && b == v1.ProtocolTCP) || + (a == v1.ProtocolTCP && b == "") +} + +func comparePorts(a, b []v1.ContainerPort) bool { + if len(a) != len(b) { + return false + } + + areContainerPortsEqual := func(a, b v1.ContainerPort) bool { + return a.Name == b.Name && + a.HostPort == b.HostPort && + areProtocolsEqual(a.Protocol, b.Protocol) && + a.HostIP == b.HostIP + } + + findByPortValue := func(portSpecs []v1.ContainerPort, port int32) (v1.ContainerPort, bool) { + for _, portSpec := range portSpecs { + if portSpec.ContainerPort == port { + return portSpec, true + } + } + return v1.ContainerPort{}, false + } + + for _, portA := range a { + portB, found := findByPortValue(b, portA.ContainerPort) + if !found { + return false + } + if !areContainerPortsEqual(portA, portB) { + return false + } + } + + return true +} + func (c *Cluster) enforceMinResourceLimits(spec *acidv1.PostgresSpec) error { var ( diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go index dc1f5ff03..84cc3240b 100644 --- a/pkg/cluster/cluster_test.go +++ b/pkg/cluster/cluster_test.go @@ -2,6 +2,7 @@ package cluster import ( "fmt" + "github.com/stretchr/testify/assert" "reflect" "testing" @@ -1088,3 +1089,171 @@ func TestValidUsernames(t *testing.T) { } } } + +func TestCluster_compareContainers(t *testing.T) { + testCases := []struct { + name string + setA []v1.Container + setB []v1.Container + expectedNeedsRollUpdate bool + expectedReasons []string + }{ + { + name: "different set sizes", + setA: []v1.Container{ + {Name: "container1"}, + }, + setB: []v1.Container{ + {Name: "container1"}, + {Name: "container2"}, + }, + expectedNeedsRollUpdate: true, + expectedReasons: []string{ + "new statefulset description's length does not match the current ones", + }, + }, + { + name: "different container name", + setA: []v1.Container{ + {Name: "container1"}, + }, + setB: []v1.Container{ + {Name: "container2"}, + }, + expectedNeedsRollUpdate: true, + expectedReasons: []string{ + "new statefulset description's container1 (index 0) name does not match the current one", + }, + }, + { + name: "different ports", + setA: []v1.Container{ + { + Ports: []v1.ContainerPort{ + { + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, + }, + }, + }, + }, + setB: []v1.Container{ + { + Ports: []v1.ContainerPort{ + { + Name: "http", + ContainerPort: 80, + Protocol: v1.ProtocolTCP, + }, + }, + }, + }, + expectedNeedsRollUpdate: true, + expectedReasons: []string{ + "new statefulset description's (index 0) ports do not match the current one", + }, + }, + { + name: "no difference", + setA: []v1.Container{ + { + Name: "container1", + Ports: []v1.ContainerPort{ + { + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, + }, + }, + }, + }, + setB: []v1.Container{ + { + Name: "container1", + Ports: []v1.ContainerPort{ + { + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, + }, + }, + }, + }, + expectedNeedsRollUpdate: false, + }, + { + name: "same ports, different order", + setA: []v1.Container{ + { + Name: "container1", + Ports: []v1.ContainerPort{ + { + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, + }, + { + Name: "http", + ContainerPort: 80, + Protocol: v1.ProtocolTCP, + }, + }, + }, + }, + setB: []v1.Container{ + { + Name: "container1", + Ports: []v1.ContainerPort{ + { + Name: "http", + ContainerPort: 80, + Protocol: v1.ProtocolTCP, + }, + { + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, + }, + }, + }, + }, + expectedNeedsRollUpdate: false, + }, + { + name: "same ports, but one with default protocol", + setA: []v1.Container{ + { + Name: "container1", + Ports: []v1.ContainerPort{ + { + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, + }, + }, + }, + }, + setB: []v1.Container{ + { + Name: "container1", + Ports: []v1.ContainerPort{ + { + Name: "metrics", + ContainerPort: 9187, + }, + }, + }, + }, + expectedNeedsRollUpdate: false, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + needs, reasons := cl.compareContainers("description", testCase.setA, testCase.setB, false, nil) + assert.Equal(t, testCase.expectedNeedsRollUpdate, needs) + assert.ElementsMatch(t, testCase.expectedReasons, reasons) + }) + } +} From fd4765c472e93d2312876631b929ae62e526347e Mon Sep 17 00:00:00 2001 From: Maksim Zhylinski Date: Tue, 25 Jan 2022 21:35:24 +0100 Subject: [PATCH 2/4] Reorder imports --- pkg/cluster/cluster_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go index 84cc3240b..228f33624 100644 --- a/pkg/cluster/cluster_test.go +++ b/pkg/cluster/cluster_test.go @@ -2,10 +2,11 @@ package cluster import ( "fmt" - "github.com/stretchr/testify/assert" "reflect" "testing" + "github.com/stretchr/testify/assert" + "github.com/sirupsen/logrus" acidv1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1" fakeacidv1 "github.com/zalando/postgres-operator/pkg/generated/clientset/versioned/fake" From 196b4c6fbc7c02a8fae02772e7f19601e2df0350 Mon Sep 17 00:00:00 2001 From: Maksim Zhylinski Date: Wed, 2 Feb 2022 09:58:55 +0100 Subject: [PATCH 3/4] Reduce tests to comparePorts only --- pkg/cluster/cluster_test.go | 175 +++++++++++------------------------- 1 file changed, 53 insertions(+), 122 deletions(-) diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go index 228f33624..164a00791 100644 --- a/pkg/cluster/cluster_test.go +++ b/pkg/cluster/cluster_test.go @@ -1091,170 +1091,101 @@ func TestValidUsernames(t *testing.T) { } } -func TestCluster_compareContainers(t *testing.T) { +func TestClusterComparePorts(t *testing.T) { testCases := []struct { - name string - setA []v1.Container - setB []v1.Container - expectedNeedsRollUpdate bool - expectedReasons []string + name string + setA []v1.ContainerPort + setB []v1.ContainerPort + expected bool }{ - { - name: "different set sizes", - setA: []v1.Container{ - {Name: "container1"}, - }, - setB: []v1.Container{ - {Name: "container1"}, - {Name: "container2"}, - }, - expectedNeedsRollUpdate: true, - expectedReasons: []string{ - "new statefulset description's length does not match the current ones", - }, - }, - { - name: "different container name", - setA: []v1.Container{ - {Name: "container1"}, - }, - setB: []v1.Container{ - {Name: "container2"}, - }, - expectedNeedsRollUpdate: true, - expectedReasons: []string{ - "new statefulset description's container1 (index 0) name does not match the current one", - }, - }, { name: "different ports", - setA: []v1.Container{ + setA: []v1.ContainerPort{ { - Ports: []v1.ContainerPort{ - { - Name: "metrics", - ContainerPort: 9187, - Protocol: v1.ProtocolTCP, - }, - }, + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, }, }, - setB: []v1.Container{ + + setB: []v1.ContainerPort{ { - Ports: []v1.ContainerPort{ - { - Name: "http", - ContainerPort: 80, - Protocol: v1.ProtocolTCP, - }, - }, + Name: "http", + ContainerPort: 80, + Protocol: v1.ProtocolTCP, }, }, - expectedNeedsRollUpdate: true, - expectedReasons: []string{ - "new statefulset description's (index 0) ports do not match the current one", - }, + expected: false, }, { name: "no difference", - setA: []v1.Container{ + setA: []v1.ContainerPort{ { - Name: "container1", - Ports: []v1.ContainerPort{ - { - Name: "metrics", - ContainerPort: 9187, - Protocol: v1.ProtocolTCP, - }, - }, + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, }, }, - setB: []v1.Container{ + setB: []v1.ContainerPort{ { - Name: "container1", - Ports: []v1.ContainerPort{ - { - Name: "metrics", - ContainerPort: 9187, - Protocol: v1.ProtocolTCP, - }, - }, + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, }, }, - expectedNeedsRollUpdate: false, + expected: true, }, { name: "same ports, different order", - setA: []v1.Container{ + setA: []v1.ContainerPort{ { - Name: "container1", - Ports: []v1.ContainerPort{ - { - Name: "metrics", - ContainerPort: 9187, - Protocol: v1.ProtocolTCP, - }, - { - Name: "http", - ContainerPort: 80, - Protocol: v1.ProtocolTCP, - }, - }, + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, + }, + { + Name: "http", + ContainerPort: 80, + Protocol: v1.ProtocolTCP, }, }, - setB: []v1.Container{ + setB: []v1.ContainerPort{ + { + Name: "http", + ContainerPort: 80, + Protocol: v1.ProtocolTCP, + }, { - Name: "container1", - Ports: []v1.ContainerPort{ - { - Name: "http", - ContainerPort: 80, - Protocol: v1.ProtocolTCP, - }, - { - Name: "metrics", - ContainerPort: 9187, - Protocol: v1.ProtocolTCP, - }, - }, + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, }, }, - expectedNeedsRollUpdate: false, + expected: true, }, { name: "same ports, but one with default protocol", - setA: []v1.Container{ + setA: []v1.ContainerPort{ { - Name: "container1", - Ports: []v1.ContainerPort{ - { - Name: "metrics", - ContainerPort: 9187, - Protocol: v1.ProtocolTCP, - }, - }, + Name: "metrics", + ContainerPort: 9187, + Protocol: v1.ProtocolTCP, }, }, - setB: []v1.Container{ + setB: []v1.ContainerPort{ { - Name: "container1", - Ports: []v1.ContainerPort{ - { - Name: "metrics", - ContainerPort: 9187, - }, - }, + Name: "metrics", + ContainerPort: 9187, }, }, - expectedNeedsRollUpdate: false, + expected: true, }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - needs, reasons := cl.compareContainers("description", testCase.setA, testCase.setB, false, nil) - assert.Equal(t, testCase.expectedNeedsRollUpdate, needs) - assert.ElementsMatch(t, testCase.expectedReasons, reasons) + got := comparePorts(testCase.setA, testCase.setB) + assert.Equal(t, testCase.expected, got) }) } } From 5fd31bc83f6cb590efbcb6de2c226feca3972d58 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Mon, 28 Feb 2022 10:47:51 +0100 Subject: [PATCH 4/4] Update pkg/cluster/cluster_test.go --- pkg/cluster/cluster_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go index 164a00791..d06cc21e1 100644 --- a/pkg/cluster/cluster_test.go +++ b/pkg/cluster/cluster_test.go @@ -1091,7 +1091,7 @@ func TestValidUsernames(t *testing.T) { } } -func TestClusterComparePorts(t *testing.T) { +func TestComparePorts(t *testing.T) { testCases := []struct { name string setA []v1.ContainerPort