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 override of secretsprovider using $PULUMI_SECRETS_PROVIDER #10749

Merged

Conversation

sfc-gh-alytvynov
Copy link
Contributor

@sfc-gh-alytvynov sfc-gh-alytvynov commented Sep 15, 2022

Description

Allow per-execution override of the secrets provider via an environment variable. This allows a temporary replacement without updating the stack config, such a during CI.

Fixes # (issue)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@sfc-gh-alytvynov sfc-gh-alytvynov force-pushed the alytvynov/secretsprovider-env-override branch from 45d59c0 to 425524b Compare September 15, 2022 18:48
@sfc-gh-alytvynov
Copy link
Contributor Author

I don't see a CHANGELOG_PENDING.md in the repo anymore. Should I add an entry in https://github.com/pulumi/pulumi/tree/master/changelog/pending?

@AaronFriel
Copy link
Member

Thanks for the contribution @sfc-gh-alytvynov! I haven't had time yet to read & review, but I wanted to reply to your question about changelogs to unblock a merge. It looks like you've got the right thing there, and if you've recently rebased, you can run make changelog to create an entry as well.

I'll make a note to update our changelog preview job to run on pull request targets as well!

Copy link
Member

@AaronFriel AaronFriel left a comment

Choose a reason for hiding this comment

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

One change requested, otherwise looks good to me.

pkg/cmd/pulumi/crypto_cloud_test.go Outdated Show resolved Hide resolved
Allow per-execution override of the secrets provider via an environment
variable. This allows a temporary replacement without updating the stack
config, such a during CI.
@sfc-gh-alytvynov sfc-gh-alytvynov force-pushed the alytvynov/secretsprovider-env-override branch from 78e847c to 57ad16c Compare September 16, 2022 22:46
@pulumi-bot
Copy link
Contributor

pulumi-bot commented Sep 16, 2022

Changelog

[uncommitted] (2022-09-22)

@sfc-gh-alytvynov
Copy link
Contributor Author

@AaronFriel thanks, updated!
That changelog preview doesn't look right though, is my changelog file correctly formatted?

@AaronFriel
Copy link
Member

/run-acceptance-tests

@Frassle
Copy link
Member

Frassle commented Sep 19, 2022

This allows a temporary replacement without updating the stack config, such a during CI.

This all looks very unsafe to me.
Secret management is already a per-stack option, but generally if your changing it you need to change it properly (i.e. decrypt and re-encrypt any stack config).

If this is to just override the URL used for cloud secret operations (i.e. kms) then I think it would be better named as something like PULUMI_CLOUD_SECRET_OVERRIDE. If this is is supposed to be used to set the default choice for secret manager for new stacks then the current code has changed the wrong place.

@sfc-gh-alytvynov
Copy link
Contributor Author

@Frassle good points!
We do have a rather unorthodox setup with different Hashicorp Vaults (dev, prod and CI). Our regular keys are in dev/prod vaults used by the CD system, but our CI system can only access the separate CI vault due to network isolation. In order to run pulumi preview from CI, we have to replicate the keys to CI vault.
At first glance, setting VAULT_ADDR would suffice, but the key paths end up changing when replicated from dev/prod to CI vault. We really only need to override the key path here.

I hope that's enough context for the change. I'm happy to rename the env var if that's the only concern.

@Frassle
Copy link
Member

Frassle commented Sep 19, 2022

I'm happy to rename the env var if that's the only concern.

I'd be fine with the change with a renamed envvar. PULUMI_SECRET_PROVIDER sounds like it's setting the overall secret provider, while what this is doing is overriding the url for cloud based secret providers. If a stack was using the pulumi sass or the passphrase secret system this envvar wouldn't affect them at all.

PULUMI_CLOUD_SECRET_OVERRIDE is my suggestion, but I'm happy with anything of that shape.

@sfc-gh-alytvynov
Copy link
Contributor Author

Sounds good, switched env var to PULUMI_CLOUD_SECRET_OVERRIDE

@AaronFriel
Copy link
Member

@sfc-gh-alytvynov just a heads up, I don't think you're doing anything wrong with the changelogs. We recently made a large set of changes to our CI system and need to fix up some of the workflows (#10790).

@sfc-gh-alytvynov
Copy link
Contributor Author

@Frassle PTAL :)

Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

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

Small changelog fix but otherwise lgtm

Co-authored-by: Fraser Waters <frassle@gmail.com>
@Frassle
Copy link
Member

Frassle commented Sep 21, 2022

bors merge

bors bot added a commit that referenced this pull request Sep 21, 2022
10749: Allow override of secretsprovider using $PULUMI_SECRETS_PROVIDER r=Frassle a=sfc-gh-alytvynov

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

Allow per-execution override of the secrets provider via an environment variable. This allows a temporary replacement without updating the stack config, such a during CI.

Fixes # (issue)

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [x] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [x] I have updated the [CHANGELOG-PENDING](https://github.com/pulumi/pulumi/blob/master/CHANGELOG_PENDING.md) file with my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Service,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


10809: Re-enable prior release process's checksums r=AaronFriel a=AaronFriel

Re-enables creating a `pulumi-${version}-checksum.txt` on release.

The s3 blob upload will upload this to S3 as it's configured to upload any artifact in the release prefixed with `pulumi-`:

https://github.com/pulumi/pulumi/blob/bce637624e00b610c213f4f60fe1dd17aa8513b2/.github/workflows/release.yml#L130-L137

Resolves #10807

Co-authored-by: Andrew Lytvynov <andrew.lytvynov@snowflake.com>
Co-authored-by: Aaron Friel <mayreply@aaronfriel.com>
@bors
Copy link
Contributor

bors bot commented Sep 21, 2022

Build failed (retrying...):

bors bot added a commit that referenced this pull request Sep 21, 2022
10749: Allow override of secretsprovider using $PULUMI_SECRETS_PROVIDER r=Frassle a=sfc-gh-alytvynov

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

Allow per-execution override of the secrets provider via an environment variable. This allows a temporary replacement without updating the stack config, such a during CI.

Fixes # (issue)

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [x] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [x] I have updated the [CHANGELOG-PENDING](https://github.com/pulumi/pulumi/blob/master/CHANGELOG_PENDING.md) file with my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Service,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


Co-authored-by: Andrew Lytvynov <andrew.lytvynov@snowflake.com>
@bors
Copy link
Contributor

bors bot commented Sep 21, 2022

Build failed:

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@AaronFriel
Copy link
Member

AaronFriel commented Sep 22, 2022

/run-acceptance-tests
Please view the results of the acceptance tests Here

@AaronFriel
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Sep 22, 2022

Build succeeded:

@bors bors bot merged commit 20e32f8 into pulumi:master Sep 22, 2022
@aaronkao
Copy link

aaronkao commented Oct 6, 2022

@sfc-gh-alytvynov hey Andrew, can you contact me at kao at pulumi.com? I want to send you some special contributor swag to thank you for this contribution.

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.

None yet

5 participants