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

Expose CoreDNS over tcp + make Dig in terratests go via tcp #845

Merged
merged 4 commits into from
Mar 25, 2022

Conversation

jkremser
Copy link
Member

@jkremser jkremser commented Feb 3, 2022

why: When we get rid of Docker Desktop and use for instance Lima, it doesn't have support for UDP port forwarding from host to containers running in the VM - lima-vm/lima#366

So as a workaround, we can run the Dig in our tests using the tcp protocol, where the redirect works fine (it uses ssh tunnel).

tests will fail, it assumes a new release on AbsaOSS/gopkg with this AbsaOSS/gopkg#11 merged in

It adds a new service for coredns that uses NodePort (30053) and then adds this redirect in k3d manifests so that

dig @127.0.0.1 -p5053 failover.cloud.example.com +short +tcp
dig @127.0.0.1 -p5054 failover.cloud.example.com +short +tcp

both work

todo: update docs for local playground

that AbsaOSS/gopkg library contains a module called strings, that was renamed to string like a year ago, but only now it's failing because of the newly released version. Hence the strings -> string thing.

btw:

DNS resolvers and recursive servers MUST support UDP, and SHOULD support TCP, for sending (non-zone-transfer) queries.
--- https://datatracker.ietf.org/doc/html/rfc7766

Signed-off-by: Jirka Kremser [email protected]

@jkremser jkremser force-pushed the lima-udp-workaround branch 2 times, most recently from 090d939 to 67f99de Compare February 8, 2022 15:49
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

Aren't we making here a strong assumption that k8gb contributor should use mac+lima?

@k0da
Copy link
Collaborator

k0da commented Feb 17, 2022

@ytsarev it is more about to use tcp as it also works on mac+lima

ports:
- name: tcp-5353
port: 53
protocol: TCP
Copy link
Member

Choose a reason for hiding this comment

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

Was it tested outside of the local scenario? Main concern is overall TCP support/expectation in the DNS environment

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, only in with terratests. There is a conditional at the very top of this file that doesn't deploy this svc for aws environment. btw. this doesn't make the switch from udp to tcp, this adds tcp protocol as the other option and instruct the tests that run in the local environment to go via tcp

Copy link
Member

Choose a reason for hiding this comment

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

cool if udp is not getting disable then we are safe I guess

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jkremser could you please move this service definition out of helm and deploy it during test setup creation (around we deploy edgeDNS)

Copy link
Member Author

Choose a reason for hiding this comment

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

it should be there

@jkremser jkremser requested a review from ytsarev March 21, 2022 15:45
somaritane
somaritane previously approved these changes Mar 22, 2022
Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

lgtm

kuritka
kuritka previously approved these changes Mar 22, 2022
ports:
- name: tcp-5353
port: 53
protocol: TCP
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jkremser could you please move this service definition out of helm and deploy it during test setup creation (around we deploy edgeDNS)

@jkremser jkremser dismissed stale reviews from kuritka and somaritane via a2ab77b March 24, 2022 17:36
@jkremser jkremser force-pushed the lima-udp-workaround branch from a2ab77b to 5acb831 Compare March 24, 2022 17:38
@jkremser jkremser requested review from kuritka, somaritane and k0da March 24, 2022 17:39
@k0da
Copy link
Collaborator

k0da commented Mar 24, 2022 via email

@jkremser jkremser force-pushed the lima-udp-workaround branch from 5acb831 to 1fb222f Compare March 25, 2022 11:42
@jkremser
Copy link
Member Author

This way it will be deployed into default namespace

good point, added k8gb ns

k0da
k0da previously approved these changes Mar 25, 2022
Copy link
Collaborator

@k0da k0da left a comment

Choose a reason for hiding this comment

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

LGTM

@jkremser jkremser requested a review from k0da March 25, 2022 14:22
@jkremser jkremser merged commit c958dea into k8gb-io:master Mar 25, 2022
@jkremser jkremser deleted the lima-udp-workaround branch March 25, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants