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

Allow passing linodeApitoken and region as secretRef #134

Merged
merged 3 commits into from
Sep 1, 2023

Conversation

tchinmai7
Copy link
Contributor

General:

  • Have you removed all sensitive information, including but not limited to access keys and passwords?
  • Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

Adding support for passing the LinodeAPIToken and region via secret references instead of raw helm values. This prevents the disclosure of the token in values files when using gitops practices.

  1. Does your submission pass tests?
  2. Have you added tests?
  3. Are you addressing a single feature in this PR?
  4. Are your commits atomic, addressing one change per commit?
  5. Are you following the conventions of the language?
  6. Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. Have you explained your rationale for why this feature is needed?
  8. Have you linked your PR to an open issue

@@ -7,3 +8,4 @@ stringData:
apiToken: {{ required ".Values.apiToken required" .Values.apiToken }}
region: {{ required ".Values.region required" .Values.region }}
type: Opaque
{{- end }}
Copy link

Choose a reason for hiding this comment

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

nit: New line here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +7 to +12
# Set these values if your APIToken and region are already present in a k8s secret.
# secretRef:
# name: "linode-ccm"
# apiTokenRef: "apiToken"
# regionRef: "region"

Copy link

Choose a reason for hiding this comment

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

Question: Are we passing in the values when we install the helm chart? If yes, why is this part commented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, these values can be passed in when installing. This is commented to keep backwards compatibility. Existing users of the chart can continue to pass in the token and region, new users can choose to use the secretRef. Didn't want to break usersa

Copy link

@komer3 komer3 left a comment

Choose a reason for hiding this comment

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

LGTM!

@srust
Copy link
Contributor

srust commented Aug 30, 2023

@schinmai-akamai can you add a unit test for this?

@tchinmai7
Copy link
Contributor Author

Helm linting passes with the token and region set manually

$ helm lint deploy/chart --set apiToken="apiToken",region="us-east"      
==> Linting deploy/chart
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

Helm linting passes with the token and region set via secret

$ helm lint deploy/chart --set secretRef.apiTokenRef="apiToken",secretRef.name="api",secretRef.regionRef="us-east"                 
==> Linting deploy/chart
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

Helm template works with the secret being created when the apiToken and region is manually set

$  helm template foo deploy/chart --set apiToken="apiToken",region="us-east" --debug
---
# Source: ccm-linode/templates/ccm-linode.yaml
apiVersion: v1
kind: Secret
metadata:
  name: ccm-linode
  namespace: kube-system
stringData:
  apiToken: apiToken
  region: us-east
type: Opaque
---
# Source: ccm-linode/templates/daemonset.yaml
          env:
            - name: LINODE_API_TOKEN
              valueFrom:
                secretKeyRef:
                  name: "ccm-linode"
                  key: "apiToken"
            - name: LINODE_REGION
              valueFrom:
                secretKeyRef:
                  name: "ccm-linode"
                  key: "region"

Helm template works with secretReference - no secret, and correct values being passed.

$ helm template foo deploy/chart --set secretRef.apiTokenRef="apiToken",secretRef.name="api",secretRef.regionRef="us-east"
# Source: ccm-linode/templates/daemonset.yaml
          env:
            - name: LINODE_API_TOKEN
              valueFrom:
                secretKeyRef:
                  name: api
                  key: apiToken
            - name: LINODE_REGION
              valueFrom:
                secretKeyRef:
                  name: api
                  key: us-east

@srust srust merged commit aec305f into linode:master Sep 1, 2023
1 check passed
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.

3 participants