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

feat: additional env vars for ratify container via helm chart #1854

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

mannbiher
Copy link
Contributor

@mannbiher mannbiher commented Oct 7, 2024

Description

What this PR does / why we need it:

Allow passing additional environment variables to ratify container via helm chart. E.g. when ratify requires proxy configuration to download plugin, user can pass the proxy environment variables via helm chart.

# values.yaml
env:
  - name: https_proxy
    value: http://proxy-server:80

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Fixes #1853

Type of change

Please delete options that are not relevant.

  • Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • Helm template with environment values set
cd charts/ratify
helm template ratify . --set env[0].name="https_proxy" --set env[0].value="http://proxy:80" --set featureFlags.RATIFY_CERT_ROTATION=true
  • Helm template without environment values set
charts/ratify
helm template ratify .  --set featureFlags.RATIFY_CERT_ROTATION=true

Checklist:

  • Does the affected code have corresponding tests?
  • Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have appropriate license header?

Post Merge Requirements

  • MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

@akashsinghal
Copy link
Collaborator

@mannbiher thanks for opening up the PR. At yesterday's PR review meeting (10/8/24), we discussed the PR and had a few questions:

  • Could you provide some more details on how the http_proxy env variable is used by Ratify? Once the env variable is set for the proxy, how is it actually consumed by Ratify?
  • If we do allow for arbitrary env variables to be set via the chart, what is the behavior when a key provided matches an existing ratify-defined env variable? Does K8s fail to install? What does the failure handling look like?
  • Regarding the chart version bump, the project only increments chart version prior to a release. Is there urgency for this feature to be released from your side? Can we include this in the next minor version release slated for December?

@mannbiher
Copy link
Contributor Author

@akashsinghal

Could you provide some more details on how the http_proxy env variable is used by Ratify? Once the env variable is set for the proxy, how is it actually consumed by Ratify?

http_proxy, https_proxy and no_proxy environment variables are respected by go http client, so ratify automatically supports it. I am currently using these variables to use ratify behind a proxy.

If we do allow for arbitrary env variables to be set via the chart, what is the behavior when a key provided matches an existing ratify-defined env variable? Does K8s fail to install? What does the failure handling look like?

I tested it today.

helm template ratify .  --set "env[0].name=RATIFY_CERT_ROTATION" --set-string "env[0].value=0" --set featureFlags.RATIFY_CERT_ROTATION=true

Generated manifest

env:
            - name: RATIFY_CERT_ROTATION
              value: "0"
            - name: RATIFY_CERT_ROTATION
              value: "1"
            - name: RATIFY_EXPERIMENTAL_HIGH_AVAILABILITY
              value: "0"

Installation works and the pod spec shows same variable defined two times. However the later value takes effect inside pod. The helm chart change has the env block first, so it should not impact other environment variables.

Environment variables inside pod.

bash-5.2$ cat /proc/1/environ|tr '\0' '\n'|grep RATIFY_
RATIFY_CONFIG=/.ratify/
RATIFY_EXPERIMENTAL_HIGH_AVAILABILITY=0
RATIFY_CERT_ROTATION=1

Regarding the chart version bump, the project only increments chart version prior to a release. Is there urgency for this feature to be released from your side? Can we include this in the next minor version release slated for December?

I would like to have the helm chart change released earlier. But we can wait. I could maintain a local copy of helm chart.

charts/ratify/Chart.yaml Outdated Show resolved Hide resolved
@akashsinghal
Copy link
Collaborator

@mannbiher would it be possible for you to update the README.md file in the chart with a description of the new value added? https://github.com/ratify-project/ratify/tree/dev/charts/ratify#parameters. We can then merge this PR.

@mannbiher
Copy link
Contributor Author

@mannbiher would it be possible for you to update the README.md file in the chart with a description of the new value added? https://github.com/ratify-project/ratify/tree/dev/charts/ratify#parameters. We can then merge this PR.

Done.

@mannbiher mannbiher force-pushed the env-via-helm branch 2 times, most recently from 0fa5821 to 722cb3c Compare October 16, 2024 03:19
Copy link
Collaborator

@akashsinghal akashsinghal left a comment

Choose a reason for hiding this comment

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

Lgtm thanks for the PR!

Copy link
Collaborator

@binbin-li binbin-li left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the change!

@binbin-li binbin-li merged commit 85781cc into ratify-project:dev Oct 16, 2024
20 checks passed
@mannbiher mannbiher deleted the env-via-helm branch October 16, 2024 13:48
duffney pushed a commit to duffney/ratify that referenced this pull request Oct 22, 2024
binbin-li pushed a commit to binbin-li/ratify that referenced this pull request Oct 23, 2024
@junczhu junczhu mentioned this pull request Oct 23, 2024
12 tasks
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.

Allow passing additional environment variables to ratify container via helm chart
4 participants