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
Bug 1903414: Do not use egressIP on reply packets #236
Bug 1903414: Do not use egressIP on reply packets #236
Conversation
@juanluisvaladas: This pull request references Bugzilla bug 1903414, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 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 kubernetes/test-infra repository. |
/Hold |
EgressIP namespaces should only force the egressIP when the pod is the client. If the pod is the server we want to reply normally.
4e34aea
to
4b3847f
Compare
/lgtm |
/hold cancel |
do we need an e2e to capture this bug? |
@@ -203,6 +203,7 @@ func (oc *ovsController) SetupOVS(clusterNetworkCIDR []string, serviceNetworkCID | |||
otx.AddFlow("table=100, priority=300,udp,udp_dst=%d,actions=drop", vxlanPort) | |||
otx.AddFlow("table=100, priority=200,tcp,tcp_dst=53,nw_dst=%s,actions=output:2", oc.localIP) | |||
otx.AddFlow("table=100, priority=200,udp,udp_dst=53,nw_dst=%s,actions=output:2", oc.localIP) | |||
otx.AddFlow("table=100, priority=150,ct_state=+rpl,actions=goto_table:101") |
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.
does this need +trk too?
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.
does this need +trk too?
no, if any other bit is set then trk
logically has to be set, so ct_state=+rpl+trk
would be the same as ct_state=+rpl
I am a bit unsure if this needs to get in to 4.7 so late in the cycle unless we have enough test coverage for egress IP as a feature. Alternatively, if we can get QE to validate that this doesn't break egress IP for openshift-sdn, we can get this in. But I would think at the very least we should have an e2e test to capture the bug itself. |
/bugzilla cc-qa |
Verified this PR, see steps in https://bugzilla.redhat.com/show_bug.cgi?id=1903414#c30. Did EgressIP regression testing, no issue found. Test run. https://polarion.engineering.redhat.com/polarion/#/project/OSE/testrun?id=OS-20210204-0117&tab=records&result=passed |
/lgtm |
@danwinship can you PTAL as well? |
@@ -203,6 +203,7 @@ func (oc *ovsController) SetupOVS(clusterNetworkCIDR []string, serviceNetworkCID | |||
otx.AddFlow("table=100, priority=300,udp,udp_dst=%d,actions=drop", vxlanPort) | |||
otx.AddFlow("table=100, priority=200,tcp,tcp_dst=53,nw_dst=%s,actions=output:2", oc.localIP) | |||
otx.AddFlow("table=100, priority=200,udp,udp_dst=53,nw_dst=%s,actions=output:2", oc.localIP) | |||
otx.AddFlow("table=100, priority=150,ct_state=+rpl,actions=goto_table:101") |
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.
does this need +trk too?
no, if any other bit is set then trk
logically has to be set, so ct_state=+rpl+trk
would be the same as ct_state=+rpl
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, huiran0826, JacobTanenbaum, juanluisvaladas 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:
Approvers can indicate their approval by writing |
@juanluisvaladas: All pull requests linked via external trackers have merged: Bugzilla bug 1903414 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
/bugzilla assign-qa |
@stevekuznetsov: Bugzilla bug 1903414 is in an unrecognized state (ON_QA) and will not be moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
/bugzilla assign-qe |
/cherry-pick release-4.6 |
@juanluisvaladas: new pull request created: #257 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 kubernetes/test-infra repository. |
Previoulsy [we added a flow](openshift#236) to send reply traffic to table 101(the ENP table) but in reality ENP should never match on repy traffic so instead just send it out. Signed-off-by: astoycos <astoycos@redhat.com>
EgressIP namespaces should only force the egressIP when the pod is the
client. If the pod is the server we want to reply normally.