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

Fail in Helm template if dnsMode=public is combined with a baseDomain ending with .internal #218

Merged
merged 2 commits into from
Feb 22, 2023

Conversation

AndiDog
Copy link
Contributor

@AndiDog AndiDog commented Feb 15, 2023

Found while testing giantswarm/roadmap#1832 for private clusters

What this PR does / why we need it

Related chat

I tested on an MC with base domain gaws.gigantic.internal, but had set dnsMode to public. Since .internal and other TLDs are reserved for private use, such records will not propagate to public DNS servers. In my case, the workload cluster then failed to come up because Cilium could not look up api.<baseDomain> via DNS. With this change, we throw an error early instead of allowing this human mistake.

λ helm template helm/cluster-aws/ --set organization=bam --set baseDomain=gaws.bla.internal --set network.dnsMode=private
---
# Source: cluster-aws/templates/list.yaml
apiVersion: v1
[...]

# Bad
λ helm template helm/cluster-aws/ --set organization=bam --set baseDomain=gaws.bla.internal --set network.dnsMode=public
Error: execution error at (cluster-aws/templates/list.yaml:7:4): dnsMode=public cannot be combined with a '*.internal' baseDomain since reserved-as-private TLDs are not propagated to public DNS servers and therefore crucial DNS records such as api.<baseDomain> cannot be looked up

Checklist

  • Update changelog in CHANGELOG.md.

Trigger e2e tests

/test create
/test upgrade

@AndiDog AndiDog requested a review from a team February 15, 2023 18:13
@AndiDog
Copy link
Contributor Author

AndiDog commented Feb 16, 2023

/retest

@tityosbot
Copy link

@AndiDog: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
create 2aaff99 link /test create
upgrade 2aaff99 link /test upgrade

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

1 similar comment
@tityosbot
Copy link

@AndiDog: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
create 2aaff99 link /test create
upgrade 2aaff99 link /test upgrade

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@AndiDog
Copy link
Contributor Author

AndiDog commented Feb 16, 2023

Tekton pipelines probably won't work right now because grizzly is down. Guess we can review anyhow.

@@ -185,7 +185,8 @@
"type": "string"
},
"dnsMode": {
"type": "string"
"type": "string",
"enum": ["public", "private"]
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@AndiDog AndiDog merged commit dc0cf5c into master Feb 22, 2023
@AndiDog AndiDog deleted the dns-public-mode-conflict branch February 22, 2023 09:43
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.

4 participants