-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-40906: Implement IPsec NAT-Traversal encapsulation option #2573
base: master
Are you sure you want to change the base?
Conversation
@pperiyasamy: This pull request references Jira Issue OCPBUGS-40906, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
@pperiyasamy: This pull request references Jira Issue OCPBUGS-40906, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
0aa71c5
to
89bed2c
Compare
89bed2c
to
9e70026
Compare
9e70026
to
ff55dc6
Compare
@pperiyasamy: This pull request references Jira Issue OCPBUGS-40906, which is invalid:
Comment In response to this:
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. |
/jira refresh |
@pperiyasamy: This pull request references Jira Issue OCPBUGS-40906, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
/retest |
/assign @jcaamano @trozet @huiran0826 |
/testwith openshift/origin#29232 |
/testwith openshift/cluster-network-operator/master/e2e-ovn-ipsec-step-registry openshift/origin#29232 |
/testwith openshift/cluster-network-operator/master/e2e-ovn-ipsec-step-registry openshift/origin#29232 |
/assign @huiran0826 |
/assign @martinkennelly |
/testwith openshift/cluster-network-operator/master/e2e-aws-ovn-ipsec-upgrade openshift/machine-config-operator#4854 |
ff55dc6
to
3211ebf
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pperiyasamy The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3211ebf
to
c8e357f
Compare
dependent PR isnt merged [1] do you need it? openshift/machine-config-operator#4797 |
yes @martinkennelly , we need openshift/machine-config-operator#4797 to be merged. @yuqi-zhang is aware of this already. but still this PR is ready for review now. |
This is the first time happening to me but the first commit keeps crashing and freezing on my chrome browser because its so gigantic because of the vendor imports. I normally like having full func commits but I ask you to split the vendoring out into a seperate commit so i can actually review |
1. Update with: go get -u github.com/openshift/build-machinery-go go get -u k8s.io/api go get -u k8s.io/apimachinery go get -u k8s.io/code-generator go get -u k8s.io/component-base go get -u k8s.io/klog/v2 go get -u k8s.io/kube-proxy go get -u k8s.io/utils go get -u sigs.k8s.io/controller-runtime go get -u github.com/openshift/api go get -u github.com/openshift/client-go go get -u github.com/openshift/library-go go get -u github.com/openshift/machine-config-operator go get -u k8s.io/apiextensions-apiserver go get -u k8s.io/client-go go get -u sigs.k8s.io/controller-tools go get -u github.com/openshift/api 2. Update go.mod file with following: replace ( github.com/google/cel-go => github.com/google/cel-go v0.22.0 github.com/openshift/machine-config-operator => github.com/RishabhSaini/machine-config-operator v0.0.1-0.20250130013234-f25892323bb7 ) 3. go mod tidy go mod vendor 4. Adapt CNO code with dependency updates. This commit skips vendor directory changes because it is too big, makes chrome to hang and difficult to review the actual changes. Signed-off-by: Periyasamy Palanisamy <[email protected]>
Signed-off-by: Periyasamy Palanisamy <[email protected]>
There is a requirement to encapsulate IPsec east west traffic in UDP via NAT-T so that those packets are compatible with intermediate NAT device(s) if present. This commit consumes new API to enable encap option and applies to OVN to configure east west ipsec tunnel connections accordingly. Signed-off-by: Periyasamy Palanisamy <[email protected]>
c8e357f
to
2355043
Compare
|
||
replace ( | ||
github.com/google/cel-go => github.com/google/cel-go v0.22.0 | ||
github.com/openshift/machine-config-operator => github.com/RishabhSaini/machine-config-operator v0.0.1-0.20250130013234-f25892323bb7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note t self; make sure this is removed in final review
@@ -332,6 +332,11 @@ data: | |||
ipsec_encapsulation=true | |||
fi | |||
{{ end }} | |||
|
|||
{{ if eq .OVNIPsecEncap "Always" }} | |||
ipsec_encapsulation=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isnt it possible this key-value is defined twice if on ibm platform and IPSec Full option set to Always?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt this be within the OVNIPsecEnable if block? I know your programming is fine here but it reads weird and id like checking OVNIPsecEncap
eqs Always to be inside the OVNIPsecEnable
bool.
@@ -332,6 +332,11 @@ data: | |||
ipsec_encapsulation=true | |||
fi | |||
{{ end }} | |||
|
|||
{{ if eq .OVNIPsecEncap "Always" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n00b question but this Key could be unset so what happens in that scenario - did you check?
do we've an e2e somewhere for the new Full modes? |
@pperiyasamy: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
There is a requirement to encapsulate IPsec east west traffic in UDP via NAT-T so that those packets are compatible with intermediate NAT device(s) if present. This PR consumes new API to enable or disable encap option and applies to
OVN to configure east west ipsec tunnel connections accordingly.
API PRs:
openshift/api#1472
openshift/api#2199
Depends on openshift/machine-config-operator#4797 for
openshift/api
bump (which needed kube v0.32.2 update).