-
Notifications
You must be signed in to change notification settings - Fork 10.3k
PSS: safe provider download during init (interactive mode only) #38205
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
Changes from all commits
a9d873f
ca934a5
a491544
e33592e
11a1875
388bcbd
c9b67fb
b59831e
38cc8d8
7fb6906
67e357e
5c553e9
9e643d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,13 @@ type Init struct { | |
| // CreateDefaultWorkspace indicates whether the default workspace should be created by | ||
| // Terraform when initializing a state store for the first time. | ||
| CreateDefaultWorkspace bool | ||
|
|
||
| // SafeInitWithPluggableStateStore indicates whether the user has opted into the process of downloading and approving | ||
| // a new provider binary to use for pluggable state storage. | ||
| // When false and `init` detects that a provider for PSS needs to be downloaded, `init` will return early and prompt the user to re-run with `-safe init`. | ||
| // When true and `init` detects that a provider for PSS needs to be downloaded then the user will experience a new UX. | ||
| // Details of the new UX depending on whether Terraform is being run in automation or not. | ||
| SafeInitWithPluggableStateStore bool | ||
| } | ||
|
|
||
| // ParseInit processes CLI arguments, returning an Init value and errors. | ||
|
|
@@ -117,6 +124,7 @@ func ParseInit(args []string, experimentsEnabled bool) (*Init, tfdiags.Diagnosti | |
| cmdFlags.Var(&init.BackendConfig, "backend-config", "") | ||
| cmdFlags.Var(&init.PluginPath, "plugin-dir", "plugin directory") | ||
| cmdFlags.BoolVar(&init.CreateDefaultWorkspace, "create-default-workspace", true, "when -input=false, use this flag to block creation of the default workspace") | ||
| cmdFlags.BoolVar(&init.SafeInitWithPluggableStateStore, "safe-init", false, `Enable the "safe init" workflow when downloading a provider binary for use with pluggable state storage.`) | ||
|
|
||
| // Used for enabling experimental code that's invoked before configuration is parsed. | ||
| cmdFlags.BoolVar(&init.EnablePssExperiment, "enable-pluggable-state-storage-experiment", false, "Enable the pluggable state storage experiment") | ||
|
|
@@ -158,6 +166,13 @@ func ParseInit(args []string, experimentsEnabled bool) (*Init, tfdiags.Diagnosti | |
| "Terraform cannot use the -create-default-workspace flag (or TF_SKIP_CREATE_DEFAULT_WORKSPACE environment variable) unless experiments are enabled.", | ||
| )) | ||
| } | ||
| if init.SafeInitWithPluggableStateStore { | ||
| diags = diags.Append(tfdiags.Sourceless( | ||
| tfdiags.Error, | ||
| "Cannot use -safe-init flag without experiments enabled", | ||
| "Terraform cannot use the -safe-init flag unless experiments are enabled.", | ||
| )) | ||
| } | ||
| } else { | ||
| // Errors using flags despite experiments being enabled. | ||
| if !init.CreateDefaultWorkspace && !init.EnablePssExperiment { | ||
|
|
@@ -167,6 +182,38 @@ func ParseInit(args []string, experimentsEnabled bool) (*Init, tfdiags.Diagnosti | |
| "Terraform cannot use the -create-default-workspace=false flag (or TF_SKIP_CREATE_DEFAULT_WORKSPACE environment variable) unless you also supply the -enable-pluggable-state-storage-experiment flag (or set the TF_ENABLE_PLUGGABLE_STATE_STORAGE environment variable).", | ||
| )) | ||
| } | ||
| if init.SafeInitWithPluggableStateStore && !init.EnablePssExperiment { | ||
| diags = diags.Append(tfdiags.Sourceless( | ||
| tfdiags.Error, | ||
| "Cannot use -safe-init flag unless the pluggable state storage experiment is enabled", | ||
| "Terraform cannot use the -safe-init flag unless you also supply the -enable-pluggable-state-storage-experiment flag (or set the TF_ENABLE_PLUGGABLE_STATE_STORAGE environment variable).", | ||
| )) | ||
| } | ||
| } | ||
|
|
||
| // Manage all flag interactions with -safe-init | ||
| if init.SafeInitWithPluggableStateStore { | ||
| if !init.Backend { | ||
| diags = diags.Append(tfdiags.Sourceless( | ||
| tfdiags.Error, | ||
| "The -safe-init and -backend=false options are mutually-exclusive", | ||
| "When -backend=false is set Terraform uses information from the last successful init to launch a backend or state store. Any providers used for pluggable state storage should already be downloaded, so -safe-init is unnecessary.", | ||
| )) | ||
| } | ||
| if len(init.PluginPath) > 0 { | ||
| diags = diags.Append(tfdiags.Sourceless( | ||
| tfdiags.Error, | ||
| "The -safe-init and -plugin-dir options are mutually-exclusive", | ||
| "Providers sourced through -plugin-dir have already been vetted by the user, so -safe-init is unnecessary. Please re-run the command without the -safe-init flag.", | ||
| )) | ||
| } | ||
| if init.Lockfile == "readonly" { | ||
| diags = diags.Append(tfdiags.Sourceless( | ||
| tfdiags.Error, | ||
| `The -safe-init and -lockfile=readonly options are mutually-exclusive`, | ||
| "The -safe-init flag is intended to help when first downloading or upgrading a provider to use for state storage, and in those scenarios the lockfile cannot be treated as read-only.", | ||
| )) | ||
| } | ||
|
Comment on lines
+210
to
+216
Member
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. Is it not valid if the user wants to check for an upgrade of the PSS provider but not necessarily perform the upgrade? It's true that first-time installation will always modify the lockfile but upgrade may not necessarily do so.
Member
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. Currently the -upgrade and -lockfile=readonly flags are mutually exclusive (this dates from when the flag was first added to the original getProviders method), so prior to this PR it's not valid to check for any available upgrades that way. Rather than just taking the above as unquestionable gospel I dug around into the original motivations for adding Brief summary of what I learnedIn the original issue #27264 the author experiences a problem where If a filesystem mirror is made via tl;dr: the new flag was a way to navigate around an issue that could have been solved by updating the Registry protocol, but understandably that wasn't an option in the past. The -lockfile=readonly flag is only intended to let people use lockfiles for filesystem mirrors that are sufficient but don't match what would be created by downloading from the Registry. It's not meant to enable something like a 'dry-run' provider download experience.
Member
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. With |
||
| } | ||
|
|
||
| if init.MigrateState && init.Json { | ||
|
|
||
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 know it's been really difficult to come up with a good name for this so I'll just leave a few thoughts here:
download-state-store-plugin,state-store-plugin,state-store,install-state-store-plugin,allow-state-store-pluginor something along these lines.-upgradeand the future flag for automation that implies approval of the interactive prompt.I do recognise though we'll need to be a bit more creative with the naming inside the implementation if we want to avoid "state store". I don't think the two names (user-facing vs internal) need to align necessarily.
Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah this is a decision that I've been putting off a bit 😅 Writing this PR using
-safe-initwas a bit of an exercise in Cunningham's Law but also just unblocking myself.I like the distinction of "safe" versus "trust" in another review comment you left, so a name that includes that might be good?
-prompt-provider-approval-prompt-provider-trust-prompt-state-provider-trust-state-provider-after-approval-interactive-provider-approval👉🏻 Another option could be to revisit the idea of users needing to opt into the security features at all when using Terraform in interactive mode; when we first got that feedback it was in the context of the 'Exit Early' UX being used in both interactive mode and automation. Maybe the prompt for input is sufficient friction for people using Terraform interactively, and a flag is only necessary in the context of automation?