SDN-3708: Support configurable masquerade subnet in ovn-k #1807

bpickard22 · 2023-05-08T22:05:04Z

Adds the necessary knobs in the cno to consume the api change (openshift/api#1410) exposing the internal masquerade subnet in ovn-k where we reserve addresses for ip masquerading

bpickard22 · 2023-05-15T20:47:36Z

/hold depends on openshift/api#1410

tssurya · 2023-05-16T14:10:39Z

/assign @tssurya

flavio-fernandes · 2023-08-08T15:53:05Z

pkg/network/ovn_kubernetes_test.go

@@ -179,6 +179,7 @@ func TestRenderedOVNKubernetesConfig(t *testing.T) {
 		egressIPConfig         *operv1.EgressIPConfig
 		masterIPs              []string
 		v4InternalSubnet       string
+		v4InternalMasqueradeSubnet string


NIT align all lines above and below this to accommodate the longer string

The IDE should do it automagically :)

ricky-rav · 2023-08-09T08:38:38Z

@bpickard22 let's make sure CNO builds with the changes in ovn_kubernetes.go :)
https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-network-operator/1807/pull-ci-openshift-cluster-network-operator-master-verify/1687179374593314816

ricky-rav

I gave the PR a first pass. We need the OVNK PR to merge first, the API PR second, and then once we update vendor/github.com/openshift/api/ with the new API, we can finally test this CNO PR.

pkg/network/ovn_kubernetes.go

ricky-rav · 2023-08-09T14:51:55Z

pkg/network/ovn_kubernetes_test.go

@@ -179,6 +179,7 @@ func TestRenderedOVNKubernetesConfig(t *testing.T) {
 		egressIPConfig         *operv1.EgressIPConfig
 		masterIPs              []string
 		v4InternalSubnet       string
+		v4InternalMasqueradeSubnet string


The IDE should do it automagically :)

pkg/network/ovn_kubernetes.go

ricky-rav · 2023-08-10T08:28:47Z

pkg/network/ovn_kubernetes.go

 			}
 			_, v4Net, err = net.ParseCIDR(oc.V4InternalSubnet)
 			if err != nil {
 				out = append(out, errors.Errorf("v4InternalSubnet is invalid: %s", err))
 			}
 			if !isV4InternalSubnetLargeEnough(conf) {
-				out = append(out, errors.Errorf("v4InternalSubnet is no large enough for the maximum number of nodes which can be supported by ClusterNetwork"))
+				out = append(out, errors.Errorf("v4InternalSubnet is not large enough for the maximum number of nodes which can be supported by ClusterNetwork"))


let's print what v4InternalSubnet is

this is the join subnet which is not part of this feature, but we can go ahead and update that log

ricky-rav · 2023-08-10T08:28:57Z

pkg/network/ovn_kubernetes.go

@@ -920,7 +923,7 @@ func validateOVNKubernetes(conf *operv1.NetworkSpec) []error {
 				out = append(out, errors.Errorf("v6InternalSubnet is invalid: %s", err))
 			}
 			if !isV6InternalSubnetLargeEnough(conf) {
-				out = append(out, errors.Errorf("v6InternalSubnet is no large enough for the maximum number of nodes which can be supported by ClusterNetwork"))
+				out = append(out, errors.Errorf("v6InternalSubnet is not large enough for the maximum number of nodes which can be supported by ClusterNetwork"))


let's print what v6InternalSubnet is

ricky-rav · 2023-08-10T09:55:18Z

@bpickard22 so in order to test CNO against the API changes that are still to be merged, you can modify the go.mod file to point to your own fork of openshift/api with the commit hash from your API PR. I tried it here: ricky-rav@94ac16e

Basically in the "replace" part of the go.mod file, you type:
github.com/openshift/api => github.com/bpickard22/api 680fd20165a4385e0b92a50ecd91d6c3763bd5d1

Then you run go mod vendor and it will rewrite the commit ID into the "v0.0.0..." form:
github.com/openshift/api => github.com/bpickard22/api v0.0.0-20230804184907-680fd20165a4

Of course you'll undo this once the API PR merges, but at least we can move forward with this CNO in the meantime :)

pkg/network/ovn_kubernetes.go

zshi-redhat · 2023-09-20T12:40:53Z

@bpickard22 Is this PR ready to merge?

jcaamano

I think that the first commit needs to be dropped as it should be merged already.

jcaamano · 2024-01-10T10:07:27Z

bindata/network/ovn-kubernetes/self-hosted/004-config.yaml

+{{- if (index . "V4InternalMasqueradeSubnet")}}
+    v4-internal-masquerade-subnet="{{.V4InternalMasqueradeSubnet}}"
+{{- end }}
+{{- if (index . "V6InternalMasqueradeSubnet")}}
+    v6-internal-masquerade-subnet="{{.V6InternalMasqueradeSubnet}}"
+{{- end }}
+


I don't think that this will cause a rollout/restart of ovn-k, making it a day-1 only configuration parameter. Is that ok?

yes that is the current intended behavior

jcaamano · 2024-01-10T10:12:09Z

pkg/network/ovn_kubernetes.go

+				_, v4Net, err = net.ParseCIDR(oc.GatewayConfig.IPv4.InternalMasqueradeSubnet)
+				if err != nil {
+					out = append(out, errors.Errorf("v4InternalMasqueradeSubnet is invalid: %s", err))
+				}


I see in other places that the error returned from net.ParseCIDR is ignored, I suspect because it is assumed to be properly validated by the API server. But here you don't. If there is an error here, are you sure you can derenference safely *v4Net later on?

correct the api would catch any invalid subnet configuration (or at least it should), just being cautious here to catch anything that i missed with my api validations. I am unsure if a value that could cause a panic would occur here because we already check for nil

Where do you check for nil?

we check that the gatewayConfig is not nil, so if no value is passed into the internalMasqueradeSubnet it should just be an empty string im pretty sure

So you have

_, v4MasqNet, err = net.ParseCIDR(oc.GatewayConfig.IPv4.InternalMasqueradeSubnet)

if that returns non-nil err and potentially nil v4MasqNet then you just append the error but continue on

out = append(out, errors.Errorf("v4InternalMasqueradeSubnet is invalid: %s", err)

and then a few lines below you dereference v4MasqNet which will panic if it is nil

if iputil.NetsOverlap(*v4MasqNet, *v4ClusterNet)

@jcaamano added a check to make sure that the v4/v6 masqNet return isnt nil

jcaamano · 2024-01-10T10:17:52Z

pkg/network/ovn_kubernetes.go

+				for _, sn := range conf.ServiceNetwork {
+					if utilnet.IsIPv4CIDRString(sn) {
+						_, v4ServiceNet, _ := net.ParseCIDR(sn)
+						if iputil.NetsOverlap(*v4Net, *v4ServiceNet) {


Should we check that there is no overlap with the join subnet as well? Perhaps it would be useful to have an utility method to pass a map of network names to networks, that just checks that there are no overlaps among any of them.

I think I would like to get this in before code freeze and then add some utility that allows us to check for overlap when we add the transit switch config this coming release, I can add a todo here if thats alright.

Regardless of the utility, don't you think we need to check for overlaps with the join subnet in this PR?

yes will add that now

@jcaamano added a check for just the join and masq subnets here, will add a utility when the transit swtich config is introduced here

bpickard22 · 2024-01-16T14:03:15Z

/retest-required

bpickard22 · 2024-01-16T19:01:10Z

/retest-required

jcaamano · 2024-01-17T09:56:39Z

pkg/network/ovn_kubernetes.go

+			if v4JoinNet == nil {
+				out = append(out, errors.Errorf("Unable to parse cidr for v4InternalSubnet %s", oc.V4InternalSubnet))
+				return out
+			}


If v4JoinNet == nil then err != nil so it doesn't make sense to check and append an error for both things.
Also this method is trying to validate all without breaking out early, I think we can keep doing that.

I would do this

_, v4JoinNet, err = net.ParseCIDR(oc.V4InternalSubnet) if err != nil { out = append(out, errors.Errorf("v4InternalSubnet is invalid: %s", err)) } else if !isV4InternalSubnetLargeEnough(conf) { out = append(out, errors.Errorf("v4InternalSubnet %s is not large enough for the maximum number of nodes which can be supported by ClusterNetwork", oc.V4InternalSubnet)) } ... ... if v4JoinNet != nil { _, v4ClusterNet, _ := net.ParseCIDR(cn.CIDR) if iputil.NetsOverlap(*v4JoinNet, *v4ClusterNet) { out = append(out, errors.Errorf("v4InternalSubnet %s overlaps with ClusterNetwork %s", oc.V4InternalSubnet, cn.CIDR)) } }

Similar approach on the other instances.

latest change implements this

You missed the second part, I added a comment for it

bpickard22 · 2024-01-18T14:18:02Z

/retest-required

jcaamano · 2024-01-18T14:44:45Z

pkg/network/ovn_kubernetes.go

+			if v4JoinNet != nil {
+				for _, cn := range conf.ClusterNetwork {
+					if utilnet.IsIPv4CIDRString(cn.CIDR) {
+						if oc.V4InternalSubnet != "" {


You should replace oc.V4InternalSubnet != "" with v4JoinNet != nil

Similar in other instances

I actually think that is a redundant if check that can just be removed

bpickard22 · 2024-01-18T16:32:07Z

/retest-required

Adds the necessary knobs in the cno to consume the api change exposing the internal masquerade subnet in ovn-k where we reserve addresses for ip masquerading Had to rebase on top of small conflict with addition of logging for libovsdb in the config yamls Also adds unit tests and verification in cno for configured masquerade subnets and corrects some tests/verification of the join subnet Signed-off-by: Ben Pickard <bpickard@redhat.com>

jcaamano · 2024-01-19T18:44:04Z

/lgtm
/approve

openshift-ci · 2024-01-19T18:45:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bpickard22, jcaamano, ricky-rav

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jcaamano]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-01-19T21:55:52Z

@bpickard22: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.15-upgrade-from-stable-4.14-images	`6a66258`	link	true	`/test 4.15-upgrade-from-stable-4.14-images`
ci/prow/4.15-upgrade-from-stable-4.14-e2e-gcp-ovn-upgrade	`6a66258`	link	false	`/test 4.15-upgrade-from-stable-4.14-e2e-gcp-ovn-upgrade`
ci/prow/4.15-upgrade-from-stable-4.14-e2e-azure-ovn-upgrade	`6a66258`	link	false	`/test 4.15-upgrade-from-stable-4.14-e2e-azure-ovn-upgrade`
ci/prow/4.15-upgrade-from-stable-4.14-e2e-aws-ovn-upgrade	`6a66258`	link	false	`/test 4.15-upgrade-from-stable-4.14-e2e-aws-ovn-upgrade`
ci/prow/e2e-azure-ovn-dualstack	`b8fe87e`	link	false	`/test e2e-azure-ovn-dualstack`
ci/prow/e2e-azure-ovn	`1b4d11d`	link	false	`/test e2e-azure-ovn`
ci/prow/e2e-openstack-ovn	`1b4d11d`	link	false	`/test e2e-openstack-ovn`
ci/prow/e2e-vsphere-ovn-dualstack-primaryv6	`1b4d11d`	link	false	`/test e2e-vsphere-ovn-dualstack-primaryv6`
ci/prow/4.16-upgrade-from-stable-4.15-e2e-azure-ovn-upgrade	`1b4d11d`	link	false	`/test 4.16-upgrade-from-stable-4.15-e2e-azure-ovn-upgrade`
ci/prow/e2e-aws-hypershift-ovn-kubevirt	`1b4d11d`	link	false	`/test e2e-aws-hypershift-ovn-kubevirt`
ci/prow/4.16-upgrade-from-stable-4.15-e2e-gcp-ovn-upgrade	`1b4d11d`	link	false	`/test 4.16-upgrade-from-stable-4.15-e2e-gcp-ovn-upgrade`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2024-01-22T17:06:33Z

@bpickard22: This pull request references SDN-3708 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Adds the necessary knobs in the cno to consume the api change (openshift/api#1410) exposing the internal masquerade subnet in ovn-k where we reserve addresses for ip masquerading

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-01-22T19:06:53Z

/retest-required

Remaining retests: 0 against base HEAD 2e23c82 and 2 for PR HEAD 1b4d11d in total

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2023

openshift-ci bot requested review from jcaamano and tssurya May 8, 2023 22:07

bpickard22 changed the title ~~[WIP}Add knobs in cno referencing api change~~ Support configurable masquerade subnet in ovn-k May 15, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2023

bpickard22 force-pushed the masq-subnet branch from aaa17d1 to 4687f42 Compare May 15, 2023 22:42

openshift-ci bot assigned tssurya May 16, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2023

bpickard22 force-pushed the masq-subnet branch from 4687f42 to b11506d Compare July 31, 2023 21:06

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2023

bpickard22 force-pushed the masq-subnet branch from b11506d to e415046 Compare August 3, 2023 19:10

flavio-fernandes reviewed Aug 8, 2023

View reviewed changes

ricky-rav suggested changes Aug 9, 2023

View reviewed changes

bpickard22 force-pushed the masq-subnet branch from e415046 to e4f831f Compare August 9, 2023 20:34

ricky-rav reviewed Aug 10, 2023

View reviewed changes

pkg/network/ovn_kubernetes.go Outdated Show resolved Hide resolved

ricky-rav reviewed Aug 10, 2023

View reviewed changes

pkg/network/ovn_kubernetes.go Outdated Show resolved Hide resolved

bpickard22 force-pushed the masq-subnet branch from e4f831f to 083aa79 Compare August 10, 2023 15:03

bpickard22 force-pushed the masq-subnet branch from 91357ac to 23b89e1 Compare August 30, 2023 01:27

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2023

bpickard22 force-pushed the masq-subnet branch from 23b89e1 to ba1ef9f Compare September 21, 2023 14:05

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2023

bpickard22 force-pushed the masq-subnet branch from ba1ef9f to 381fe25 Compare September 21, 2023 18:22

openshift-ci bot assigned ricky-rav Jan 3, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2024

jcaamano suggested changes Jan 10, 2024

View reviewed changes

bpickard22 force-pushed the masq-subnet branch from 99d8652 to ea17000 Compare January 10, 2024 20:54

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2024

bpickard22 force-pushed the masq-subnet branch 2 times, most recently from 0a0a0d4 to f8b2469 Compare January 15, 2024 22:03

bpickard22 force-pushed the masq-subnet branch 2 times, most recently from 3358439 to c6ef9f9 Compare January 16, 2024 14:33

jcaamano reviewed Jan 17, 2024

View reviewed changes

bpickard22 force-pushed the masq-subnet branch from c6ef9f9 to b89cf6e Compare January 17, 2024 22:47

jcaamano reviewed Jan 18, 2024

View reviewed changes

bpickard22 requested a review from ricky-rav January 18, 2024 19:38

bpickard22 force-pushed the masq-subnet branch from b89cf6e to 1b4d11d Compare January 19, 2024 17:41

openshift-ci bot assigned jcaamano Jan 19, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2024

bpickard22 changed the title ~~Support configurable masquerade subnet in ovn-k~~ SDN-3708: Support configurable masquerade subnet in ovn-k Jan 22, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 22, 2024

openshift-merge-bot bot merged commit dc94dc0 into openshift:master Jan 22, 2024
33 of 39 checks passed

SDN-3708: Support configurable masquerade subnet in ovn-k #1807

SDN-3708: Support configurable masquerade subnet in ovn-k #1807

Conversation

bpickard22 commented May 8, 2023 • edited

bpickard22 commented May 15, 2023

tssurya commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricky-rav commented Aug 9, 2023

ricky-rav 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

ricky-rav commented Aug 10, 2023

zshi-redhat commented Sep 20, 2023

jcaamano 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

bpickard22 commented Jan 16, 2024

bpickard22 commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpickard22 commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpickard22 commented Jan 18, 2024

jcaamano commented Jan 19, 2024

openshift-ci bot commented Jan 19, 2024

openshift-ci bot commented Jan 19, 2024

openshift-ci-robot commented Jan 22, 2024 • edited by openshift-ci bot

openshift-ci-robot commented Jan 22, 2024

bpickard22 commented May 8, 2023 •

edited

openshift-ci-robot commented Jan 22, 2024 •

edited by openshift-ci bot