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

Improve error messages around AWS config #3310

Merged
merged 28 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
83d1474
improve error messages around AWS config
VenelinMartinov Jan 24, 2024
9881bd5
add tests, message for invalid creds
VenelinMartinov Jan 25, 2024
f3dde28
use the pre-release bridge
VenelinMartinov Jan 25, 2024
366c7f1
move error messages to separate text files
VenelinMartinov Jan 25, 2024
a67788d
fix tests
VenelinMartinov Jan 25, 2024
5f4ed15
add expired sso error
VenelinMartinov Jan 25, 2024
1f87b21
link in providertest with more logging
VenelinMartinov Jan 25, 2024
5e454ec
Revert "link in providertest with more logging"
VenelinMartinov Jan 25, 2024
f9d0e9d
dont skip aws cred validation
VenelinMartinov Jan 25, 2024
4857a86
try to run validation again
VenelinMartinov Jan 26, 2024
2b10680
skip credentials tests in short mode
VenelinMartinov Jan 26, 2024
0ec092c
tfgen and build_sdks
VenelinMartinov Jan 26, 2024
c99f4c3
schema
VenelinMartinov Jan 26, 2024
a2673af
schema?
VenelinMartinov Jan 26, 2024
9adbe12
more schema
VenelinMartinov Jan 26, 2024
4a7a7b3
cleaned plugins, regenerate schema and sdks
VenelinMartinov Jan 26, 2024
806bc3d
unset the aws creds before running the tests
VenelinMartinov Jan 27, 2024
7a82647
merge master
VenelinMartinov Jan 27, 2024
50bcc1a
reset credential validation flag before tests
VenelinMartinov Jan 29, 2024
c249e19
fix test for duplicate errors
VenelinMartinov Jan 29, 2024
9d8a964
remove unused test main
VenelinMartinov Jan 29, 2024
039c10b
use prerelease pf
VenelinMartinov Jan 29, 2024
847eaf6
go mod tidy
VenelinMartinov Jan 29, 2024
c946c9a
add some *s
VenelinMartinov Jan 29, 2024
376f960
make the validation run flag local
VenelinMartinov Jan 30, 2024
f4f5bb8
Merge branch 'master' into vvm/improve_beginner_error_messages
VenelinMartinov Jan 31, 2024
90f9a4c
don't match response inputs
VenelinMartinov Jan 31, 2024
3e4f0db
fix tests
VenelinMartinov Jan 31, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,5 @@ sdk/python/*.egg-info


sdk/python/venv
go.work
go.work.sum
183 changes: 183 additions & 0 deletions provider/configure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package provider

import (
"context"
"os"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -117,6 +118,185 @@ func TestCheckConfigFastWithCustomEndpoints(t *testing.T) {
require.Truef(t, time0.Add(cutoff).After(time.Now()), "CheckConfig with custom endpoints is taking more than %v", cutoff)
}

func unsetAWSEnv() {
os.Unsetenv("AWS_SECRET_ACCESS_KEY")
os.Unsetenv("AWS_SESSION_TOKEN")
os.Unsetenv("AWS_DEFAULT_REGION")
os.Unsetenv("AWS_PROFILE")
os.Unsetenv("AWS_ACCESS_KEY_ID")
os.Unsetenv("AWS_SECRET_ACCESS_KEY")
os.Unsetenv("AWS_REGION")
os.Setenv("AWS_CONFIG_FILE", "non-existent/config.json")
os.Setenv("AWS_SHARED_CREDENTIALS_FILE", "non-existent/credentials")
}

func skipIfNotShort(t *testing.T) {
if !testing.Short() {
t.Skipf("Skipping test in non-short mode")
}
}

func TestMissingCredentialsErrorMessage(t *testing.T) {
skipIfNotShort(t)
unsetAWSEnv()
// We need to reset the credentialsValidationRun flag to false, since the
// previous test might have set it to true.
credentialsValidationRun.Store(false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if credentialsValidationRun.CompareAndSwap(false, true) {

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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": {

os.Setenv("AWS_SKIP_CREDENTIALS_VALIDATION", "false")

replaySequence(t, `
[{
"method": "/pulumirpc.ResourceProvider/CheckConfig",
"request": {
"urn": "urn:pulumi:dev::aws_no_creds::pulumi:providers:aws::default_6_18_2",
"olds": {},
"news": {
"version": "6.18.2"
}
},
"response": {
"inputs": {
"skipCredentialsValidation": "false",
"skipMetadataApiCheck": "true",
"skipRegionValidation": "true",
"version": "6.18.2"
},
"failures": [
{
"reason": "No valid credential sources found.\nPlease see https://www.pulumi.com/registry/packages/aws/installation-configuration/ for more information about providing credentials.\nNEW: You can use Pulumi ESC to set up dynamic credentials with AWS OIDC to ensure the correct and valid credentials are used.\nLearn more: https://www.pulumi.com/registry/packages/aws/installation-configuration/#dynamically-generate-credentials"
}
]
},
"metadata": {
"kind": "resource",
"mode": "client",
"name": "aws"
}
}]`)
}

func TestMissingRegionErrorMessage(t *testing.T) {
skipIfNotShort(t)
unsetAWSEnv()
credentialsValidationRun.Store(false)
os.Setenv("AWS_ACCESS_KEY_ID", "VALID")
os.Setenv("AWS_SECRET_ACCESS_KEY", "VALID")
os.Setenv("AWS_SKIP_CREDENTIALS_VALIDATION", "false")

replaySequence(t, strings.ReplaceAll(`
[{
"method": "/pulumirpc.ResourceProvider/CheckConfig",
"request": {
"urn": "urn:pulumi:dev::aws_no_creds::pulumi:providers:aws::default_6_18_2",
"olds": {},
"news": {
"version": "6.18.2"
}
},
"response": {
"inputs": {
"skipCredentialsValidation": "false",
"skipMetadataApiCheck": "true",
"skipRegionValidation": "true",
"version": "6.18.2"
},
"failures": [
{
"reason": "Missing region information\nMake sure you have set your AWS region, e.g. '''pulumi config set aws:region us-west-2'''."
}
]
},
"metadata": {
"kind": "resource",
"mode": "client",
"name": "aws"
}
}]`, "'''", "`"))
}

func TestInvalidCredentialsErrorMessage(t *testing.T) {
skipIfNotShort(t)
unsetAWSEnv()
credentialsValidationRun.Store(false)
os.Setenv("AWS_ACCESS_KEY_ID", "INVALID")
os.Setenv("AWS_SECRET_ACCESS_KEY", "INVALID")
os.Setenv("AWS_REGION", "us-west-2")
os.Setenv("AWS_SKIP_CREDENTIALS_VALIDATION", "false")

replaySequence(t, `
[{
"method": "/pulumirpc.ResourceProvider/CheckConfig",
"request": {
"urn": "urn:pulumi:dev::aws_no_creds::pulumi:providers:aws::default_6_18_2",
"olds": {},
"news": {
"version": "6.18.2"
}
},
"response": {
"inputs": {
"region": "us-west-2",
"skipCredentialsValidation": "false",
"skipMetadataApiCheck": "true",
"skipRegionValidation": "true",
"version": "6.18.2"
},
"failures": [
{
"reason": "Invalid credentials configured.\nPlease see https://www.pulumi.com/registry/packages/aws/installation-configuration/ for more information about providing credentials.\nNEW: You can use Pulumi ESC to set up dynamic credentials with AWS OIDC to ensure the correct and valid credentials are used.\nLearn more: https://www.pulumi.com/registry/packages/aws/installation-configuration/#dynamically-generate-credentials"
}
]
},
"metadata": {
"kind": "resource",
"mode": "client",
"name": "aws"
}
}]`)
}

func TestOtherFailureErrorMessage(t *testing.T) {
skipIfNotShort(t)
unsetAWSEnv()
credentialsValidationRun.Store(false)
os.Setenv("AWS_ACCESS_KEY_ID", "INVALID")
os.Setenv("AWS_SECRET_ACCESS_KEY", "INVALID")
os.Setenv("AWS_REGION", "us-west-2")
os.Setenv("AWS_PROFILE", "non-existent-profile")
os.Setenv("AWS_SKIP_CREDENTIALS_VALIDATION", "false")

replaySequence(t, `
[{
"method": "/pulumirpc.ResourceProvider/CheckConfig",
"request": {
"urn": "urn:pulumi:dev::aws_no_creds::pulumi:providers:aws::default_6_18_2",
"olds": {},
"news": {
"version": "6.18.2"
}
},
"response": {
"inputs": {
"region": "us-west-2",
"skipCredentialsValidation": "false",
"skipMetadataApiCheck": "true",
"skipRegionValidation": "true",
"version": "6.18.2"
},
"failures": [
{
"reason": "unable to validate AWS credentials.\nDetails: loading configuration: failed to get shared config profile, non-existent-profile\n"
}
]
},
"metadata": {
"kind": "resource",
"mode": "client",
"name": "aws"
}
}]`)
}

func replaySequence(t *testing.T, sequence string) {
info := *Provider()
ctx := context.Background()
Expand All @@ -135,3 +315,6 @@ func replaySequence(t *testing.T, sequence string) {
func init() {
version.Version = "6.0.0"
}

func TestMain(m *testing.M) {
}
4 changes: 4 additions & 0 deletions provider/errors/expired_sso.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Failed to refresh cached SSO credentials.
Please refresh SSO login.
NEW: You can use Pulumi ESC to set up dynamic credentials with AWS OIDC to ensure the correct and valid credentials are used.
Learn more: https://www.pulumi.com/registry/packages/aws/installation-configuration/#dynamically-generate-credentials
4 changes: 4 additions & 0 deletions provider/errors/invalid_credentials.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Invalid credentials configured.
Please see https://www.pulumi.com/registry/packages/aws/installation-configuration/ for more information about providing credentials.
NEW: You can use Pulumi ESC to set up dynamic credentials with AWS OIDC to ensure the correct and valid credentials are used.
Learn more: https://www.pulumi.com/registry/packages/aws/installation-configuration/#dynamically-generate-credentials
4 changes: 4 additions & 0 deletions provider/errors/no_credentials.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
No valid credential sources found.
Please see https://www.pulumi.com/registry/packages/aws/installation-configuration/ for more information about providing credentials.
NEW: You can use Pulumi ESC to set up dynamic credentials with AWS OIDC to ensure the correct and valid credentials are used.
Learn more: https://www.pulumi.com/registry/packages/aws/installation-configuration/#dynamically-generate-credentials
2 changes: 2 additions & 0 deletions provider/errors/no_region.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Missing region information
Make sure you have set your AWS region, e.g. `pulumi config set aws:region us-west-2`.
6 changes: 5 additions & 1 deletion provider/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,15 @@ func providerServer(t *testing.T) pulumirpc.ResourceProviderServer {
return p
}

func test(t *testing.T, dir string, opts ...providertest.Option) {
func skipIfShort(t *testing.T) {
if testing.Short() {
t.Skipf("Skipping in testing.Short() mode, assuming this is a CI run without AWS creds")
return
}
}

func test(t *testing.T, dir string, opts ...providertest.Option) {
skipIfShort(t)
opts = append(opts,
providertest.WithProviderName("aws"),
providertest.WithBaselineVersion("5.42.0"),
Expand Down
71 changes: 64 additions & 7 deletions provider/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,18 @@ func durationFromConfig(vars resource.PropertyMap, prop resource.PropertyKey) (*
return nil, nil
}

//go:embed errors/no_credentials.txt
var noCredentialsError string

//go:embed errors/invalid_credentials.txt
var invalidCredentialsError string

//go:embed errors/no_region.txt
var noRegionError string

//go:embed errors/expired_sso.txt
var expiredSSOError string

func validateCredentials(vars resource.PropertyMap, c shim.ResourceConfig) error {
config := &awsbase.Config{
AccessKey: stringValue(vars, "accessKey", []string{"AWS_ACCESS_KEY_ID"}),
Expand Down Expand Up @@ -662,13 +674,58 @@ func validateCredentials(vars resource.PropertyMap, c shim.ResourceConfig) error
config.SharedConfigFiles = []string{configPath}

if _, _, diag := awsbase.GetAwsConfig(context.Background(), config); diag != nil && diag.HasError() {
return fmt.Errorf("unable to validate AWS credentials. \n"+
"Details: %s\n"+
"Make sure you have set your AWS region, e.g. `pulumi config set aws:region us-west-2`. \n\n"+
"NEW: You can use Pulumi ESC to set up dynamic credentials with AWS OIDC to ensure the "+
"correct and valid credentials are used.\nLearn more: "+
"https://www.pulumi.com/registry/packages/aws/installation-configuration/#dynamically-generate-credentials",
formatDiags(diag))
formattedDiag := formatDiags(diag)
// Normally it'd query sts.REGION.amazonaws.com
// but if we query sts..amazonaws.com, then we don't have a region.
if strings.Contains(formattedDiag, "dial tcp: lookup sts..amazonaws.com: no such host") {
VenelinMartinov marked this conversation as resolved.
Show resolved Hide resolved
return tfbridge.CheckFailureError{
Failures: []tfbridge.CheckFailureErrorElement{
{
Reason: noRegionError,
Property: "",
},
},
}
}
if strings.Contains(formattedDiag, "no EC2 IMDS role found") {
return tfbridge.CheckFailureError{
Failures: []tfbridge.CheckFailureErrorElement{
{
Reason: noCredentialsError,
Property: "",
},
},
}
}
if strings.Contains(formattedDiag, "The security token included in the request is invalid") {
return tfbridge.CheckFailureError{
Failures: []tfbridge.CheckFailureErrorElement{
{
Reason: invalidCredentialsError,
Property: "",
},
},
}
}
if strings.Contains(formattedDiag, "failed to refresh cached credentials") {
return tfbridge.CheckFailureError{
Failures: []tfbridge.CheckFailureErrorElement{
{
Reason: expiredSSOError,
Property: "",
},
},
}
}

return tfbridge.CheckFailureError{
Failures: []tfbridge.CheckFailureErrorElement{
{
Reason: fmt.Sprintf("unable to validate AWS credentials.\nDetails: %s\n", formattedDiag),
Property: "",
},
},
}
}

return nil
Expand Down
Loading