feat(fulcio/add-env): Add additional env variables #530
Conversation
saisatishkarra
force-pushed
the
feat/fulcio-env-gcp-creds
branch
from
9bbd178
to
2c3fe7e
Compare
Support GCP credentials for external cloud provider workloads Signed-off-by: saisatish karra <[email protected]>
saisatishkarra
force-pushed
the
feat/fulcio-env-gcp-creds
branch
from
2c3fe7e
to
af3fc29
Compare
saisatishkarra
force-pushed
the
feat/fulcio-env-gcp-creds
branch
3 times, most recently
from
bcf4c93
to
066dec4
Compare
Signed-off-by: saisatish karra <[email protected]>
saisatishkarra
force-pushed
the
feat/fulcio-env-gcp-creds
branch
from
066dec4
to
bef4aba
Compare
charts/fulcio/values.yaml
Outdated
| args: | ||
| port: 5555 | ||
| grpcPort: 5554 | ||
| # valid values: GCP workload identity config json for trusted external cloud providers | ||
| creds: "" |
There was a problem hiding this comment.
the name creds is somewhat ambitious
There was a problem hiding this comment.
Is there an expected naming suggestion? I used this to maintain the existing convention between TSA and Fulcio helm chart. How about cloud_credential_config?
There was a problem hiding this comment.
I am good with cloud_credential_config
| @@ -59,14 +59,27 @@ spec: | |||
| - "--kms-cert-chain-path=/etc/fulcio-config/chain.pem" | |||
| {{- end }} | |||
| - "--ct-log-url={{ if .Values.server.args.disable_ct_log }}{{ else if .Values.server.args.ct_log_url }}{{ .Values.server.args.ct_log_url }}{{ else }}http://{{ .Values.ctlog.name }}.{{ .Values.ctlog.namespace.name }}.svc/{{ .Values.ctlog.createctconfig.logPrefix }}{{ end }}" | |||
| {{- if eq .Values.server.args.certificateAuthority "fileca" }} | |||
| {{- if .Values.server.env }} | |||
There was a problem hiding this comment.
This doesnt have a closing end tag, yet strangely no error is thrown
There was a problem hiding this comment.
The closing {{- end}} for line 64 is at line82
There was a problem hiding this comment.
Got it
To simplify, this condition should be an or statement that checks the presence of Values.server.env or whether Values.server.args.certificateAuthority == fileca. This would avoid the redundant PASSWORD env variable definition
There was a problem hiding this comment.
I don't think the or condition between Values.server.env and Values.server.args.certificateAuthority == fileca would work because there might be a case where there are env variables specified as key-value pairs and the certificateAuthority == "<something not fileca>" at which point it would still try to populate the PASSWORD as a secret ref that is NOT optional and the pod might fail / retry due to lack of missing secret.
There was a problem hiding this comment.
that is still fine. having the proposed conditional would still suffice. this conditional would remain in order to capture when it was defined, otherwise only the key/values would be captured. The contents within this conditional block can be removed as its no longer needed
Signed-off-by: saisatish karra <[email protected]>
|
@sabre1041 Fixed the issues. Please review and revert. |
charts/fulcio/Chart.yaml
Outdated
| @@ -5,7 +5,7 @@ description: | | |||
|
|
|||
| type: application | |||
|
|
|||
| version: 2.3.2 | |||
| version: 2.4.2 | |||
There was a problem hiding this comment.
| version: 2.4.2 | |
| version: 2.4.0 |
| @@ -59,14 +59,27 @@ spec: | |||
| - "--kms-cert-chain-path=/etc/fulcio-config/chain.pem" | |||
| {{- end }} | |||
| - "--ct-log-url={{ if .Values.server.args.disable_ct_log }}{{ else if .Values.server.args.ct_log_url }}{{ .Values.server.args.ct_log_url }}{{ else }}http://{{ .Values.ctlog.name }}.{{ .Values.ctlog.namespace.name }}.svc/{{ .Values.ctlog.createctconfig.logPrefix }}{{ end }}" | |||
| {{- if eq .Values.server.args.certificateAuthority "fileca" }} | |||
| {{- if .Values.server.env }} | |||
There was a problem hiding this comment.
Got it
To simplify, this condition should be an or statement that checks the presence of Values.server.env or whether Values.server.args.certificateAuthority == fileca. This would avoid the redundant PASSWORD env variable definition
charts/fulcio/values.yaml
Outdated
| args: | ||
| port: 5555 | ||
| grpcPort: 5554 | ||
| # valid values: GCP workload identity config json for trusted external cloud providers | ||
| creds: "" |
There was a problem hiding this comment.
I am good with cloud_credential_config
| @@ -11,3 +11,4 @@ data: | |||
| {{- if (eq .Values.server.args.certificateAuthority "kmsca")}} | |||
| chain.pem: {{.Values.server.args.kms_cert_chain | quote }} | |||
| {{- end }} | |||
| cloud_credentials: {{.Values.server.args.creds | quote }} | |||
There was a problem hiding this comment.
Add a conditional to avoid including when not specified
There was a problem hiding this comment.
Added condition!
Signed-off-by: saisatish karra <[email protected]>
|
Hello @saisatishkarra |
Description of the change
Support GCP credentials for external cloud provider workloads
Existing or Associated Issue(s)
Additional Information
Similar changes that were made in TSA
Checklist
Chart.yamlaccording to semver. Where applicable, update and bump the versions in any associated umbrella chartvalues.yamland added to the README.md. The helm-docs utility can be used to generate the necessary content. Usehelm-docs --dry-runto preview the content.ct lintcommand.