Improve error messages around AWS config #3310
Conversation
Does the PR have any schema changes?Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
This is still very verbose for this simple condition... why should I know about Sorry for being difficult but I think it's worth to spend an hour or two to get this to a very nice place for our users and pin the behavior with a test since we keep kind of changing how this presents. Ideally there's something that's easy for our friends from the docs team to help edit. Appreciate it! I realize the ask would put us ahead of what Terraform does :) |
ProgramTest w/o credentials that expects to fail and than asserts against stdout/stderr might work. |
|
Updated errors: Error with no credentials: Error with no region: Error with region but invalid credentials: Expired SSO credentials: |
|
|
This reverts commit 1f87b21.
VenelinMartinov
force-pushed
the
vvm/improve_beginner_error_messages
branch
from
240efbf to
50bcc1a
Compare
provider/configure_test.go
Outdated
| // We need to reset the credentialsValidationRun flag to false, since the | ||
| // previous test might have set it to true. | ||
| credentialsValidationRun.Store(false) |
There was a problem hiding this comment.
This behaviour is unfortunate and not very obvious. It took me a while to realise that the flag was the reason only the first test in a test run would succeed.
LMK if anyone has a better idea how to fix this - the issue is that the credential validation code is run only once because of
pulumi-aws/provider/resources.go
Line 752 in 50bcc1a
This is normally the behaviour we want, except that during tests we run the same "provider" binary multiple times and the flag needs to be reset between runs.
There was a problem hiding this comment.
The easiest way to do it would be to make preConfigureCallback a factory that takes a reference to atomic.Bool and returns func(vars resource.PropertyMap, config shim.ResourceConfig) error. Then you could put credentialsValidationRun inside of the Provider() function.
There was a problem hiding this comment.
That would require a third interface for preConfigureCallback. If we are doing that why not implement a preConfigureOnce and then have the logic in the bridge around making sure we only call it once?
Not sure if it's worth doing for the sake of the tests.
There was a problem hiding this comment.
Yeah this is fine-ish this is what we have to do to test global state. To get rid of global state we'd need to move this to a parameter of provider somehow so tests can set something else in there
// We should only run the validation once to avoid duplicating the reported errors.
var credentialsValidationRun atomic.Bool
There was a problem hiding this comment.
That would require a third interface for preConfigureCallback.
No it wouldn't. I'm not suggesting making tfbridge.ProviderInfo.PreConfigureCallback a factory. I'm suggesting that we make the local preConfigureCallback a factory to capture a reference to a atomic.Bool on Provider()'s stack:
provider/resources.go | 52 ++++++++++++++++++++++++++-------------------------
1 file changed, 27 insertions(+), 25 deletions(-)
diff --git a/provider/resources.go b/provider/resources.go
index a0705e4db1..8d598f6b73 100644
--- a/provider/resources.go
+++ b/provider/resources.go
@@ -731,36 +731,35 @@ func validateCredentials(vars resource.PropertyMap, c shim.ResourceConfig) error
return nil
}
-// We should only run the validation once to avoid duplicating the reported errors.
-var credentialsValidationRun atomic.Bool
-
// preConfigureCallback validates that AWS credentials can be successfully discovered. This emulates the credentials
// configuration subset of `github.com/terraform-providers/terraform-provider-aws/aws.providerConfigure`. We do this
// before passing control to the TF provider to ensure we can report actionable errors.
-func preConfigureCallback(vars resource.PropertyMap, c shim.ResourceConfig) error {
- skipCredentialsValidation := boolValue(vars, "skipCredentialsValidation",
- []string{"AWS_SKIP_CREDENTIALS_VALIDATION"})
-
- // if we skipCredentialsValidation then we don't need to do anything in
- // preConfigureCallback as this is an explicit operation
- if skipCredentialsValidation {
- log.Printf("[INFO] pulumi-aws: skip credentials validation")
- return nil
- }
+func preConfigureCallback(alreadyRun *atomic.Bool) func(resource.PropertyMap, shim.ResourceConfig) error {
+ return func(vars resource.PropertyMap, c shim.ResourceConfig) error {
+ skipCredentialsValidation := boolValue(vars, "skipCredentialsValidation",
+ []string{"AWS_SKIP_CREDENTIALS_VALIDATION"})
+
+ // if we skipCredentialsValidation then we don't need to do anything in
+ // preConfigureCallback as this is an explicit operation
+ if skipCredentialsValidation {
+ log.Printf("[INFO] pulumi-aws: skip credentials validation")
+ return nil
+ }
- var err error
- if credentialsValidationRun.CompareAndSwap(false, true) {
- log.Printf("[INFO] pulumi-aws: starting to validate credentials. " +
- "Disable this by AWS_SKIP_CREDENTIALS_VALIDATION or " +
- "skipCredentialsValidation option")
- err = validateCredentials(vars, c)
- if err == nil {
- log.Printf("[INFO] pulumi-aws: credentials are valid")
- } else {
- log.Printf("[INFO] pulumi-aws: error validating credentials: %v", err)
+ var err error
+ if alreadyRun.CompareAndSwap(false, true) {
+ log.Printf("[INFO] pulumi-aws: starting to validate credentials. " +
+ "Disable this by AWS_SKIP_CREDENTIALS_VALIDATION or " +
+ "skipCredentialsValidation option")
+ err = validateCredentials(vars, c)
+ if err == nil {
+ log.Printf("[INFO] pulumi-aws: credentials are valid")
+ } else {
+ log.Printf("[INFO] pulumi-aws: error validating credentials: %v", err)
+ }
}
+ return err
}
- return err
}
// managedByPulumi is a default used for some managed resources, in the absence of something more meaningful.
@@ -785,6 +784,9 @@ func ProviderFromMeta(metaInfo *tfbridge.MetadataInfo) *tfbridge.ProviderInfo {
ctx := context.Background()
upstreamProvider := newUpstreamProvider(ctx)
+ // We should only run the validation once to avoid duplicating the reported errors.
+ var credentialsValidationRun atomic.Bool
+
p := pftfbridge.MuxShimWithDisjointgPF(ctx, shimv2.NewProvider(upstreamProvider.SDKV2Provider, shimv2.WithDiffStrategy(shimv2.PlanState)), upstreamProvider.PluginFrameworkProvider)
prov := tfbridge.ProviderInfo{
@@ -840,7 +842,7 @@ func ProviderFromMeta(metaInfo *tfbridge.MetadataInfo) *tfbridge.ProviderInfo {
},
},
},
- PreConfigureCallback: preConfigureCallback,
+ PreConfigureCallback: preConfigureCallback(&credentialsValidationRun),
Resources: map[string]*tfbridge.ResourceInfo{
// AWS Certificate Manager
"aws_acm_certificate_validation": {
|
This needs a release with pulumi/pulumi-terraform-bridge#1654 |
|
I think we are hitting issues around PF and SDKV2 bridge applying config defaults - I believe that when the sdkv2 I don't believe this makes a difference for correctness here but I don't think we can express "match string or missing" in our json matching library. I'll change the test with explicit inputs for the config, hopefully that fixes the inconsistency. Related: pulumi/pulumi-terraform-bridge#1603 |
Should fully address #2285 after pulumi/pulumi-terraform-bridge#1640
This makes the error messages when the user has no credentials or no region configured better and more actionable:
Before, no credentials configured:
The line about the region is irrelevant here.
After, no credentials configured:
Before, no region configured:
Here, it is not at all clear that it is the region at fault, since the note about setting the region shows up every time.
After, no region configured:
The note about
config set aws:regiononly shows up in this error case, so clearly actionable.For comparison, upstream, no credentials configured: