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

fix(connectivity): add dns rules and change protocol to ANY #1402

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

eminaktas
Copy link
Contributor

Connectivity tests fail with the node-local-dns environment. This PR adds missing DNS rules and changes protocol from UDP to ANY

@eminaktas eminaktas requested a review from a team as a code owner February 17, 2023 16:02
@eminaktas eminaktas temporarily deployed to ci February 17, 2023 16:02 — with GitHub Actions Inactive
@ldelossa
Copy link
Contributor

/test

Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

See the inline comment.

Comment on lines 22 to 24
- ports:
- port: "53"
protocol: ANY
Copy link
Member

Choose a reason for hiding this comment

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

Why are we opening the policy rules for all protocols if the intention is to only allow DNS traffic? Did you try adding a similar rule for TCP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for asking. I should have mentioned it already. I was searching for the root cause. I did a test with TCP it didn't affect the result. The problem is that the policies with FQDNS do not work. Of course, I am talking about the environment with the node-local-dns It looks like it cannot detect the protocol when only the client-egress-to-fqdns-one-one-one-one or the client-egress-only-dns policies are applied.

I added the DNS rule (the part below) for the above policies. But the problem wasn't that something was missing.

      rules:
        dns:
        - matchPattern: "*"

Then, I noticed that Cilium couldn't validate the protocols.

We might add a test environment with node-local-dns deployed. I can help with it.

I noticed this problem while I was testing the new release of Cilium.

Copy link
Member

Choose a reason for hiding this comment

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

Worth noting that if this is set to ANY and there is a rules stanza, Cilium only supports redirecting traffic for TCP and UDP so I believe this is effectively the same as having another rule for TCP specifically.

@joestringer joestringer added the needs-rebase This PR needs to be rebased because it has merge conflicts. label May 5, 2023
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Considering the Cilium policy behaviour for how it handles the node local DNS IP and the way that these manifests are currently working around the issue, I think that this proposal is accurate.

Switching from UDP to ANY is also effectively an alias for TCP and UDP anyway, and DNS can commonly go over both UDP and TCP.

@eminaktas eminaktas requested a review from a team as a code owner October 25, 2023 07:47
@maintainer-s-little-helper
Copy link

Commit 7f1a3fd does not match "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

Signed-off-by: Emin Aktaş <eminaktas34@gmail.com>
Signed-off-by: Emin Aktas <eminaktas34@gmail.com>
@eminaktas eminaktas temporarily deployed to ci October 25, 2023 07:55 — with GitHub Actions Inactive
@tklauser tklauser removed the needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 25, 2023
@aditighag
Copy link
Member

Sorry for the delay.

@aditighag
Copy link
Member

Test failures -

AKS: no-policies/pod-to-world/https-to-bing.com-1: cilium-test/client2-5c6c769648-7zd4n (10.0.1.164) -> bing.com-https (bing.com:443)

We have similar flakes every now and then where tests involve connecting to an external entity.

I've restarted the external workloads test as there were some infrastructure failures in the earlier run.

@squeed squeed removed the request for review from ldelossa November 8, 2023 12:10
@squeed
Copy link
Contributor

squeed commented Nov 8, 2023

This flaked on #2070 and a 1.1.1.1 blip -- marking ready to merge.

@squeed squeed added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 8, 2023
@squeed squeed merged commit 6a29b6b into cilium:main Nov 8, 2023
17 of 19 checks passed
@eminaktas eminaktas deleted the connectivity-fix branch November 10, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants