-
Notifications
You must be signed in to change notification settings - Fork 101
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
Documented Azure DNS deployment #1525
Conversation
…DNS service. Signed-off-by: Nuno Guedes <[email protected]>
Signed-off-by: Nuno Guedes <[email protected]>
✅ Deploy Preview for k8gb-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infbase thank you so much for the contribution! Overall, it looks great, my main concern is that the Azure clientSecret
is getting exposed as plain text. Can we find a way to avoid it? Happy to collaborate on this topic and try to find a solution together.
"aadClientId": "{{ .Values.azuredns.aadClientId }}", | ||
{{- end -}} | ||
{{- if .Values.azuredns.aadClientSecret -}} | ||
"aadClientSecret": "{{ .Values.azuredns.aadClientSecret }}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean we will have the client secret as plain text in the values.yaml
? We probably should keep it in the Secret
@@ -0,0 +1,9 @@ | |||
{{- if and .Values.azuredns.enabled .Values.azuredns.createAuthSecret }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably should create this Secret with Credentials outside of the helm chart and point to it from the values.yaml
for k8gb to pick up
}, | ||
"aadClientSecret": { | ||
"type": "string" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As pointed out above, I have concerns about propagating aadClientId
and aadClientSecret
as plaintext values. Can we keep them in some input Secret? I appreciate that it might be tricky as we construct the external-dns Secret data in external-dns.azure-credentials
template function.
@infbase as discussed in community , let's provide ability to reference existing secret for more secure scenario |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Yury Tsarev <[email protected]>
Updated Helm chart, docs and examples for connecting to Azure Public DNS service.
Users should now be able to connect external-dns to an existing Azure Public DNS zone by setting the correct helm values, as demonstrated in the example deployment added to the docs in this PR.
Related to issue #642