diff --git a/internal/ingress/annotations/authreq/main.go b/internal/ingress/annotations/authreq/main.go index 1cf2de222..93666d1aa 100644 --- a/internal/ingress/annotations/authreq/main.go +++ b/internal/ingress/annotations/authreq/main.go @@ -28,6 +28,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress/annotations/parser" ing_errors "k8s.io/ingress-nginx/internal/ingress/errors" "k8s.io/ingress-nginx/internal/ingress/resolver" + "k8s.io/ingress-nginx/internal/sets" ) // Config returns external authentication configuration for an Ingress rule @@ -62,18 +63,12 @@ func (e1 *Config) Equal(e2 *Config) bool { if e1.Method != e2.Method { return false } - for _, ep1 := range e1.ResponseHeaders { - found := false - for _, ep2 := range e2.ResponseHeaders { - if ep1 == ep2 { - found = true - break - } - } - if !found { - return false - } + + match := sets.StringElementsMatch(e1.ResponseHeaders, e2.ResponseHeaders) + if !match { + return false } + if e1.RequestRedirect != e2.RequestRedirect { return false } diff --git a/internal/ingress/annotations/ipwhitelist/main.go b/internal/ingress/annotations/ipwhitelist/main.go index e930f2f5f..cb61a9fae 100644 --- a/internal/ingress/annotations/ipwhitelist/main.go +++ b/internal/ingress/annotations/ipwhitelist/main.go @@ -28,6 +28,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress/annotations/parser" ing_errors "k8s.io/ingress-nginx/internal/ingress/errors" "k8s.io/ingress-nginx/internal/ingress/resolver" + "k8s.io/ingress-nginx/internal/sets" ) // SourceRange returns the CIDR @@ -44,23 +45,11 @@ func (sr1 *SourceRange) Equal(sr2 *SourceRange) bool { return false } - if len(sr1.CIDR) != len(sr2.CIDR) { + match := sets.StringElementsMatch(sr1.CIDR, sr2.CIDR) + if !match { return false } - for _, s1l := range sr1.CIDR { - found := false - for _, sl2 := range sr2.CIDR { - if s1l == sl2 { - found = true - break - } - } - if !found { - return false - } - } - return true } diff --git a/internal/ingress/annotations/luarestywaf/main.go b/internal/ingress/annotations/luarestywaf/main.go index 39ac19382..05221342c 100644 --- a/internal/ingress/annotations/luarestywaf/main.go +++ b/internal/ingress/annotations/luarestywaf/main.go @@ -17,7 +17,6 @@ limitations under the License. package luarestywaf import ( - "reflect" "strings" extensions "k8s.io/api/extensions/v1beta1" @@ -25,6 +24,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress/annotations/parser" "k8s.io/ingress-nginx/internal/ingress/errors" "k8s.io/ingress-nginx/internal/ingress/resolver" + "k8s.io/ingress-nginx/internal/sets" ) var luaRestyWAFModes = map[string]bool{"ACTIVE": true, "INACTIVE": true, "SIMULATE": true} @@ -54,9 +54,12 @@ func (e1 *Config) Equal(e2 *Config) bool { if e1.Debug != e2.Debug { return false } - if !reflect.DeepEqual(e1.IgnoredRuleSets, e2.IgnoredRuleSets) { + + match := sets.StringElementsMatch(e1.IgnoredRuleSets, e2.IgnoredRuleSets) + if !match { return false } + if e1.ExtraRulesetString != e2.ExtraRulesetString { return false } diff --git a/internal/ingress/annotations/ratelimit/main.go b/internal/ingress/annotations/ratelimit/main.go index 44e1a46c9..04a89184a 100644 --- a/internal/ingress/annotations/ratelimit/main.go +++ b/internal/ingress/annotations/ratelimit/main.go @@ -27,6 +27,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress/annotations/parser" "k8s.io/ingress-nginx/internal/ingress/resolver" "k8s.io/ingress-nginx/internal/net" + "k8s.io/ingress-nginx/internal/sets" ) const ( @@ -94,17 +95,9 @@ func (rt1 *Config) Equal(rt2 *Config) bool { return false } - for _, r1l := range rt1.Whitelist { - found := false - for _, rl2 := range rt2.Whitelist { - if r1l == rl2 { - found = true - break - } - } - if !found { - return false - } + match := sets.StringElementsMatch(rt1.Whitelist, rt2.Whitelist) + if !match { + return false } return true diff --git a/internal/ingress/controller/util.go b/internal/ingress/controller/util.go index 13a39e146..d7e88ec7e 100644 --- a/internal/ingress/controller/util.go +++ b/internal/ingress/controller/util.go @@ -17,17 +17,15 @@ limitations under the License. package controller import ( + "fmt" "os" "os/exec" "syscall" - "k8s.io/apimachinery/pkg/util/intstr" - - "fmt" - "k8s.io/klog" api "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/kubernetes/pkg/util/sysctl" "k8s.io/ingress-nginx/internal/ingress" diff --git a/internal/ingress/type_equals_test.go b/internal/ingress/type_equals_test.go index ca561a462..8a078b6ae 100644 --- a/internal/ingress/type_equals_test.go +++ b/internal/ingress/type_equals_test.go @@ -71,3 +71,56 @@ func readJSON(p string) (*Configuration, error) { return &c, nil } + +func TestL4ServiceElementsMatch(t *testing.T) { + testCases := []struct { + listA []L4Service + listB []L4Service + expected bool + }{ + {nil, nil, true}, + {[]L4Service{{Port: 80}}, nil, false}, + {[]L4Service{{Port: 80}}, []L4Service{{Port: 80}}, true}, + { + []L4Service{ + {Port: 80, Endpoints: []Endpoint{{Address: "1.1.1.1"}}}}, + []L4Service{{Port: 80}}, + false, + }, + { + []L4Service{ + {Port: 80, Endpoints: []Endpoint{{Address: "1.1.1.1"}, {Address: "1.1.1.2"}}}}, + []L4Service{ + {Port: 80, Endpoints: []Endpoint{{Address: "1.1.1.2"}, {Address: "1.1.1.1"}}}}, + true, + }, + } + + for _, testCase := range testCases { + result := compareL4Service(testCase.listA, testCase.listB) + if result != testCase.expected { + t.Errorf("expected %v but returned %v (%v - %v)", testCase.expected, result, testCase.listA, testCase.listB) + } + } +} + +func TestIntElementsMatch(t *testing.T) { + testCases := []struct { + listA []int + listB []int + expected bool + }{ + {nil, nil, true}, + {[]int{}, nil, false}, + {[]int{}, []int{1}, false}, + {[]int{1}, []int{1}, true}, + {[]int{1, 2, 3}, []int{3, 2, 1}, true}, + } + + for _, testCase := range testCases { + result := compareInts(testCase.listA, testCase.listB) + if result != testCase.expected { + t.Errorf("expected %v but returned %v (%v - %v)", testCase.expected, result, testCase.listA, testCase.listB) + } + } +} diff --git a/internal/ingress/types_equals.go b/internal/ingress/types_equals.go index 8b6f3e132..f838bb0aa 100644 --- a/internal/ingress/types_equals.go +++ b/internal/ingress/types_equals.go @@ -16,6 +16,10 @@ limitations under the License. package ingress +import ( + "k8s.io/ingress-nginx/internal/sets" +) + // Equal tests for equality between two Configuration types func (c1 *Configuration) Equal(c2 *Configuration) bool { if c1 == c2 { @@ -25,23 +29,11 @@ func (c1 *Configuration) Equal(c2 *Configuration) bool { return false } - if len(c1.Backends) != len(c2.Backends) { + match := compareBackends(c1.Backends, c2.Backends) + if !match { return false } - for _, c1b := range c1.Backends { - found := false - for _, c2b := range c2.Backends { - if c1b.Equal(c2b) { - found = true - break - } - } - if !found { - return false - } - } - if len(c1.Servers) != len(c2.Servers) { return false } @@ -53,41 +45,20 @@ func (c1 *Configuration) Equal(c2 *Configuration) bool { } } - if len(c1.TCPEndpoints) != len(c2.TCPEndpoints) { + match = compareL4Service(c1.TCPEndpoints, c2.TCPEndpoints) + if !match { return false } - for _, tcp1 := range c1.TCPEndpoints { - found := false - for _, tcp2 := range c2.TCPEndpoints { - if (&tcp1).Equal(&tcp2) { - found = true - break - } - } - if !found { - return false - } - } - if len(c1.UDPEndpoints) != len(c2.UDPEndpoints) { + match = compareL4Service(c1.UDPEndpoints, c2.UDPEndpoints) + if !match { return false } - for _, udp1 := range c1.UDPEndpoints { - found := false - for _, udp2 := range c2.UDPEndpoints { - if (&udp1).Equal(&udp2) { - found = true - break - } - } - if !found { - return false - } - } if len(c1.PassthroughBackends) != len(c2.PassthroughBackends) { return false } + for _, ptb1 := range c1.PassthroughBackends { found := false for _, ptb2 := range c2.PassthroughBackends { @@ -158,38 +129,18 @@ func (b1 *Backend) Equal(b2 *Backend) bool { return false } - if len(b1.Endpoints) != len(b2.Endpoints) { + match := compareEndpoints(b1.Endpoints, b2.Endpoints) + if !match { return false } - for _, udp1 := range b1.Endpoints { - found := false - for _, udp2 := range b2.Endpoints { - if (&udp1).Equal(&udp2) { - found = true - break - } - } - if !found { - return false - } - } - if !b1.TrafficShapingPolicy.Equal(b2.TrafficShapingPolicy) { return false } - for _, vb1 := range b1.AlternativeBackends { - found := false - for _, vb2 := range b2.AlternativeBackends { - if vb1 == vb2 { - found = true - break - } - } - if !found { - return false - } + match = sets.StringElementsMatch(b1.AlternativeBackends, b2.AlternativeBackends) + if !match { + return false } return true @@ -452,21 +403,10 @@ func (l1 *Location) Equal(l2 *Location) bool { return false } - if len(l1.CustomHTTPErrors) != len(l2.CustomHTTPErrors) { + match := compareInts(l1.CustomHTTPErrors, l2.CustomHTTPErrors) + if !match { return false } - for _, code1 := range l1.CustomHTTPErrors { - found := false - for _, code2 := range l2.CustomHTTPErrors { - if code1 == code2 { - found = true - break - } - } - if !found { - return false - } - } if !(&l1.ModSecurity).Equal(&l2.ModSecurity) { return false @@ -530,21 +470,10 @@ func (e1 *L4Service) Equal(e2 *L4Service) bool { if !(&e1.Backend).Equal(&e2.Backend) { return false } - if len(e1.Endpoints) != len(e2.Endpoints) { - return false - } - for _, ep1 := range e1.Endpoints { - found := false - for _, ep2 := range e2.Endpoints { - if (&ep1).Equal(&ep2) { - found = true - break - } - } - if !found { - return false - } + match := compareEndpoints(e1.Endpoints, e2.Endpoints) + if !match { + return false } return true @@ -598,18 +527,82 @@ func (s1 *SSLCert) Equal(s2 *SSLCert) bool { return false } - for _, cn1 := range s1.CN { - found := false - for _, cn2 := range s2.CN { - if cn1 == cn2 { - found = true - break - } - } - if !found { - return false - } + match := sets.StringElementsMatch(s1.CN, s2.CN) + if !match { + return false } return true } + +var compareEndpointsFunc = func(e1, e2 interface{}) bool { + ep1, ok := e1.(Endpoint) + if !ok { + return false + } + + ep2, ok := e2.(Endpoint) + if !ok { + return false + } + + return (&ep1).Equal(&ep2) +} + +func compareEndpoints(a, b []Endpoint) bool { + return sets.Compare(a, b, compareEndpointsFunc) +} + +var compareBackendsFunc = func(e1, e2 interface{}) bool { + b1, ok := e1.(*Backend) + if !ok { + return false + } + + b2, ok := e2.(*Backend) + if !ok { + return false + } + + return b1.Equal(b2) +} + +func compareBackends(a, b []*Backend) bool { + return sets.Compare(a, b, compareBackendsFunc) +} + +var compareIntsFunc = func(e1, e2 interface{}) bool { + b1, ok := e1.(int) + if !ok { + return false + } + + b2, ok := e2.(int) + if !ok { + return false + } + + return b1 == b2 +} + +func compareInts(a, b []int) bool { + return sets.Compare(a, b, compareIntsFunc) +} + +var compareL4ServiceFunc = func(e1, e2 interface{}) bool { + b1, ok := e1.(L4Service) + if !ok { + return false + } + + b2, ok := e2.(L4Service) + if !ok { + return false + } + + return (&b1).Equal(&b2) +} + +func compareL4Service(a, b []L4Service) bool { + return sets.Compare(a, b, compareL4ServiceFunc) +} diff --git a/internal/sets/match.go b/internal/sets/match.go new file mode 100644 index 000000000..3732176db --- /dev/null +++ b/internal/sets/match.go @@ -0,0 +1,101 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package sets + +import ( + "reflect" +) + +type equalFunction func(e1, e2 interface{}) bool + +// Compare checks if the parameters are iterable and contains the same elements +func Compare(listA, listB interface{}, eq equalFunction) bool { + a := reflect.ValueOf(listA) + ok := isIterable(a) + if !ok { + return false + } + + b := reflect.ValueOf(listB) + ok = isIterable(b) + if !ok { + return false + } + + if a.IsNil() && b.IsNil() { + return true + } + + if a.IsNil() != b.IsNil() { + return false + } + + if a.Len() != b.Len() { + return false + } + + visited := make([]bool, b.Len()) + + for i := 0; i < a.Len(); i++ { + found := false + for j := 0; j < b.Len(); j++ { + if visited[j] { + continue + } + + if eq(a.Index(i).Interface(), b.Index(j).Interface()) { + visited[j] = true + found = true + break + } + } + + if !found { + return false + } + } + + return true +} + +var compareStrings = func(e1, e2 interface{}) bool { + s1, ok := e1.(string) + if !ok { + return false + } + + s2, ok := e2.(string) + if !ok { + return false + } + + return s1 == s2 +} + +// StringElementsMatch compares two string slices and returns if are equals +func StringElementsMatch(a, b []string) bool { + return Compare(a, b, compareStrings) +} + +func isIterable(obj reflect.Value) bool { + switch reflect.TypeOf(obj).Kind() { + case reflect.Struct, reflect.Slice, reflect.Array: + return true + default: + return false + } +} diff --git a/internal/sets/match_test.go b/internal/sets/match_test.go new file mode 100644 index 000000000..e2366d2c7 --- /dev/null +++ b/internal/sets/match_test.go @@ -0,0 +1,60 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package sets + +import ( + "testing" +) + +var ( + testCasesElementMatch = []struct { + listA []string + listB []string + expected bool + }{ + {nil, nil, true}, + {[]string{"1"}, nil, false}, + {[]string{"1"}, []string{"1"}, true}, + {[]string{"1", "2", "1"}, []string{"1", "1", "2"}, true}, + {[]string{"1", "3", "1"}, []string{"1", "1", "2"}, false}, + {[]string{"1", "1"}, []string{"1", "2"}, false}, + } +) + +func TestElementsMatch(t *testing.T) { + + for _, testCase := range testCasesElementMatch { + result := StringElementsMatch(testCase.listA, testCase.listB) + if result != testCase.expected { + t.Errorf("expected %v but returned %v (%v - %v)", testCase.expected, result, testCase.listA, testCase.listB) + } + + sameResult := StringElementsMatch(testCase.listB, testCase.listA) + if sameResult != testCase.expected { + t.Errorf("expected %v but returned %v (%v - %v)", testCase.expected, sameResult, testCase.listA, testCase.listB) + } + } +} + +func BenchmarkStringElementsMatchReflection(b *testing.B) { + b.ResetTimer() + for i := 0; i < b.N; i++ { + for _, test := range testCasesElementMatch { + StringElementsMatch(test.listA, test.listB) + } + } +}