-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Egress router & local network #12460
Conversation
@@ -21,8 +21,9 @@ if ! ip route get ${EGRESS_DESTINATION} | grep -q macvlan0; then | |||
ip addr add ${EGRESS_SOURCE}/32 dev macvlan0 | |||
ip link set up dev macvlan0 | |||
ip route add ${EGRESS_GATEWAY}/32 dev macvlan0 | |||
ip route add ${EGRESS_DESTINATION}/32 via ${EGRESS_GATEWAY} dev macvlan0 | |||
|
|||
if [ ! "${EGRESS_DESTINATION}" == "${EGRESS_GATEWAY}" ]; then |
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 the !=
syntax work with [
? If so, it may read better:
if [ "${EGRESS_DESTINATION}" != "${EGRESS_GATEWAY}" ]; then
ip route add ${EGRESS_DESTINATION}/32 via ${EGRESS_GATEWAY} dev macvlan0 | ||
|
||
if [ ! "${EGRESS_DESTINATION}" == "${EGRESS_GATEWAY}" ]; then | ||
ip route add ${EGRESS_DESTINATION}/32 via ${EGRESS_GATEWAY} dev macvlan0 |
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.
ip route add "${EGRESS_DESTINATION}/32" via "${EGRESS_GATEWAY}" dev macvlan0
^ ^ ^ ^
Yes fine. I am also adding parameter escaping to the original code. It is not a big thing here as there is no risk of privilege escalation: the user giving the parameters needs the rights for creating the pod. |
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.
LGTM, thanks for the patch
[test] |
Evaluated for origin test up to c96e21f |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12895/) (Base Commit: 283a990) |
1 similar comment
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12895/) (Base Commit: 283a990) |
@danwinship please can you double check to see if this makes sense. |
So... you're using the egress router to talk to a server on the same subnet as your OpenShift cluster? |
Yes, it may be a bit of an edge case, mostly in test environments where all servers are in a single subnet and IP address white listing is used for instance with MySQL. |
OK, I guess that makes sense. Please squash the two commits together though, and rebase it to master rather than having a merge commit |
…tination addresses are the same
[merge] |
Evaluated for origin merge up to 4a23c38 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12943/) (Base Commit: f136dff) (Image: devenv-rhel7_5699) |
Origin Action Required: Please contact #openshift-dev to have this pull request manually reviewed and tested |
Adding a test for not having a route added if the gateway and the destination addresses are the same. Confer issue #12248