Add nodeSelector to Egress Firewall #3002

trozet · 2022-05-25T19:17:51Z

This allows destination addresses to be chosen via node selector for
egress firewall rules and addresses the use case where a user wants to
open holes in the firewall to specific nodes. For example, if a user
wants to enable kapi service accesss to host networked kapi server
endpoints.

Signed-off-by: Tim Rozet trozet@redhat.com

- What this PR does and why is it needed

- Special notes for reviewers

- How to verify it

- Description for the changelog

coveralls · 2022-05-25T19:37:21Z

Coverage increased (+0.2%) to 52.041% when pulling 9069a79 on trozet:ef_node_select into 1d976e4 on ovn-org:master.

go-controller/pkg/ovn/egressfirewall.go

go-controller/pkg/ovn/egressfirewall_test.go

go-controller/pkg/factory/factory.go

martinkennelly · 2022-05-27T14:18:27Z

go-controller/pkg/crd/egressfirewall/v1/types.go

 	// +kubebuilder:validation:Pattern=^([A-Za-z0-9-]+\.)*[A-Za-z0-9-]+\.?$
 	DNSName string `json:"dnsName,omitempty"`
+	// nodeSelector will allow/deny traffic to the Kubernetes node IP of selected nodes. If this is, set
+	// cidrSelector and DNSName must be unset.


how is this condition validated? I havent seen it so far. Are we rejecting when this condition isnt true otherwise the user will see unexpected results.

do u need // +optional or this new field , also indicate empty selector "Default" allow/deny to any node

will add @msherif1234

@martinkennelly I wasn't really validating it, just ignoring nodeSelector if one of the other 2 were set. This is how the previous egress firewall interacted between DNSName and CIDRSelector

The order right now is:

If dns name provided, use that

else if cidr , use that

else use node selector

I could add some validation to make sure only 1 is set. Seems like a good idea to me. wdyt?

go-controller/pkg/ovn/egressfirewall.go

msherif1234 · 2022-05-31T21:20:23Z

go-controller/pkg/crd/egressfirewall/v1/types.go

 	// +kubebuilder:validation:Pattern=^([A-Za-z0-9-]+\.)*[A-Za-z0-9-]+\.?$
 	DNSName string `json:"dnsName,omitempty"`
+	// nodeSelector will allow/deny traffic to the Kubernetes node IP of selected nodes. If this is, set
+	// cidrSelector and DNSName must be unset.


do u need // +optional or this new field , also indicate empty selector "Default" allow/deny to any node

go-controller/pkg/ovn/egressfirewall.go

msherif1234 · 2022-05-31T21:32:06Z

go-controller/pkg/ovn/egressfirewall.go

+		return nil
+	} else if len(egressFirewallACLs) > 1 {
+		klog.Errorf("Duplicate ACL found for egress firewall: %s", name)
+	}


do we need to return here with the above error ? in case retry help if it was transient error ?

If we find there are more than one ACL with the same name, which there shouldn't be, we just delete them all. IMO delete shouldn't fail here, we should just log that there is some error where we configured duplicates.

go-controller/pkg/ovn/obj_retry.go

msherif1234 · 2022-06-01T13:26:57Z

can u extend egfw e2e with nodeSelector test case

trozet · 2023-02-15T02:59:18Z

revived! latest update is just a rebase. I'll address everyone's comments tmrw. Thanks for all the feedback.

npinaeva · 2023-02-17T12:12:03Z

I don't know if you've considered an option to create address set per node selector, and only update it instead of acls?
That will require some more objects, but should be better from the nbdb side, and events handling too.
in general that should be similar to the EgressDNS we have now, but for node selectors.

trozet · 2023-02-23T20:33:32Z

I don't know if you've considered an option to create address set per node selector, and only update it instead of acls? That will require some more objects, but should be better from the nbdb side, and events handling too. in general that should be similar to the EgressDNS we have now, but for node selectors.

This does seem cleaner because then operations would simply be mutate to an address set rather than update an ACL. Let me see if I can easily change it.

trozet · 2023-02-27T19:48:28Z

Talking with @npinaeva today we decided to defer doing an address set to hold node IPs. The ideal case with an address set is to create one per node selector so that we could minimize resources and share where needed. However there are some limitations today with mapping external ids in OVN to the node selector. Once that issue is more generally solved within the project we will move the node selector in EF to use address set.

go-controller/pkg/ovn/egressfirewall.go

Follows the upstream implementation here: ovn-org/ovn-kubernetes#3002 Signed-off-by: Tim Rozet <trozet@redhat.com>

npinaeva

Logic lgtm, had some comments for tests

npinaeva · 2023-02-28T09:08:24Z

go-controller/pkg/ovn/egressfirewall_test.go

@@ -608,6 +615,257 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 				gomega.Expect(err).NotTo(gomega.HaveOccurred())

 			})
