daemon: Start DNS proxy before k8s with restored rules #12964

jrajahalme · 2020-08-25T12:10:40Z

Note: This is a forward-port of #12718 and #12731, which were developed on v1.7 branch.

Split the endpoint restoration process into more stages so that the saved endpoint DNS history can be used earlier in the bootstrap to start the DNS proxy. This helps ensure that the first regeneration of an Endpoint does not (temporarily) remove the policy keys needed to keep old traffic flowing.

TPROXY rules for the DNS proxy can only be installed couple of seconds later in the bootstrap process. To mitigate this and allow the DNS proxy start serving traffic sooner, it now reads the old TPROXY rule, if available, and tries to re-use the same proxy port number as before the restart. This allows the old datapath config to start feeding traffic to the new proxy instance even before the datapath config can be rewritten.

DNS proxy starts with restored DNS rules based on allowed IP addresses. These rules are removed for each endpoint as soon as the first regeneration completes. Such restored rules should allow DNS requests to be served, but for new DNS resolutions to be added to the Endpoint's policy the restored endpoint's must still have their first regeneration completed.

Fixes: #11004

DNS Proxy is started earlier in the Cilium agent bootstrap to make it available to running endpoints sooner.

jrajahalme · 2020-08-25T12:43:30Z

test-me-please

jrajahalme · 2020-08-25T13:02:37Z

test-me-please

joestringer

I stuck to my codeowners for this review but also we reviewed in detail for the original submission so I don't think it's such a big deal.

There's just one note below which accounts for a change in master branch since this PR was proposed. We'll need to co-ordinate on followup for that condition to prevent regression.

joestringer · 2020-09-02T19:51:00Z

pkg/datapath/iptables/iptables.go

+func (m *IptablesManager) GetProxyPort(name string) uint16 {
+	prog := "iptables"
+	if !option.Config.EnableIPv4 {
+		prog = "ip6tables"
+	}
+
+	res, err := runProgCombinedOutput(prog, []string{"-t", "mangle", "-n", "-L", ciliumPreMangleChain}, false)
+	if err != nil {
+		return 0
+	}
+
+	re := regexp.MustCompile(name + ".*TPROXY redirect 0.0.0.0:([1-9][0-9]*) mark")
+	str := re.FindString(string(res))
+	portStr := re.ReplaceAllString(str, "$1")
+	portInt, err := strconv.Atoi(portStr)
+	if err != nil {
+		log.WithError(err).Debugf("Port number cannot be parsed: %s", portStr)
+		return 0
+	}
+	return uint16(portInt)
+}


Hmm, this should be good for v1.8 but for latest with bpf tproxy it will not work.

Probably makes sense to merge the rest of this PR as-is and have a separate commit to address the tproxy case. Not sure whether latest code will pass with this broken (maybe it'll pass if we don't have an actual runtime test for this condition..)

@joestringer Any hints how to do this with the new bpf tproxy? Is there a header file left over by the previous run we could parse instead?

AFAIK the port is only placed into BPF policy maps for each endpoint, which isn't the most reliable way to store/restore proxy ports for all protocols.

I think we could consider writing each proxy port to one of the config files, purely for restore purposes.

I've filed #13208 to follow up.

joestringer · 2020-09-02T19:52:45Z

CI tests were green but I'm not sure whether net-next will pass when rebased against master. Will re-kick CI for that job.

joestringer · 2020-09-02T19:52:52Z

retest-net-next

pchaigno

Is there any way we could we test this in the e2e test? E.g., by testing connectivity earlier in Restart Cilium validate that FQDN is still working? Not sure how we would detect that proxy should be ready and it's okay to test connectivity now.

pkg/fqdn/dnsproxy/proxy.go

Start proxy support earlier in the daemon bootstrap, notably before any k8s setup. Fetch old endpoints earlier so that the DNS history is avalailable before k8s is set up and move dns proxy initialization earlier in the bootstrap. Reuse DNS proxy port from previous run on restart unless overridden by an explicit Cilium agent option These changes allow the DNS proxy start serving requests as soon as the toFQDN policy is received from k8s and avoid any service disruption prevously possible due to endpoints being regenerated before the DNS proxy was started. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Store current DNS rules with the Endpoint and use them in the DNS proxy during initial regeneration of the restrored endpoints. DNS proxy starts with restored DNS rules based on allowed IP addresses. These rules are removed for each endpoint as soon as the first regeneration completes. Such restored rules should allow DNS requests to be served, but for new DNS resolutions to be added to the Endpoint's policy the restored endpoint's must still have their first regeneration completed. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Update DNSRules, if any, before writing headers to capture potentially changed allowed destination IPs. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Remove restored DNS rules after a successful regeneration, and also at endpoint delete to cover endpoints that were never regenerated. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Normally the number of DNS proxy rules should be very small. To guard against pathological cases, limit the number of IPs processed to 1000 per port. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Use restored Endpoints during Cilium restart when Endpoints are no yet available. Do not error out if the destination IP can not be found from ipcache, but default to WORLD destination security identity instead. This allows IP-based restored rules to be processed before ipcache is fully updated. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

"Proxy stats not found when updating" warnigns are currently issued if stats updates are received for a proxy redirect that can not be found. There are two common scenarios where this can happen as part of normal operation: 1. A policy change removed a proxy redirect, and stats updates from requests that were redirected to the proxy before the datapath redirect entry was removed are received. 2. DNS proxy issues stats for requests that have been forwarded on the basis of a restored DNS policy while the Endpoint policy has not yet been computed. Demote this log message to debug level to avoid these false warnings. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

