Support for DNS names in egress routes by pravisankar · Pull Request #15409 · openshift/origin

pravisankar · 2017-07-21T17:37:57Z

Introduced dns-proxy egress router mode that allows specifying DNS name for EGRESS_DESTINATION.
Currently, dns-proxy egress mode implementation is based on HAProxy.
HAProxy 1.6+ version is used to leverage DNS resolution at runtime.

Trello Card: https://trello.com/c/407uoUFz

pravisankar · 2017-07-21T17:44:03Z

@openshift/networking PTAL

pravisankar · 2017-07-21T17:44:08Z

[test]

openshift-bot · 2017-07-21T17:48:03Z

Evaluated for origin test up to caae214

openshift-bot · 2017-07-21T19:39:53Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3408/) (Base Commit: e97ba37) (PR Branch Commit: caae214)

pravisankar · 2017-07-27T02:34:49Z

/unassign kargakis
/assign danwinship

danwinship · 2017-08-01T16:17:48Z

images/egress/dns-proxy/Dockerfile

+    tar xvzf haproxy-1.7.5.tar.gz && \
+    groupadd haproxy && \
+    useradd -g haproxy haproxy && \
+    cd haproxy-* && make TARGET=linux2628 CPU=native USE_PCRE=1 USE_OPENSSL=1 USE_ZLIB=1 && make install && \


eek... Not sure about this. We don't do anything like that anywhere else...

What exactly is different if we don't have the new haproxy?

Dynamic DNS resolution feature is only available in HAProxy 1.6+ version (currently we are running haproxy 1.5). I don't know if this method is acceptable currently or not but we have done similar stuff in release-1.3 (https://github.com/openshift/origin/blob/release-1.3/images/router/haproxy/Dockerfile)

"Dynamic DNS resolution" meaning that if we used haproxy 1.5, it would only do the DNS resolution at startup and not notice any changes after that?

I feel like we need some sort of higher-up (ie, not just networking team) approval for actually building binaries from source in the images like this...

(Another possibility maybe is to pre-build the package into an RPM and make it available in a COPR, like we do for the openvswitch packages for the dind image.)

Yes, haproxy 1.5.x resolves DNS at startup and will fail if the IP changes.

@eparis @smarterclayton is this acceptable? (building haproxy from source)..if not, how should we proceed?
Having haproxy 1.6+ will also solve this card: https://trello.com/c/J1ODldZK/516-move-to-a-version-of-haproxy-with-lua-capability

danwinship · 2017-08-01T16:26:26Z

images/egress/dns-proxy/egress-dns-proxy.sh