+			ginkgo.It("egress firewall with node selector updates during node update", func() {


we either should add gwMode in the name here, or if you don't need to test that for gateway modes, move it outside of for _, gwMode := range []config.GatewayMode loop
same for the second test

I think this was just something I missed when I rebased. I think in the past the tests were split into local and shared and for create vs update and it didnt make sense. iirc you fixed it. I'll update the test to use the right gw mode.

npinaeva · 2023-02-28T09:08:57Z

go-controller/pkg/ovn/egressfirewall_test.go

@@ -608,6 +615,257 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 				gomega.Expect(err).NotTo(gomega.HaveOccurred())

 			})
+			ginkgo.It("egress firewall with node selector updates during node update", func() {
+				config.Gateway.Mode = config.GatewayModeShared


npinaeva · 2023-02-28T09:10:09Z

go-controller/pkg/ovn/egressfirewall_test.go

+				config.Gateway.Mode = config.GatewayModeShared
+				config.Gateway.NodeportEnable = true
+				const (
+					namespaceName        = "namespace1"


npinaeva · 2023-02-28T09:13:51Z

go-controller/pkg/ovn/egressfirewall_test.go

+						node1Name string = "node1"
+					)
+
+					expectedOVNClusterRouter := &nbdb.LogicalRouter{


Use functions like newOVNClusterRouter() newRouterPortGroup() etc, check hybrid_test for examples

npinaeva · 2023-02-28T09:15:43Z

go-controller/pkg/ovn/egressfirewall_test.go

+						SBData: []libovsdbtest.TestData{datapath},
+					}
+
+					selector := metav1.LabelSelector{MatchLabels: map[string]string{"name": "test"}}


maybe make "name" and "test" constants?

npinaeva · 2023-02-28T09:39:50Z

go-controller/pkg/ovn/egressfirewall_test.go

+						buildEgressFwAclName("namespace1", t.EgressFirewallStartPriority),
+						nbdb.ACLDirectionToLport,
+						t.EgressFirewallStartPriority,
+						"(ip4.dst == 9.9.9.9) && ip4.src == $a10481622940199974102",


9.9.9.9 -> node1.NodeIP
a10481622940199974102 -> asHash like https://github.com/ovn-org/ovn-kubernetes/blob/master/go-controller/pkg/ovn/egressfirewall_test.go#L281

npinaeva · 2023-02-28T09:40:21Z

go-controller/pkg/ovn/egressfirewall_test.go

+						t.OvnACLLoggingMeter,
+						"",
+						false,
+						map[string]string{"egressFirewall": "namespace1"},


"namespace1" -> namespace1.Name

didn't work? :D

npinaeva · 2023-02-28T09:41:45Z

go-controller/pkg/ovn/egressfirewall_test.go

+					_, err = fakeOVN.fakeClient.KubeClient.CoreV1().Nodes().Patch(context.TODO(), node1Name, types.MergePatchType, patchData, metav1.PatchOptions{})
+					gomega.Expect(err).NotTo(gomega.HaveOccurred())
+					expectedClusterPortGroup.ACLs = []string{}
+					expectedDatabaseState = append(expectedDatabaseState)


npinaeva · 2023-02-28T09:48:51Z

go-controller/pkg/ovn/egressfirewall_test.go

+		node1Name string = "node1"
+		node2Name string = "node2"


npinaeva · 2023-02-28T09:50:27Z

test/e2e/e2e.go

+    to:
+      cidrSelector: %s
+`, f.Namespace.Name, exFWPermitTcpDnsDest, singleIPMask, exFWPermitCIDR, f.Namespace.Name, labelMatch, exFWDenyCIDR)
+		framework.Logf("Egress Firewall CR generated: %s", egressFirewallConfig)


do we need this log?

I found it was useful because then I can see the labelMatch string generated

that is more of a test debugging right? once you know the CR is correct, you don't need the log?

This allows destination addresses to be chosen via node selector for egress firewall rules and addresses the use case where a user wants to open holes in the firewall to specific nodes. For example, if a user wants to enable kapi service accesss to host networked kapi server endpoints. Signed-off-by: Tim Rozet <trozet@redhat.com>

npinaeva

some more renames would be nice, but that is lgtm anyway

npinaeva · 2023-02-28T19:20:14Z

go-controller/pkg/ovn/egressfirewall_test.go

@@ -611,6 +618,238 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 				gomega.Expect(err).NotTo(gomega.HaveOccurred())

 			})
+			ginkgo.It(fmt.Sprintf("egress firewall with node selector updates during node update, gateway mode %s", gwMode), func() {
+				config.Gateway.Mode = gwMode


you don't need that, it is set in the beginning of the loop

actually this whole for loop doesn't work properly with setting up gwMode, but I can update it in another pr

npinaeva · 2023-02-28T19:22:38Z

go-controller/pkg/ovn/egressfirewall_test.go

+					gomega.Expect(err).NotTo(gomega.HaveOccurred())
+					asHash, _ := getNsAddrSetHashNames(namespace1.Name)
+					ipv4ACL := libovsdbops.BuildACL(
+						buildEgressFwAclName("namespace1", t.EgressFirewallStartPriority),


"namespace1" -> namespace1.Name

npinaeva · 2023-02-28T19:23:11Z

go-controller/pkg/ovn/egressfirewall_test.go

+						t.OvnACLLoggingMeter,
+						"",
+						false,
+						map[string]string{"egressFirewall": "namespace1"},


didn't work? :D

tssurya

only reviewed the CRD parts in this PR since I am reviewing the CNO PR.

tssurya · 2023-03-01T06:59:43Z

dist/templates/k8s.ovn.org_egressfirewalls.yaml.j2

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
  annotations:
-    controller-gen.kubebuilder.io/version: v0.11.3
+    controller-gen.kubebuilder.io/version: v0.7.0


why was the client-gen version downgraded here?

tssurya · 2023-03-01T07:02:55Z

dist/templates/k8s.ovn.org_egressfirewalls.yaml.j2

                          pattern: ^([A-Za-z0-9-]+\.)*[A-Za-z0-9-]+\.?$
                          type: string
+                        nodeSelector:
+                          description: nodeSelector will allow/deny traffic to the
+                            Kubernetes node IP of selected nodes. If this is, set


nit: If this is set, cidrSelector and DNSName must be unset

tssurya · 2023-03-01T07:13:42Z

dist/templates/k8s.ovn.org_egressfirewalls.yaml.j2

                      type: object
-                      minProperties: 1


I don't understand why we removed min and max properties here? Even if we added a new nodeSelector, the rule still holds that we need at least one of them set (nodeselector/dnsname/cidrselector) and that only max one can be set which is clear from the description of each field.
Removing kubebuilder rules breaks backwards compatibility of how API would have complained around this and now it would stop complaining if more than one is set OR none are set.

lol I know why this was removed, its because we never had it in the first place! Seems like it was hardcoded:
We might need to change the types.go file to actually reflect and incorporate this:

// to is the target that traffic is allowed/denied to // +kubebuilder:validation:MinProperties:=1 // +kubebuilder:validation:MaxProperties:=1 To EgressFirewallDestination `json:"to"`

Reason being API will now start behaving differently than before if we remove these validation hooks.

Follows the upstream implementation here: ovn-org/ovn-kubernetes#3002 Signed-off-by: Tim Rozet <trozet@redhat.com>

trozet requested review from JacobTanenbaum and npinaeva May 25, 2022 19:19

trozet mentioned this pull request May 25, 2022

Remove egress firewall cache #2959

Closed

martinkennelly requested changes May 27, 2022

View reviewed changes

npinaeva reviewed May 29, 2022

View reviewed changes

go-controller/pkg/ovn/egressfirewall.go Show resolved Hide resolved

npinaeva reviewed May 29, 2022

View reviewed changes

go-controller/pkg/ovn/egressfirewall.go Outdated Show resolved Hide resolved

msherif1234 reviewed May 31, 2022

View reviewed changes

huiran0826 mentioned this pull request Sep 8, 2022

OSDOCS-3908: enhances egressfirewall to include nodeSelector openshift/openshift-docs#49806

Merged

trozet force-pushed the ef_node_select branch from 9069a79 to 2e738fc Compare February 15, 2023 02:57

trozet force-pushed the ef_node_select branch 2 times, most recently from cffedaf to 5fe0052 Compare February 15, 2023 18:57

trozet force-pushed the ef_node_select branch from 5fe0052 to 7b570b2 Compare February 27, 2023 19:46

trozet force-pushed the ef_node_select branch from 7b570b2 to 1a331d9 Compare February 27, 2023 19:51

trozet commented Feb 27, 2023

View reviewed changes

go-controller/pkg/ovn/egressfirewall.go Show resolved Hide resolved

trozet mentioned this pull request Feb 27, 2023

Enables nodeSelector to be used in egress firewall rule openshift/cluster-network-operator#1720

Merged

trozet added a commit to trozet/cluster-network-operator that referenced this pull request Feb 27, 2023

Enables nodeSelector to be used in egress firewall rule

e79be22

Follows the upstream implementation here: ovn-org/ovn-kubernetes#3002 Signed-off-by: Tim Rozet <trozet@redhat.com>

npinaeva reviewed Feb 28, 2023

View reviewed changes

trozet force-pushed the ef_node_select branch from 1a331d9 to 34350bd Compare February 28, 2023 16:24

npinaeva approved these changes Feb 28, 2023

View reviewed changes

trozet merged commit 2136c9e into ovn-org:master Feb 28, 2023

tssurya reviewed Mar 1, 2023

View reviewed changes

trozet added a commit to trozet/cluster-network-operator that referenced this pull request Mar 1, 2023

Enables nodeSelector to be used in egress firewall rule

d91d378

Follows the upstream implementation here: ovn-org/ovn-kubernetes#3002 Signed-off-by: Tim Rozet <trozet@redhat.com>

Add nodeSelector to Egress Firewall #3002

Add nodeSelector to Egress Firewall #3002

Conversation

trozet commented May 25, 2022

coveralls commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msherif1234 commented Jun 1, 2022

trozet commented Feb 15, 2023

npinaeva commented Feb 17, 2023

trozet commented Feb 23, 2023

trozet commented Feb 27, 2023

npinaeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npinaeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tssurya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment