-
Notifications
You must be signed in to change notification settings - Fork 123
charts: adding manual tls support #2845
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
base: main
Are you sure you want to change the base?
Conversation
This is just adding manual tls support as we have today in kustomize. But I find redundant a bit, and could be improved later. Signed-off-by: Beraldo Leal <[email protected]>
| # | ||
| # 1. Provide the certificate files below (create mode) or via an external | ||
| # secret (reference mode, see existingTlsSecretName) | ||
| # 2. Set CACERT_FILE, CERT_FILE, CERT_KEY in providerConfigs pointing to |
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.
Hi @beraldoleal ,
is this correct? I only see CACERT_FILE referenced in 84b1ab8#diff-68532edec3310fc4be4fa3c3b66f08eec86a9cf01767fdd193ec6016f7589153R54 and only to check if the hasTlsCerts
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.
Not sure if I got your question.. it seems truncated... what could be wrong?
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.
Never mind, I understand it.
While in here, would be possible to set good defaults:
CACERT_FILE="/etc/certificates/ca.crt"
CERT_FILE="/etc/certificates/client.crt"
CERT_KEY="/etc/certificates/client.key"
So that users could simply uncomment the values in src/cloud-api-adaptor/install/charts/peerpods/providers/<provider>.yaml?
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.
The default here is a real default in the Go binary, not just a hint in the YAML. If we change it to /etc/certificates/ca.crt, the auto-generated provider file would show:
# CA cert file
# (default: "/etc/certificates/ca.crt")
# CACERT_FILE: "/etc/certificates/ca.crt"
But even with the line still commented out in the chart, the Go binary itself uses that hardcoded default when the env var is not set (which is expected). So CAA would assume "the user wants custom certs at /etc/certificates/ca.crt" and try to load them, even though nobody uploaded any certs. This would break the default flow, which is automatic in-memory TLS generation.
If we change the default value we are changing the current default behaviour, and if we just add any value here in the template only (somehow), this would confuse users on what is the actual default.
Instead, I think we could just improve the help message to give users a clue of what path to use when they do want custom certs. Does that makes sense?
This is just adding manual tls support as we have today in kustomize. But I find redundant a bit, and could be improved later.