+function generate_dns_resolvers() {
+    echo "resolvers dns-resolver"
+    # Fetch nameservers from /etc/resolv.conf
+    declare -a nameservers=$(cat /etc/resolv.conf |grep nameserver|awk -F" " '{print $2}')


origin style is to put declaration and initialization on separate lines. (IIRC because if you write it the way you did here, and there's an error, the error gets ignored)

also, you don't need cat and grep: $(awk '/nameserver/ {print $2}' /etc/resolv.conf)

also, actually you need a ^ before nameserver so it doesn't get tripped up by the comments NetworkManager sometimes adds:

# NOTE: the libc resolver may not support more than 3 nameservers. # The nameservers listed below may not be recognized.

danwinship · 2017-08-01T16:28:08Z

images/egress/dns-proxy/egress-dns-proxy.sh

+        echo "    nameserver ns$n ${ns}:${NS_PORT}"
+    done
+
+    # Add google DNS servers as fallback


oh no, don't do that. The user may not want to leak their DNS requests to google.

Ok, I will remove.

danwinship · 2017-08-01T16:33:53Z

images/egress/dns-proxy/egress-dns-proxy.sh

+    generate_haproxy_frontends_backends
+}
+
+function setup_haproxy_syslog() {


Ugh. There's no way to get it to just log to stdout/stderr?

We might want to make it not bother doing this unless the pod spec sets some sort of "debug" variable...

By default it is not logging stuff to stdout/stderr, I will enable this stuff when 'debug' variable is set.

danwinship · 2017-08-01T16:34:11Z

images/egress/dns-proxy/egress-dns-proxy.sh

+
+    check_prereqs
+
+    rm -f ${CONF}


"${CONF}" (everywhere)

danwinship · 2017-08-01T16:38:12Z

images/egress/router/egress-router.sh

+        die "Failed to fetch Pod IP"
+    fi
+
+    echo -A PREROUTING -p tcp -d "${pod_ip}" -j DNAT --to-destination "${EGRESS_SOURCE}"


Why do you need the pod_ip match here? We don't do that in the standard egress-router.

In the standard egress-router, iptables rules are set on the pod and packets from service IP to pod IP will hit the iptables on the pod that does the actual job.
In this dns proxy case, haproxy is running on the macvlan interface and packets from service IP will only reach the pod IP. This additional rule will forward all tcp packets from pod to macvlan interface which has the egress source IP.

Actually haproxy doesn't need to listen on macvlan interface. It can listen on pod/all interfaces and we don't need this additional iptables forward rule.

danwinship · 2017-08-01T16:39:42Z

images/egress/dns-proxy/egress_dns_proxy_test.go

+		}
+		out := string(outBytes)
+		for _, frontend := range test.frontends {
+			if !strings.Contains(out, frontend) {


This doesn't verify that there are no unexpected additional frontend/backend rules

I will add this test

Did you fix this? Basically I was suggesting you should parse the output into static config, resolvers, and frontends sections, so you could then just check for equality rather than just "contains". (This might be easier if you had multiple testing modes; eg, one that just runs generate_dns_resolvers and nothing else, and one that just runs generate_haproxy_frontends_backends and nothing else.)

We may have multiple frontends and backends and equality check will work if the order is predefined among the frontends/backends. We don't have to enforce the order when generating these frontends/backends, instead we are checking for exact number of frontends and backends expected. Lines 137 to 140 and 147 to 150 validates this case.

danwinship · 2017-08-01T16:41:11Z

images/egress/dns-proxy/egress_dns_proxy_test.go

+	}
+}
+
+func TestHAProxyDefaults(t *testing.T) {


I don't think this is really testing anything useful. OTOH, it might be good to test the resolver code

I was trying to catch the case where config file was overwritten by some write instead of append('>' instead of '>>' in the script). I will add the resolver test.

pravisankar · 2017-08-02T01:44:44Z

@danwinship updated, ptal

dcbw · 2017-08-04T18:33:34Z

@pravisankar maybe need ot rebase to get an update to contrib/completions/OWNERS? It's failing the verify there with FAILURE: Generated completions out of date. Please run hack/update-generated-completions.sh

danwinship · 2017-08-08T13:17:35Z

images/egress/dns-proxy/Dockerfile

+    tar xvzf haproxy-1.7.5.tar.gz && \
+    groupadd haproxy && \
+    useradd -g haproxy haproxy && \
+    cd haproxy-* && make TARGET=linux2628 CPU=native USE_PCRE=1 USE_OPENSSL=1 USE_ZLIB=1 && make install && \


"Dynamic DNS resolution" meaning that if we used haproxy 1.5, it would only do the DNS resolution at startup and not notice any changes after that?

danwinship · 2017-08-08T13:18:21Z

images/egress/dns-proxy/Dockerfile

+    tar xvzf haproxy-1.7.5.tar.gz && \
+    groupadd haproxy && \
+    useradd -g haproxy haproxy && \
+    cd haproxy-* && make TARGET=linux2628 CPU=native USE_PCRE=1 USE_OPENSSL=1 USE_ZLIB=1 && make install && \


I feel like we need some sort of higher-up (ie, not just networking team) approval for actually building binaries from source in the images like this...

(Another possibility maybe is to pre-build the package into an RPM and make it available in a COPR, like we do for the openvswitch packages for the dind image.)

danwinship · 2017-08-08T13:19:43Z

images/egress/dns-proxy/Dockerfile

+RUN INSTALL_PKGS="rsyslog gcc make openssl-devel pcre-devel tar wget socat" && \
+    yum install -y $INSTALL_PKGS && \
+    rpm -V $INSTALL_PKGS && \
+    wget http://www.haproxy.org/download/1.7/src/haproxy-1.7.5.tar.gz && \


if we are doing this, can we verify the md5sum as well before building?

danwinship · 2017-08-08T13:20:46Z

images/egress/dns-proxy/egress-dns-proxy.sh

+function generate_haproxy_defaults() {
+    echo "
+global
+    log         127.0.0.1 local2


will this cause problems if the debug logging isn't enabled?

Tested with and without debug logging and found no issues.

danwinship · 2017-08-08T13:29:19Z

images/egress/dns-proxy/egress_dns_proxy_test.go

+		}
+		out := string(outBytes)
+		for _, frontend := range test.frontends {
+			if !strings.Contains(out, frontend) {


Did you fix this? Basically I was suggesting you should parse the output into static config, resolvers, and frontends sections, so you could then just check for equality rather than just "contains". (This might be easier if you had multiple testing modes; eg, one that just runs generate_dns_resolvers and nothing else, and one that just runs generate_haproxy_frontends_backends and nothing else.)

ghyde · 2018-01-18T22:48:48Z

Now that the router has been updated to HAProxy 1.8 (#18053), can the egress router be updated to support dynamic DNS resolution?

Introduced dns-proxy egress router that allows specifying DNS name for EGRESS_DESTINATION. Currently, dns-proxy egress mode implementation is based on HAProxy. HAProxy 1.6+ version is used to leverage DNS resolution at runtime.

pravisankar · 2018-02-15T19:12:05Z

@openshift/sig-networking @danwinship @knobunc ready for re-review, PTAL

pravisankar · 2018-02-15T19:12:21Z

/retest

danwinship · 2018-02-19T15:32:38Z

/lgtm

openshift-ci-robot · 2018-02-19T15:32:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, pravisankar

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~hack/lib/OWNERS~~ [danwinship]
~~images/egress/OWNERS~~ [danwinship,pravisankar]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

eparis · 2018-02-19T15:49:07Z

/hold
looks like a 3.10 thing...

pravisankar · 2018-03-01T23:23:42Z

/hold cancel
master is open for 3.10

openshift-merge-robot · 2018-03-01T23:25:42Z

/test all [submit-queue is verifying that this PR is safe to merge]

pravisankar · 2018-03-02T00:50:54Z

/retest

openshift-merge-robot · 2018-03-02T01:02:14Z

Automatic merge from submit-queue (batch tested with PRs 15409, 18763).

openshift-ci-robot · 2018-03-02T01:32:56Z

@pravisankar: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_templates	`9e4afb1`	link	`/test extended_templates`
ci/openshift-jenkins/extended_conformance_install_update	`9e4afb1`	link	`/test extended_conformance_install_update`
ci/openshift-jenkins/extended_networking_minimal	`5a94143`	link	`/test extended_networking_minimal`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

pravisankar added the component/networking label Jul 21, 2017

pravisankar force-pushed the egress-haproxy-mode branch from 021c3ea to caae214 Compare July 21, 2017 17:42

openshift-merge-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 24, 2017

openshift-merge-robot assigned 0xmichalis and bparees Jul 25, 2017

bparees removed their assignment Jul 25, 2017

pravisankar mentioned this pull request Jul 27, 2017

Docs: Egress router DNS proxy mode openshift/openshift-docs#4854

Merged

openshift-ci-robot assigned danwinship and unassigned 0xmichalis Jul 27, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2017

pravisankar force-pushed the egress-haproxy-mode branch from caae214 to 77c1008 Compare July 28, 2017 23:05

openshift deleted a comment from openshift-merge-robot Jul 29, 2017

danwinship suggested changes Aug 1, 2017

View reviewed changes

pravisankar force-pushed the egress-haproxy-mode branch 3 times, most recently from 39825e5 to 6cdaa67 Compare August 2, 2017 01:27

openshift-merge-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 2, 2017

pravisankar mentioned this pull request Aug 2, 2017

Docs: Updates to Egress router DNS proxy mode openshift/openshift-docs#4910

Merged

pravisankar force-pushed the egress-haproxy-mode branch from 6cdaa67 to 9e4afb1 Compare August 7, 2017 18:52

danwinship suggested changes Aug 8, 2017

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2017

Ravi Sankar Penta added 3 commits February 13, 2018 22:20

Support for DNS names in egress routes

cb3ce18

Introduced dns-proxy egress router that allows specifying DNS name for EGRESS_DESTINATION. Currently, dns-proxy egress mode implementation is based on HAProxy. HAProxy 1.6+ version is used to leverage DNS resolution at runtime.

Egress router setup for dns proxy mode

786b263

Added test cases for dns proxy egress router mode

5a94143

pravisankar force-pushed the egress-haproxy-mode branch from 9e4afb1 to 5a94143 Compare February 14, 2018 06:38

openshift deleted a comment from openshift-merge-robot Feb 14, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2018

pravisankar requested a review from knobunc February 15, 2018 19:08

openshift-ci-robot added the sig/networking label Feb 15, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2018

eparis added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Feb 19, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2018

openshift-merge-robot merged commit 65dc8e1 into openshift:master Mar 2, 2018

alebedev87 mentioned this pull request May 17, 2023

OCPBUGS-11385: Removed chroot setting openshift/images#137

Merged

Conversation

pravisankar commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pravisankar commented Jul 21, 2017

Uh oh!

pravisankar commented Jul 21, 2017

Uh oh!

openshift-bot commented Jul 21, 2017

Uh oh!

openshift-bot commented Jul 21, 2017

Uh oh!

pravisankar commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pravisankar commented Aug 2, 2017

Uh oh!

dcbw commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghyde commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pravisankar commented Feb 15, 2018

Uh oh!

pravisankar commented Feb 15, 2018

Uh oh!

danwinship commented Feb 19, 2018

pravisankar commented Jul 21, 2017 •

edited

Loading

ghyde commented Jan 18, 2018 •

edited

Loading

eparis commented Feb 19, 2018 •

edited

Loading

openshift-ci-robot commented Mar 2, 2018 •

edited

Loading