-
Notifications
You must be signed in to change notification settings - Fork 172
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 support for multiple secrets to Pulumi shell plugin #288
Conversation
@@ -18,7 +17,25 @@ func PulumiCLI() schema.Executable { | |||
), | |||
Uses: []schema.CredentialUsage{ | |||
{ | |||
Name: credname.PersonalAccessToken, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this Personal Access Token is for authenticating to Pulumi Cloud, instead of deleting it from the usages, you could consider:
-
creating a separate
CredentialUsage
in this usages slice, with theDescription: Credentials for authenticating to Pulumi Cloud
, and using theName: credname.PersonalAccessToken,
pattern you used before, andNeedsAuth: needsauth.ForCommand("login")
-
Creating a second plugin for better isolation between definitions called
PulumiCloud
or something similar, and that can define its ownpulumi
executable similar to the one from the other plugin, and all the rules of defining the credential usage from 1 still apply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needsauth.ForCommand("login")
would inject the credential from 1Pwd into the shell command, but this shell command writes the credential to a file on disk, which defeats the purpose of using the shell plugin IMO.
I could use some help creating the proper schema to support the following scenarios:
- always injecting secrets for regular Pulumi CLI commands which require credentials:
new
,preview
,up
,destroy
, ... - inject
PULUMI_BACKEND_URL
when a custom state backend was configured. This can be a cloud object storage URL (s3, azure blob storage, ...) or the self-hosted variant of our Pulumi Cloud (business critical customers) - inject
PULUMI_ACCESS_TOKEN
when Pulumi Cloud (or self-hosted, see previous) is used. - inject any other credentials in the setup, depending the
op plugin
configuration for the current folder
Could you put me on the right track?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm first going to go through a few things you mentioned, to make sure I'm getting everything right. We want to:
- provision 3rd party CLI credentials whenever
new
,preview
,up
,destroy
, etc are used. - provision either of the
PULUMI_ACCESS_TOKEN
orPULUMI_BACKEND_URL
based on what state backend is being used. Sometimes both can be provisioned, other times only one of them can be provisioned. - not do anything when
login
is used
If all of the above are true then I'm proposing creating 2 credential usages:
Uses: []schema.CredentialUsage{
{
Description: "State backend credentials",
Name: credname.PersonalAccessToken,
NeedsAuth: needsauth.IfAny(
needsauth.ForCommand("new"),
needsauth.ForCommand("preview"),
needsauth.ForCommand("up"),
),
Optional: true,
},
{
Description: "Credentials to provision for 3rd party CLIs",
SelectFrom: &schema.CredentialSelection{
ID: "project",
IncludeAllCredentials: true,
AllowMultiple: true,
},
NeedsAuth: needsauth.IfAny(
needsauth.ForCommand("new"),
needsauth.ForCommand("preview"),
needsauth.ForCommand("up"),
),
Optional: true,
},
},
Let me know if you have any questions about the specific chosen values on the specified fields of the credential usages above.
Another thing that I'm proposing is to make both fields in PersonalAccessToken
optional so that this requirement can be satisfied:
Sometimes both can be provisioned, other times only one of them can be provisioned.
in the context of PULUMI_BACKEND_URL
and PULUMI_ACCESS_TOKEN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndyTitu about this recommendation:
Another thing that I'm proposing is to make both fields in PersonalAccessToken optional so that this requirement can be satisfied:
Sometimes both can be provisioned, other times only one of them can be provisioned.
in the context of
PULUMI_BACKEND_URL
andPULUMI_ACCESS_TOKEN
My latest code has the token and backend URL split up in separate CredentialUsage
schema definitions. See the latest code in this PR. Is this the setup you intended?
Also, with multiple CredentialUsage
schema definitions, can you advice how to write the Go unit tests? They are failing now, I tried to update these but I don't get them to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the intended setup. But I've seen the code advanced a bit from that state. As mentioned on developer Slack, given the current limitations, it could be worth it to first focus on either making it run correctly with the schema while ignoring the UX limitations when importing or focus on what Pulumi users are mostly using (between a PAT without a backend endpoint url, or vice-versa, or both at the same time) and optimise for that workflow.
After that it's done, we can look into any failing tests.
What do you think @ringods ?
865ea9f
to
f37f465
Compare
f37f465
to
a894ed6
Compare
Closing PR due to pulumi/pulumi#13919 |
Overview
Add support for multiple secrets in the Pulumi shell plugin.
Type of change
Related Issue(s)
How To Test
pulumi up
Changelog