-
Notifications
You must be signed in to change notification settings - Fork 1.2k
BUG: experimental secret support for non-standard resource IDs was not propagating to deploy prepare #10839
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
Open
Berlioz
wants to merge
3
commits into
main
Choose a base branch
from
vsfan_secrets_bugfix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+120
−1
Open
BUG: experimental secret support for non-standard resource IDs was not propagating to deploy prepare #10839
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -361,6 +361,8 @@ | |
| opts.isEmulator, | ||
| opts.force, | ||
| ); | ||
| // build.secretEnvVars and build.params may be out of sync after param resolution, if new secret resources were created during it | ||
| matchSecretEnvVars(opts.build); | ||
|
|
||
| return { backend: toBackend(opts.build, paramValues), envs: paramValues, secretRefs: secretRefs }; | ||
| } | ||
|
|
@@ -514,7 +516,7 @@ | |
| // List param, we try resolving a String param instead. | ||
| try { | ||
| regions = params.resolveList(bdEndpoint.region, paramValues); | ||
| } catch (err: any) { | ||
| if (err instanceof ExprParseError) { | ||
| regions = [params.resolveString(bdEndpoint.region, paramValues)]; | ||
| } else { | ||
|
|
@@ -804,7 +806,7 @@ | |
| * /version can be omitted and will cause the secret to resolve to whatever the latest version was at time of deploy. | ||
| * | ||
| * For each binding imported from the .env file, | ||
| * 1) TODO: Check if a conflicting SecretParam with the same name exists. If so, override the param so that the prompting flow will look in the right place when deciding whether or not to create a new Secret. | ||
| * 1) Check if a conflicting SecretParam with the same name exists. If so, override the param so that the prompting flow will look in the right place when deciding whether or not to create a new Secret. | ||
| * 2) Upsert the binding directly into the Build's SecretEnvVars, which will cause it to be actually available in process.ENV | ||
| */ | ||
| export function applyEnvSecretBindings( | ||
|
|
@@ -863,6 +865,41 @@ | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Updates the SecretEnvVars of a Build to use the same backing resources as its Secret Params. | ||
| * | ||
| * This is usually ensured by applyEnvSecretBindings, which reconciles the state of SecretEnvVars and | ||
| * Secrets before param handling. However, param handling itself can change the Secrets in the case where | ||
| * the user is prompted to create a new Secret resource, and selects a non-default resource ID. | ||
| * | ||
| * This function is a no-op in all other cases. | ||
| */ | ||
| export function matchSecretEnvVars(build: Build): void { | ||
| for (const param of build.params) { | ||
| if (param.type !== "secret") { | ||
| continue; | ||
| } | ||
| const secretParam = param; | ||
| if (!secretParam.resourceId) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const endpoint of Object.values(build.endpoints)) { | ||
| for (const envVar of endpoint.secretEnvironmentVariables ?? []) { | ||
| if (envVar.key.toUpperCase() !== secretParam.name.toUpperCase()) { | ||
| continue; | ||
| } | ||
| if (envVar.secret !== secretParam.resourceId) { | ||
| logger.debug( | ||
| `Inserted newly created secret into build.SecretEnvVars: ${envVar.key}=${secretParam.resourceId}`, | ||
| ); | ||
| envVar.secret = secretParam.resourceId; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+877
to
+901
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but it came through applyEnvSecretBindings, so we know--whatever, fine |
||
|
|
||
| /** | ||
| * Parses any of the supported formats used to refer to a Secret in .env: | ||
| * API_KEY=<secret-id> | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Jetski thinks we can flatten this more:
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 pretty sure Record is preferred to Map in this repo?
More seriously, I don't agree with Jetski here and I don't even think this suggestion would compile without some fairly ugly surgery: in the context of this file, SecretParam is build.SecretParam which contains just the wire-level representation of a secret that we get from the SDK. For the types to line up here the Map would have to be declared with the params.SecretParam type, which isn't currently exported.
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.
Fair point on both counts! I didn't catch that SecretParam in build.ts was a local type collision with params.ts. Since you have already pushed the test cases, going ahead with the approval