feat(fulcio/add-env): Add additional env variables #530

saisatishkarra · 2023-05-18T21:25:47Z

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 version bumped in Chart.yaml according to semver. Where applicable, update and bump the versions in any associated umbrella chart
Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
JSON Schema generated.
List tests pass for Chart using the Chart Testing tool and the ct lint command.

Support GCP credentials for external cloud provider workloads Signed-off-by: saisatish karra <[email protected]>

charts/fulcio/values.yaml

Signed-off-by: saisatish karra <[email protected]>

charts/fulcio/README.md

sabre1041 · 2023-05-20T00:21:24Z

charts/fulcio/values.yaml

  args:
    port: 5555
    grpcPort: 5554
+    # valid values: GCP workload identity config json for trusted external cloud providers
+    creds: ""


the name creds is somewhat ambitious

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?

I am good with cloud_credential_config

sabre1041 · 2023-05-20T00:30:05Z

charts/fulcio/templates/fulcio-deployment.yaml

@@ -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 }}


This doesnt have a closing end tag, yet strangely no error is thrown

The closing {{- end}} for line 64 is at line82

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

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.

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]>

saisatishkarra · 2023-05-22T17:50:06Z

@sabre1041 Fixed the issues. Please review and revert.

sabre1041 · 2023-05-25T05:16:24Z

charts/fulcio/Chart.yaml

@@ -5,7 +5,7 @@ description: |

 type: application

-version: 2.3.2
+version: 2.4.2


Suggested change

version: 2.4.2

version: 2.4.0

sabre1041 · 2023-05-25T05:21:44Z

charts/fulcio/templates/fulcio-deployment.yaml

@@ -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 }}


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

sabre1041 · 2023-05-25T05:22:09Z

charts/fulcio/values.yaml

  args:
    port: 5555
    grpcPort: 5554
+    # valid values: GCP workload identity config json for trusted external cloud providers
+    creds: ""


I am good with cloud_credential_config

sabre1041 · 2023-05-25T05:22:33Z

charts/fulcio/templates/fulcio-configmap.yaml

@@ -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 }}


Add a conditional to avoid including when not specified

Added condition!

Signed-off-by: saisatish karra <[email protected]>

Sifungurux · 2024-06-27T12:51:59Z

Hello @saisatishkarra
I really need the feature that enabled to set env as well. Are the feature gone cold

Racer159 · 2024-07-19T17:18:15Z

Just to add - not having the extra environment variables in the fulcio chart also makes configuring a Vault/OpenBao KMS more difficult since you cannot easily set env vars like VAULT_ADDR - https://github.com/sigstore/sigstore/blob/1bbe33108312349ecb12badff611f23c262c0760/pkg/signature/kms/hashivault/client.go#L101 (you'd have to patch it after the fact unless I am missing something)

saisatishkarra force-pushed the feat/fulcio-env-gcp-creds branch from 9bbd178 to 2c3fe7e Compare May 18, 2023 21:31

feat(fulcio/add-env): Add additional env variables

af3fc29

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 May 18, 2023 21:41

cpanato reviewed May 19, 2023

View reviewed changes

charts/fulcio/values.yaml Outdated Show resolved Hide resolved

saisatishkarra force-pushed the feat/fulcio-env-gcp-creds branch 3 times, most recently from bcf4c93 to 066dec4 Compare May 19, 2023 14:06

saisatishkarra requested a review from cpanato May 19, 2023 14:06

fix(helm): make env variable optional

bef4aba

Signed-off-by: saisatish karra <[email protected]>

saisatishkarra force-pushed the feat/fulcio-env-gcp-creds branch from 066dec4 to bef4aba Compare May 19, 2023 14:07

sabre1041 requested changes May 20, 2023

View reviewed changes

modify env variable type

a03c22a

Signed-off-by: saisatish karra <[email protected]>

saisatishkarra requested a review from sabre1041 May 21, 2023 19:36

sabre1041 requested changes May 25, 2023

View reviewed changes

improve naming scheme for extrernal credential config

1012833

Signed-off-by: saisatish karra <[email protected]>

saisatishkarra requested a review from sabre1041 May 25, 2023 13:35

Racer159 mentioned this pull request Jul 24, 2024

add additional env variables to fulcio chart #790

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fulcio/add-env): Add additional env variables #530

feat(fulcio/add-env): Add additional env variables #530

saisatishkarra commented May 18, 2023 •

edited

Loading

sabre1041 May 20, 2023

saisatishkarra May 21, 2023 •

edited

Loading

sabre1041 May 25, 2023

saisatishkarra May 25, 2023

sabre1041 May 20, 2023

saisatishkarra May 21, 2023

sabre1041 May 25, 2023

saisatishkarra May 25, 2023

sabre1041 May 25, 2023

saisatishkarra commented May 22, 2023

sabre1041 May 25, 2023

saisatishkarra May 25, 2023

sabre1041 May 25, 2023

sabre1041 May 25, 2023

sabre1041 May 25, 2023

saisatishkarra May 25, 2023

Sifungurux commented Jun 27, 2024

Racer159 commented Jul 19, 2024

feat(fulcio/add-env): Add additional env variables #530

Are you sure you want to change the base?

feat(fulcio/add-env): Add additional env variables #530

Conversation

saisatishkarra commented May 18, 2023 • edited Loading

Description of the change

Existing or Associated Issue(s)

Additional Information

Checklist

Choose a reason for hiding this comment

saisatishkarra May 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saisatishkarra commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sifungurux commented Jun 27, 2024

Racer159 commented Jul 19, 2024

saisatishkarra commented May 18, 2023 •

edited

Loading

saisatishkarra May 21, 2023 •

edited

Loading