removeRestoredRules() requires locking, better reflect that in the function name. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Re-run connectivity test while Cilium is still restarting. This should succeed as the same DNS names were used in a connectivity test before the restart. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2020-09-17T08:02:05Z

test-me-please

jrajahalme added area/daemon Impacts operation of the Cilium daemon. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. needs-backport/1.8 labels Aug 25, 2020

jrajahalme requested review from a team August 25, 2020 12:10

jrajahalme requested review from a team as code owners August 25, 2020 12:10

jrajahalme force-pushed the pr/jrajahalme/tune-dns-proxy-bootstrap branch 3 times, most recently from 9075612 to a129374 Compare August 25, 2020 12:42

jrajahalme force-pushed the pr/jrajahalme/tune-dns-proxy-bootstrap branch from a129374 to a40e957 Compare August 25, 2020 12:59

joestringer approved these changes Sep 2, 2020

View reviewed changes

joestringer added the needs/e2e-test This issue is not covered by existing CI tests, but should be. label Sep 2, 2020

aanm approved these changes Sep 3, 2020

View reviewed changes

pchaigno approved these changes Sep 11, 2020

View reviewed changes

pkg/fqdn/dnsproxy/proxy.go Outdated Show resolved Hide resolved

jrajahalme added 7 commits September 17, 2020 10:53

endpoint: Update DNSRules on header rewrite

e96ab91

Update DNSRules, if any, before writing headers to capture potentially changed allowed destination IPs. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

endpoint: Remove restored DNS rules

73417a9

Remove restored DNS rules after a successful regeneration, and also at endpoint delete to cover endpoints that were never regenerated. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

fqdn: Limit the max processing in GetRules()

f677470

Normally the number of DNS proxy rules should be very small. To guard against pathological cases, limit the number of IPs processed to 1000 per port. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme force-pushed the pr/jrajahalme/tune-dns-proxy-bootstrap branch from a40e957 to 7c41d5c Compare September 17, 2020 07:54

jrajahalme requested a review from a team as a code owner September 17, 2020 07:54

jrajahalme added 2 commits September 17, 2020 10:58

fqdn: rename removeRestoredRules() as removeRestoredRulesLocked()

e60089f

removeRestoredRules() requires locking, better reflect that in the function name. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

test: Test DNS proxying while Cilium is restarting.

782cc71

Re-run connectivity test while Cilium is still restarting. This should succeed as the same DNS names were used in a connectivity test before the restart. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme force-pushed the pr/jrajahalme/tune-dns-proxy-bootstrap branch 2 times, most recently from 266aede to 782cc71 Compare September 17, 2020 08:01

pchaigno approved these changes Sep 17, 2020

View reviewed changes

aditighag approved these changes Sep 17, 2020

View reviewed changes

joestringer mentioned this pull request Sep 17, 2020

Store and restore proxy ports across agent restarts via state headers #13208

Open

aanm merged commit 6173e1c into master Sep 17, 2020

aanm deleted the pr/jrajahalme/tune-dns-proxy-bootstrap branch September 17, 2020 20:28

jrajahalme added backport-pending/1.8 and removed needs-backport/1.8 labels Sep 21, 2020

jrajahalme mentioned this pull request Sep 21, 2020

v1.8 backports 2020-09-21 #13226

Merged

rolinh added backport-done/1.8 and removed backport-pending/1.8 labels Sep 29, 2020

joestringer mentioned this pull request Oct 1, 2020

Prepare for release v1.8.4 #13361

Merged

pchaigno mentioned this pull request Feb 17, 2021

Cilium latest error: iptables: No chain/target/match by that name #15002

Closed

christarazi mentioned this pull request Mar 13, 2024

fqdn: Add Protocol to DNS Proxy Cache #31328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

daemon: Start DNS proxy before k8s with restored rules #12964

daemon: Start DNS proxy before k8s with restored rules #12964

Uh oh!

jrajahalme commented Aug 25, 2020 •

edited

Loading

Uh oh!

jrajahalme commented Aug 25, 2020

Uh oh!

jrajahalme commented Aug 25, 2020

Uh oh!

joestringer left a comment

Uh oh!

joestringer Sep 2, 2020 •

edited

Loading

Uh oh!

jrajahalme Sep 9, 2020

Uh oh!

joestringer Sep 9, 2020

Uh oh!

joestringer Sep 17, 2020

Uh oh!

joestringer commented Sep 2, 2020

Uh oh!

joestringer commented Sep 2, 2020

Uh oh!

pchaigno left a comment

Uh oh!

Uh oh!

jrajahalme commented Sep 17, 2020

Uh oh!

Uh oh!

daemon: Start DNS proxy before k8s with restored rules #12964

daemon: Start DNS proxy before k8s with restored rules #12964

Uh oh!

Conversation

jrajahalme commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrajahalme commented Aug 25, 2020

Uh oh!

jrajahalme commented Aug 25, 2020

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrajahalme Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

joestringer Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

joestringer Sep 17, 2020

Choose a reason for hiding this comment

Uh oh!

joestringer commented Sep 2, 2020

Uh oh!

joestringer commented Sep 2, 2020

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrajahalme commented Sep 17, 2020

Uh oh!

Uh oh!

jrajahalme commented Aug 25, 2020 •

edited

Loading

joestringer Sep 2, 2020 •

edited

Loading