Skip to content
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

Aspsk/fix dns resolution on kind #24713

Merged
merged 2 commits into from Apr 4, 2023
Merged

Conversation

aspsk
Copy link
Contributor

@aspsk aspsk commented Apr 3, 2023

Patch coredns configmap to bypass docker DNS resolver in kind and use 8.8.8.8. Re-enable masquerading and fast routing for the EGW test.

Fixes: #23283
Fixes: #23330

Always use the 8.8.8.8 DNS resolver in kind

@aspsk aspsk added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/ci This PR makes changes to the CI. labels Apr 3, 2023
@aspsk aspsk requested review from a team as code owners April 3, 2023 18:17
gentoo-root
gentoo-root previously approved these changes Apr 3, 2023
@aspsk aspsk force-pushed the aspsk/fix-dns-resolution-on-kind branch from e1a1486 to c30501d Compare April 3, 2023 19:06
@gentoo-root gentoo-root dismissed their stale review April 3, 2023 19:24

Code changed.

@aspsk aspsk force-pushed the aspsk/fix-dns-resolution-on-kind branch from c30501d to 1fe045f Compare April 3, 2023 19:57
@aspsk
Copy link
Contributor Author

aspsk commented Apr 3, 2023

/test

@aspsk aspsk force-pushed the aspsk/fix-dns-resolution-on-kind branch from 1fe045f to dce2efc Compare April 4, 2023 06:19
@aspsk
Copy link
Contributor Author

aspsk commented Apr 4, 2023

/test

@aspsk
Copy link
Contributor Author

aspsk commented Apr 4, 2023

The net-next fails the geneve/dsr testes, because geneve changes were merged to the master later than this PR:

Suite-k8s-1.26.K8sDatapathServicesTest Checks N/S loadbalancing Tests with XDP, direct routing, DSR with Geneve and Maglev
Suite-k8s-1.26.K8sDatapathServicesTest Checks N/S loadbalancing Tests with TC, direct routing and dsr with geneve
Suite-k8s-1.26.K8sDatapathServicesTest Checks N/S loadbalancing Tests with TC, geneve tunnel and dsr
Suite-k8s-1.26.K8sDatapathEgressGatewayTest tunnel disabled with endpointRoutes disabled, XDP and DSR with Geneve dispatch no egress gw policy connectivity works
Suite-k8s-1.26.K8sDatapathEgressGatewayTest tunnel disabled with endpointRoutes disabled, XDP and DSR with Geneve dispatch egress gw policy both egress gw and basic connectivity work
Suite-k8s-1.26.K8sDatapathEgressGatewayTest tunnel disabled with endpointRoutes disabled, XDP and DSR with Geneve dispatch egress gw policy with exclusion CIDRs Traffic is not SNATed with egress gateway IP

So removing the test commit, everything looks fine

@aspsk aspsk force-pushed the aspsk/fix-dns-resolution-on-kind branch 2 times, most recently from 5729751 to bc710b3 Compare April 4, 2023 11:24
We are using our Kind provisioning script to create K8s clusters when testing
in the CI. Recently, we discovered that on some kernels a default DNS resolver,
which is dockerd, is troublesome for the BPF host routing, which we want to
test in the CI (#23283).

Fix this by patching the coredns configmap after creating a kind cluster to
point to the 8.8.8.8 resolver. Alternative fixes (may still be applied later):

  * Pass a custom /etc/resolv.conf to kubelet via --resolv-conf in the Kind /
    kubeadm config.

  * Override /etc/resolv.conf of Kind nodes after creating a cluster (no race
    condition, as CoreDNS pods won't be started, as a CNI is not ready).

  * Patch Kind to allow users to specify custom DNS entries (i.e., docker run
    --dns="1.1.1.1,8.8.8.8").

Fixes: #23283
Fixes: #23330

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
The #23283 should have been fixed by the 3e8f697 ("contrib/kind: set custom
DNS resolver for Kind nodes") commit, so we can re-enable masquerading by
default and re-enable fast routing.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
@aspsk
Copy link
Contributor Author

aspsk commented Apr 4, 2023

CC: @brb @lmb

@aspsk
Copy link
Contributor Author

aspsk commented Apr 4, 2023

/test

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

# This is required because in case of BPF Host Routing we bypass iptables thus
# breaking DNS. See https://github.com/cilium/cilium/issues/23330
NewCoreFile=$(kubectl get cm -n kube-system coredns -o jsonpath='{.data.Corefile}' | sed 's,forward . /etc/resolv.conf,forward . 8.8.8.8,' | sed -z 's/\n/\\n/g')
kubectl patch configmap/coredns -n kube-system --type merge -p '{"data":{"Corefile": "'"$NewCoreFile"'"}}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this command synchronous? If not, is there a way to wait for the change to be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean exactly? I think that after this command is finished, the coredns CM will not expose the old value. The coredns or any other non-hostns pods are not running at this stage yet

@brb
Copy link
Member

brb commented Apr 4, 2023

Have you had a successful run with https://github.com/cilium/cilium/blob/master/.github/workflows/conformance-datapath.yaml#L23 uncommented?

@aspsk
Copy link
Contributor Author

aspsk commented Apr 4, 2023

Have you had a successful run with https://github.com/cilium/cilium/blob/master/.github/workflows/conformance-datapath.yaml#L23 uncommented?

Affirmative (the only thing which failed was net-next on geneve/dsr, as is listed above: #24713 (comment))

@brb brb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 4, 2023
@brb brb merged commit 79aa8b1 into master Apr 4, 2023
57 checks passed
@brb brb deleted the aspsk/fix-dns-resolution-on-kind branch April 4, 2023 14:06
@brb brb added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label May 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 12, 2023
@brb
Copy link
Member

brb commented May 12, 2023

Only bce8d9e needs to be backported to v1.13.

@brb brb mentioned this pull request May 12, 2023
2 tasks
@brb brb added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels May 12, 2023
@julianwiedmann julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. release-note/ci This PR makes changes to the CI. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
6 participants