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

Add Helm Chart variables to mount the Custom Azure Environment File #451

Merged
merged 1 commit into from Apr 6, 2021

Conversation

chrisamert
Copy link
Contributor

To get the Azure KeyVault Provider working in air-gapped or on-prem Azure clouds, the Custom Azure Environment file needs to be mounted on the KeyVault Provider pods. This change adds new variables to the Helm chart that allows this Volume Mount to be created.

Fixes #444

Reason for Change:

Enables the Helm Chart to configure a volume mount necessary for running the Azure KeyVault Provider in air-gapped & on-prem Azure instances.

Requirements

  • squashed commits
  • included documentation
  • added unit tests and e2e tests (if applicable).

Issue Fixed:

Fixes #444

Does this change contain code from or inspired by another project?

  • Yes
  • No

If "Yes," did you notify that project's maintainers and provide attribution?

Special Notes for Reviewers:

Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @chrisamert. This change is specific to the cloud env file name. I think instead if we allow mounting arbitrary volumes it'll work for the cloud env file and also for any other required files in the future. WDYT?

Essentially supporting custom volumeMounts and volumes.

@chrisamert
Copy link
Contributor Author

Thank you for the PR @chrisamert. This change is specific to the cloud env file name. I think instead if we allow mounting arbitrary volumes it'll work for the cloud env file and also for any other required files in the future. WDYT?

Essentially supporting custom volumeMounts and volumes.

@aramase The cloudEnv is the only scenario I know of at the moment that requires a custom volume mount, so I was hesitant to make it a general solution. It also didn't seem like something that was likely to be needed in the future; I don't picture the Pod needing access to host files (beyond the one it mounts itself) a very common scenario. My thought is to leave it specific to the cloud env file as the intent is more clear. I'll leave the ultimate decision up to you all though, if you'd rather see this as a general solution supporting any custom 'volumes' and 'volumeMounts', I can certaintly update this PR with that change.

@chrisamert chrisamert force-pushed the CustomAzureEnvironmentHelmChart branch from 25491d2 to bd59c13 Compare April 2, 2021 18:34
@chrisamert
Copy link
Contributor Author

Thank you for the PR @chrisamert. This change is specific to the cloud env file name. I think instead if we allow mounting arbitrary volumes it'll work for the cloud env file and also for any other required files in the future. WDYT?

Essentially supporting custom volumeMounts and volumes.

@aramase I've gone ahead and implemented the custom 'volume' and 'volumeMounts' here. Let me know what you think, I'm fine with either approach

Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

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

@chrisamert The changes look good. Could you rebase the PR?

To get the Azure KeyVault Provider working in air-gapped or on-prem Azure clouds, the Custom Azure Environment file needs to be mounted on the KeyVault Provider pods.  This change adds new variables to the Helm chart that allows this Volume Mount to be created.

Fixes Azure#444
@chrisamert chrisamert force-pushed the CustomAzureEnvironmentHelmChart branch from bd59c13 to 27e7f7c Compare April 5, 2021 19:50
@chrisamert
Copy link
Contributor Author

Done!

@aramase aramase requested a review from sozercan April 5, 2021 19:55
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@aramase aramase merged commit 32c3d68 into Azure:master Apr 6, 2021
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.

Helm Chart should allow specifying a custom volume mount to support Custom Azure Environments
3 participants