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

Add and enable two egressAllowRules to ensure DNS access #3179

Merged
merged 2 commits into from Aug 2, 2023

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Aug 1, 2023

This chart ships with togglable predefined networking rules declared in NetworkPolicy resources that may or may not be enforced in a k8s cluster, and if they are enforced they are enforced by varying projects such as Calico or Cilium.

We have previously had a single rule (dnsPortsPrivateIPs) to ensure pods have access to a k8s cluster's DNS server, but it seems that this rule isn't enogh in two known situations. This PR is adding two rules (dnsPortsCloudMetadataServer and dnsPortsKubeSystemNamespace) to handle those situations.

The new rules are described in the configuration reference, screenshotted below.

image


This PR is based on insights gathered by @vizeit in #3167 and https://www.vizeit.com/jupyterhub-on-gke-autopilot/, thank you soo much @vizeit for digging into this!!!

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

This is a wonderful, nice investigation :) Thank you for deep diving into all of this. I like the defaults, and the reasoning too.

One thing I'd like added is a reference to dnsPortsCloudMetadataServer in the documentation for blockWithIptables, saying that even when it is true, DNS access is allowed and controlled by dnsPortsCloudMetadataServer. With that, this looks good to me.

@consideRatio
Copy link
Member Author

Thanks for reviewing @yuvipanda, and thanks again to @vizeit who did most of the digging that this is based on!

I agree with the idea on your docs update Yuvi! I didn't address it in this PR yet though @yuvipanda. I'd like to do it after we've decided on #3180, so I opened #3184 to ensure its gets addressed. Do you have thoughts on #3180?

@consideRatio
Copy link
Member Author

I'll merge this and open a PR to review with my suggested resolution to #3180 and #3184

@consideRatio consideRatio merged commit 1608658 into jupyterhub:main Aug 2, 2023
15 checks passed
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Aug 2, 2023
@vizeit
Copy link

vizeit commented Aug 2, 2023

Thanks for reviewing @yuvipanda, and thanks again to @vizeit who did most of the digging that this is based on!

I agree with the idea on your docs update Yuvi! I didn't address it in this PR yet though @yuvipanda. I'd like to do it after we've decided on #3180, so I opened #3184 to ensure its gets addressed. Do you have thoughts on #3180?

My pleasure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installation on GKE (standard & autopilot) fails with Dataplane V2 & kube-dns(Cilium)
3 participants