From 4f756871f60ea371ac93e2fcc5866cde88152083 Mon Sep 17 00:00:00 2001 From: Ori Shoshan Date: Wed, 6 Nov 2024 09:47:20 +0200 Subject: [PATCH] Improve network policy comparison function to account for cases where network policies would do no-op updates when unnecessary (#512) --- src/go.mod | 1 + src/go.sum | 2 + .../database/database_reconciler.go | 6 + .../networkpolicy/compare.go | 198 +++++++++++++++--- .../networkpolicy/compare_test.go | 122 +++++++++++ 5 files changed, 302 insertions(+), 27 deletions(-) diff --git a/src/go.mod b/src/go.mod index ec4b4a483..e1e88cb70 100644 --- a/src/go.mod +++ b/src/go.mod @@ -96,6 +96,7 @@ require ( github.com/go-openapi/jsonpointer v0.19.6 // indirect github.com/go-openapi/jsonreference v0.20.2 // indirect github.com/go-openapi/swag v0.22.3 // indirect + github.com/go-test/deep v1.1.1 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/golang-jwt/jwt/v5 v5.2.0 // indirect github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect diff --git a/src/go.sum b/src/go.sum index 32d54678b..9009df5d5 100644 --- a/src/go.sum +++ b/src/go.sum @@ -224,6 +224,8 @@ github.com/go-sql-driver/mysql v1.8.1/go.mod h1:wEBSXgmK//2ZFJyE+qWnIsVGmvmEKlqw github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY= github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572 h1:tfuBGBXKqDEevZMzYi5KSi8KkcZtzBcTgAUUtapy0OI= github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572/go.mod h1:9Pwr4B2jHnOSGXyyzV8ROjYa2ojvAY6HCGYYfMoC3Ls= +github.com/go-test/deep v1.1.1 h1:0r/53hagsehfO4bzD2Pgr/+RgHqhmf+k1Bpse2cTu1U= +github.com/go-test/deep v1.1.1/go.mod h1:5C2ZWiW0ErCdrYzpqxLbTX7MG14M9iiw8DgHncVwcsE= github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ= github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= diff --git a/src/operator/controllers/intents_reconcilers/database/database_reconciler.go b/src/operator/controllers/intents_reconcilers/database/database_reconciler.go index 7ace275ef..ce7ed78c9 100644 --- a/src/operator/controllers/intents_reconcilers/database/database_reconciler.go +++ b/src/operator/controllers/intents_reconcilers/database/database_reconciler.go @@ -87,6 +87,12 @@ func (r *DatabaseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return ctrl.Result{}, errors.Wrap(err) } + if !lo.SomeBy(clientIntents.GetDatabaseIntents(), func(intent otterizev2alpha1.Target) bool { + return true + }) { + return ctrl.Result{}, nil + } + clusterID, err := r.getClusterID(ctx) if err != nil { return ctrl.Result{}, errors.Wrap(err) diff --git a/src/operator/controllers/intents_reconcilers/networkpolicy/compare.go b/src/operator/controllers/intents_reconcilers/networkpolicy/compare.go index cf623743d..3cc64c39b 100644 --- a/src/operator/controllers/intents_reconcilers/networkpolicy/compare.go +++ b/src/operator/controllers/intents_reconcilers/networkpolicy/compare.go @@ -1,10 +1,13 @@ package networkpolicy import ( + "github.com/go-test/deep" "github.com/otterize/intents-operator/src/shared/errors" networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "reflect" "sort" + "strings" ) // PAY ATTENTION: deepEqual is sensitive the differance between nil and empty slice @@ -33,7 +36,8 @@ func isNetworkPolicySpecEqual(spec1, spec2 networkingv1.NetworkPolicySpec) bool // Sort and compare Ingress rules sortIngressRules(spec1.Ingress) sortIngressRules(spec2.Ingress) - if !reflect.DeepEqual(spec1.Ingress, spec2.Ingress) { + if diffs := deep.Equal(spec1.Ingress, spec2.Ingress); len(diffs) != 0 { + println(diffs) return false } @@ -54,28 +58,132 @@ func isNetworkPolicySpecEqual(spec1, spec2 networkingv1.NetworkPolicySpec) bool return true } -// Helper function to sort Ingress rules by all relevant fields deterministically +// Sorts Ingress rules in a consistent order func sortIngressRules(rules []networkingv1.NetworkPolicyIngressRule) { for i := range rules { + // Sort the Ports and From fields within each rule sortNetworkPolicyPorts(rules[i].Ports) sortNetworkPolicyPeers(rules[i].From) } + + // Sort the rules based on Ports and From fields in a consistent order sort.SliceStable(rules, func(i, j int) bool { - return reflect.DeepEqual(rules[i], rules[j]) + // Compare Ports + for k := 0; k < len(rules[i].Ports) && k < len(rules[j].Ports); k++ { + if !portsEqual(rules[i].Ports[k], rules[j].Ports[k]) { + return lessPorts(rules[i].Ports[k], rules[j].Ports[k]) + } + } + if len(rules[i].Ports) != len(rules[j].Ports) { + return len(rules[i].Ports) < len(rules[j].Ports) + } + + // Compare From peers + for k := 0; k < len(rules[i].From) && k < len(rules[j].From); k++ { + if !peersEqual(rules[i].From[k], rules[j].From[k]) { + return lessPeers(rules[i].From[k], rules[j].From[k]) + } + } + return len(rules[i].From) < len(rules[j].From) }) } -// Helper function to sort Egress rules by all relevant fields deterministically +// Sorts Egress rules in a consistent order func sortEgressRules(rules []networkingv1.NetworkPolicyEgressRule) { for i := range rules { + // Sort the Ports and To fields within each rule sortNetworkPolicyPorts(rules[i].Ports) sortNetworkPolicyPeers(rules[i].To) } + + // Sort the rules based on Ports and To fields in a consistent order sort.SliceStable(rules, func(i, j int) bool { - return reflect.DeepEqual(rules[i], rules[j]) + // Compare Ports + for k := 0; k < len(rules[i].Ports) && k < len(rules[j].Ports); k++ { + if !portsEqual(rules[i].Ports[k], rules[j].Ports[k]) { + return lessPorts(rules[i].Ports[k], rules[j].Ports[k]) + } + } + if len(rules[i].Ports) != len(rules[j].Ports) { + return len(rules[i].Ports) < len(rules[j].Ports) + } + + // Compare To peers + for k := 0; k < len(rules[i].To) && k < len(rules[j].To); k++ { + if !peersEqual(rules[i].To[k], rules[j].To[k]) { + return lessPeers(rules[i].To[k], rules[j].To[k]) + } + } + return len(rules[i].To) < len(rules[j].To) }) } +// Helper function to check if two NetworkPolicyPort objects are equal +func portsEqual(port1, port2 networkingv1.NetworkPolicyPort) bool { + return reflect.DeepEqual(port1, port2) +} + +// Helper function to determine consistent ordering between two NetworkPolicyPort objects +func lessPorts(port1, port2 networkingv1.NetworkPolicyPort) bool { + if port1.Protocol != nil && port2.Protocol != nil { + if *port1.Protocol != *port2.Protocol { + return *port1.Protocol < *port2.Protocol + } + } else if port1.Protocol != port2.Protocol { + return port1.Protocol != nil + } + + if port1.Port != nil && port2.Port != nil { + return port1.Port.IntVal < port2.Port.IntVal + } + return port1.Port != nil +} + +// Helper function to check if two NetworkPolicyPeer objects are equal +func peersEqual(peer1, peer2 networkingv1.NetworkPolicyPeer) bool { + return reflect.DeepEqual(peer1, peer2) +} + +// Helper function to determine consistent ordering between two NetworkPolicyPeer objects +func lessPeers(peer1, peer2 networkingv1.NetworkPolicyPeer) bool { + if peer1.PodSelector != nil && peer2.PodSelector != nil { + if !reflect.DeepEqual(peer1.PodSelector, peer2.PodSelector) { + return peer1.PodSelector.String() < peer2.PodSelector.String() + } + } else if peer1.PodSelector != peer2.PodSelector { + return peer1.PodSelector != nil + } + + if peer1.NamespaceSelector != nil && peer2.NamespaceSelector != nil { + if !reflect.DeepEqual(peer1.NamespaceSelector, peer2.NamespaceSelector) { + return peer1.NamespaceSelector.String() < peer2.NamespaceSelector.String() + } + } else if peer1.NamespaceSelector != peer2.NamespaceSelector { + return peer1.NamespaceSelector != nil + } + + if peer1.IPBlock != nil && peer2.IPBlock != nil { + if peer1.IPBlock.CIDR != peer2.IPBlock.CIDR { + return peer1.IPBlock.CIDR < peer2.IPBlock.CIDR + } + // Sort by Except list if CIDRs are equal + return lessExceptList(peer1.IPBlock.Except, peer2.IPBlock.Except) + } + return peer1.IPBlock != nil +} + +// Helper function to determine consistent ordering between two lists of Except CIDRs +func lessExceptList(except1, except2 []string) bool { + sort.Strings(except1) + sort.Strings(except2) + for i := 0; i < len(except1) && i < len(except2); i++ { + if except1[i] != except2[i] { + return except1[i] < except2[i] + } + } + return len(except1) < len(except2) +} + // Helper function to sort NetworkPolicyPorts by all fields deterministically func sortNetworkPolicyPorts(ports []networkingv1.NetworkPolicyPort) { sort.SliceStable(ports, func(i, j int) bool { @@ -102,36 +210,72 @@ func sortNetworkPolicyPorts(ports []networkingv1.NetworkPolicyPort) { }) } -// Helper function to sort NetworkPolicyPeers by a deterministic order func sortNetworkPolicyPeers(peers []networkingv1.NetworkPolicyPeer) { sort.SliceStable(peers, func(i, j int) bool { - if peers[i].PodSelector != nil && peers[j].PodSelector != nil { - if !reflect.DeepEqual(peers[i].PodSelector, peers[j].PodSelector) { - return reflect.DeepEqual(peers[i].PodSelector, peers[j].PodSelector) - } - } else if peers[i].PodSelector != peers[j].PodSelector { - return peers[i].PodSelector != nil + return lessPeer(peers[i], peers[j]) + }) +} + +// Helper function to determine consistent ordering between two NetworkPolicyPeer objects +func lessPeer(peer1, peer2 networkingv1.NetworkPolicyPeer) bool { + // Compare PodSelector + if peer1.PodSelector != nil && peer2.PodSelector != nil { + if !selectorsEqual(*peer1.PodSelector, *peer2.PodSelector) { + return selectorToString(*peer1.PodSelector) < selectorToString(*peer2.PodSelector) } + } else if peer1.PodSelector != peer2.PodSelector { + return peer1.PodSelector != nil + } - if peers[i].NamespaceSelector != nil && peers[j].NamespaceSelector != nil { - if !reflect.DeepEqual(peers[i].NamespaceSelector, peers[j].NamespaceSelector) { - return reflect.DeepEqual(peers[i].NamespaceSelector, peers[j].NamespaceSelector) - } - } else if peers[i].NamespaceSelector != peers[j].NamespaceSelector { - return peers[i].NamespaceSelector != nil + // Compare NamespaceSelector + if peer1.NamespaceSelector != nil && peer2.NamespaceSelector != nil { + if !selectorsEqual(*peer1.NamespaceSelector, *peer2.NamespaceSelector) { + return selectorToString(*peer1.NamespaceSelector) < selectorToString(*peer2.NamespaceSelector) } + } else if peer1.NamespaceSelector != peer2.NamespaceSelector { + return peer1.NamespaceSelector != nil + } - if peers[i].IPBlock != nil && peers[j].IPBlock != nil { - if peers[i].IPBlock.CIDR != peers[j].IPBlock.CIDR { - return peers[i].IPBlock.CIDR < peers[j].IPBlock.CIDR - } + // Compare IPBlock + if peer1.IPBlock != nil && peer2.IPBlock != nil { + if peer1.IPBlock.CIDR != peer2.IPBlock.CIDR { + return peer1.IPBlock.CIDR < peer2.IPBlock.CIDR + } + // Sort by Except list if CIDRs are equal + return lessExceptList(peer1.IPBlock.Except, peer2.IPBlock.Except) + } + return peer1.IPBlock != nil +} - sort.Strings(peers[i].IPBlock.Except) - sort.Strings(peers[j].IPBlock.Except) - return reflect.DeepEqual(peers[i].IPBlock.Except, peers[j].IPBlock.Except) +// Helper function to check if two LabelSelectors are equal +func selectorsEqual(sel1, sel2 metav1.LabelSelector) bool { + return selectorToString(sel1) == selectorToString(sel2) +} + +// Helper function to convert a LabelSelector to a string for consistent comparison +func selectorToString(selector metav1.LabelSelector) string { + var sb strings.Builder + sb.WriteString("MatchLabels:") + if selector.MatchLabels != nil { + keys := make([]string, 0, len(selector.MatchLabels)) + for k := range selector.MatchLabels { + keys = append(keys, k) } - return peers[i].IPBlock != nil - }) + sort.Strings(keys) + for _, k := range keys { + sb.WriteString(k + "=" + selector.MatchLabels[k] + ",") + } + } + sb.WriteString("MatchExpressions:") + for _, expr := range selector.MatchExpressions { + sb.WriteString(expr.Key + "=" + string(expr.Operator) + "[") + sort.Strings(expr.Values) + for _, v := range expr.Values { + sb.WriteString(v + ",") + } + sb.WriteString("],") + } + return sb.String() } // Helper function to sort PolicyTypes diff --git a/src/operator/controllers/intents_reconcilers/networkpolicy/compare_test.go b/src/operator/controllers/intents_reconcilers/networkpolicy/compare_test.go index 9f6aa375b..ae71ab77d 100644 --- a/src/operator/controllers/intents_reconcilers/networkpolicy/compare_test.go +++ b/src/operator/controllers/intents_reconcilers/networkpolicy/compare_test.go @@ -298,4 +298,126 @@ spec: if !isNetworkPolicySpecEqual(netpolA.Spec, netpolB.Spec) { t.Error("Expected NetworkPolicySpecs to be equal") } + + if !isNetworkPolicySpecEqual(netpolB.Spec, netpolA.Spec) { + t.Error("Expected NetworkPolicySpecs to be equal") + } +} + +func TestNetworkPolicyRuleOrderIndependenceWithMultipleSelectors(t *testing.T) { + // Common fields for test initialization + tcp := v1.ProtocolTCP + udp := v1.ProtocolUDP + port80 := intstr.FromInt(80) + port443 := intstr.FromInt(443) + + // Define two NetworkPolicySpecs with the same Ingress and Egress rules in different orders, + // each with multiple PodSelector and NamespaceSelector entries in different orders + spec1 := networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "test"}, + }, + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + Ports: []networkingv1.NetworkPolicyPort{ + {Protocol: &tcp, Port: &port80}, + }, + From: []networkingv1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"role": "frontend"}, + }, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "backend"}, + }, + }, + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"role": "database"}, + }, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "frontend"}, + }, + }, + }, + }, + { + Ports: []networkingv1.NetworkPolicyPort{ + {Protocol: &udp, Port: &port443}, + }, + From: []networkingv1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"role": "cache"}, + }, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "data"}, + }, + }, + }, + }, + }, + Egress: []networkingv1.NetworkPolicyEgressRule{ + { + Ports: []networkingv1.NetworkPolicyPort{ + {Protocol: &tcp, Port: &port443}, + }, + To: []networkingv1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"role": "app"}, + }, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "frontend"}, + }, + }, + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"role": "api"}, + }, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "backend"}, + }, + }, + }, + }, + { + Ports: []networkingv1.NetworkPolicyPort{ + {Protocol: &udp, Port: &port80}, + }, + To: []networkingv1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"role": "service"}, + }, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "dev"}, + }, + }, + }, + }, + }, + } + + // Initialize spec2 by copying spec1 and swapping elements + spec2 := *spec1.DeepCopy() + + // Swap the order of Ingress rules + spec2.Ingress[0], spec2.Ingress[1] = spec2.Ingress[1], spec2.Ingress[0] + + // Swap the order of From peers in the first Ingress rule + spec2.Ingress[1].From[1], spec2.Ingress[1].From[0] = spec2.Ingress[1].From[0], spec2.Ingress[1].From[1] + + // Swap the order of Egress rules + spec2.Egress[0], spec2.Egress[1] = spec2.Egress[1], spec2.Egress[0] + + // Sort and compare + if !isNetworkPolicySpecEqual(spec1, spec2) { + t.Error("Expected NetworkPolicySpecs to be equal regardless of Ingress and Egress rule order and Peer selectors") + } + + // And in reverse + if !isNetworkPolicySpecEqual(spec2, spec1) { + t.Error("Expected NetworkPolicySpecs to be equal regardless of Ingress and Egress rule order and Peer selectors") + } }