Merge pull request #8889 from ironcladlou/aws-auth-refactor

NO-JIRA: Fall back to AWS SDK default credential chain when no explicit credentials provided

Author: openshift-merge-bot[bot]

cmd/cluster/aws/destroy.go

@@ -157,8 +157,18 @@ func DestroyCluster(ctx context.Context, o *core.DestroyOptions) error {
 // ValidateCredentialInfo validates if the credentials secret name is empty, the aws-creds or sts-creds mutually exclusive and are not empty; validates if
 // the credentials secret is not empty, that it can be retrieved.
 func ValidateCredentialInfo(opts awsutil.AWSCredentialsOptions, credentialSecretName, namespace, kubeconfigPath string) error {
+	return validateCredentialInfo(opts, credentialSecretName, namespace, kubeconfigPath, opts.Validate)
+}
+
+// ValidateProductCredentialInfo is like ValidateCredentialInfo but requires explicit --sts-creds and --role-arn
+// flags rather than allowing SDK default chain fallback.
+func ValidateProductCredentialInfo(opts awsutil.AWSCredentialsOptions, credentialSecretName, namespace, kubeconfigPath string) error {
+	return validateCredentialInfo(opts, credentialSecretName, namespace, kubeconfigPath, opts.ValidateProduct)
+}
+
+func validateCredentialInfo(opts awsutil.AWSCredentialsOptions, credentialSecretName, namespace, kubeconfigPath string, validate func() error) error {
 	if len(credentialSecretName) == 0 {
-		if err := opts.Validate(); err != nil {
+		if err := validate(); err != nil {
 			return err
 		}
 		return nil

cmd/cluster/aws/destroy_test.go

@@ -14,7 +14,7 @@ func Test_ValidateCredentialInfo(t *testing.T) {
 		inputOptions *core.DestroyOptions
 		expectError  bool
 	}{
-		"when CredentialSecretName is blank and aws-creds is also blank": {
+		"when CredentialSecretName is blank and aws-creds is also blank it should fall back to SDK default chain": {
 			inputOptions: &core.DestroyOptions{
 				CredentialSecretName: "",
 				AWSPlatform: core.AWSPlatformDestroyOptions{
@@ -23,7 +23,7 @@ func Test_ValidateCredentialInfo(t *testing.T) {
 					},
 				},
 			},
-			expectError: true,
+			expectError: false,
 		},
 		"when CredentialSecretName is blank and aws-creds is not blank": {
 			inputOptions: &core.DestroyOptions{

cmd/infra/aws/create_cli_role.go

@@ -175,8 +175,6 @@ func NewCreateCLIRoleCommand() *cobra.Command {
 	cmd.Flags().StringVar(&opts.RoleName, "name", opts.RoleName, "Role name")
 	cmd.Flags().StringToStringVarP(&opts.AdditionalTags, "additional-tags", "t", opts.AdditionalTags, "Additional tags to apply to the role created (e.g. 'key1=value1,key2=value2')")
 
-	_ = cmd.MarkFlagRequired("aws-creds")
-
 	logger := log.Log
 	cmd.RunE = func(cmd *cobra.Command, args []string) error {
 		if err := opts.Run(cmd.Context(), logger); err != nil {

cmd/infra/aws/create_operator_roles.go

@@ -126,18 +126,12 @@ func (o *CreateOperatorRolesOptions) Run(ctx context.Context, logger logr.Logger
 		return err
 	}
 
-	var awsSession *aws.Config
-	if o.AWSCredentialsOpts.AWSCredentialsFile != "" || o.AWSCredentialsOpts.STSCredentialsFile != "" {
-		if err := o.AWSCredentialsOpts.Validate(); err != nil {
-			return err
-		}
-		var err error
-		awsSession, err = o.AWSCredentialsOpts.GetSession(ctx, "cli-create-operator-roles", nil, o.Region)
-		if err != nil {
-			return err
-		}
-	} else {
-		awsSession = awsutil.NewSession(ctx, "cli-create-operator-roles", "", "", "", o.Region)
+	if err := o.AWSCredentialsOpts.Validate(); err != nil {
+		return err
+	}
+	awsSession, err := o.AWSCredentialsOpts.GetSession(ctx, "cli-create-operator-roles", nil, o.Region)
+	if err != nil {
+		return err
 	}
 	awsConfig := awsutil.NewConfig()
 	iamClient := iam.NewFromConfig(*awsSession, func(o *iam.Options) {

cmd/infra/aws/destroy.go

@@ -101,20 +101,17 @@ func (o *DelegatedAWSCredentialOptions) Validate() error {
 		}
 	}
 
-	// ensure that only one type of credential has been passed
-	globalCredentialsPresent := o.AWSCredentialsOpts.AWSCredentialsFile != "" || o.AWSCredentialsOpts.STSCredentialsFile != ""
-	if globalCredentialsPresent {
-		if !anyComponentCredentialsPresent {
-			return o.AWSCredentialsOpts.Validate()
-		} else {
+	if anyComponentCredentialsPresent {
+		if o.AWSCredentialsOpts.AWSCredentialsFile != "" || o.AWSCredentialsOpts.STSCredentialsFile != "" || o.AWSCredentialsOpts.RoleArn != "" {
 			return fmt.Errorf("cannot set any --aws-creds.component flags at the same time as other credentials")
 		}
-	} else {
 		if !allComponentCredentialsPresent {
-			return fmt.Errorf("either --aws-creds, --sts-creds, or all --aws-creds.component flags must be set")
+			return fmt.Errorf("all --aws-creds.component flags must be set when using per-component credentials")
 		}
+		return nil
 	}
-	return nil
+
+	return o.AWSCredentialsOpts.Validate()
 }
 
 func NewDestroyCommand() *cobra.Command {
@@ -199,7 +196,13 @@ func (o *DestroyInfraOptions) DestroyInfra(ctx context.Context) error {
 	var clusterRoute53Client, vpcOwnerRoute53Client, listRoute53Client, recordsRoute53Client awsapi.ROUTE53API
 	var s3Client awsapi.S3API
 	var ramClient *ram.Client
-	if o.AWSCredentialsOpts.AWSCredentialsOpts.AWSCredentialsFile != "" || o.AWSCredentialsOpts.AWSCredentialsOpts.STSCredentialsFile != "" {
+	useDelegatedCredentials := o.AWSEbsCsiDriverControllerCredentialsFile != "" ||
+		o.CloudControllerCredentialsFile != "" ||
+		o.CloudNetworkConfigControllerCredentialsFile != "" ||
+		o.ControlPlaneOperatorCredentialsFile != "" ||
+		o.NodePoolCredentialsFile != "" ||
+		o.OpenshiftImageRegistryCredentialsFile != ""
+	if !useDelegatedCredentials {
 		awsSession, err := o.AWSCredentialsOpts.AWSCredentialsOpts.GetSession(ctx, "cli-destroy-infra", o.CredentialsSecretData, o.Region)
 		if err != nil {
 			return err
@@ -265,6 +268,7 @@ func (o *DestroyInfraOptions) DestroyInfra(ctx context.Context) error {
 			return fmt.Errorf("failed to create delegating client: %w", err)
 		}
 		ec2Client = delegatingClent.EC2Client
+		vpcOwnerEC2Client = ec2Client
 		elbClient = delegatingClent.ELBClient
 		elbv2Client = delegatingClent.ELBV2Client
 		listRoute53Client = delegatingClent.ROUTE53Client

cmd/infra/aws/destroy_test.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"testing"
 
+	awsutil "github.com/openshift/hypershift/cmd/infra/aws/util"
 	"github.com/openshift/hypershift/support/awsapi"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -21,6 +22,107 @@ import (
 	"go.uber.org/mock/gomock"
 )
 
+func TestDelegatedAWSCredentialOptionsValidate(t *testing.T) {
+	allComponentCreds := DelegatedAWSCredentialOptions{
+		AWSCredentialsOpts:                          &awsutil.AWSCredentialsOptions{},
+		AWSEbsCsiDriverControllerCredentialsFile:    "/tmp/ebs.ini",
+		CloudControllerCredentialsFile:              "/tmp/cloud.ini",
+		CloudNetworkConfigControllerCredentialsFile: "/tmp/net.ini",
+		ControlPlaneOperatorCredentialsFile:         "/tmp/cpo.ini",
+		NodePoolCredentialsFile:                     "/tmp/np.ini",
+		OpenshiftImageRegistryCredentialsFile:       "/tmp/registry.ini",
+	}
+
+	testCases := []struct {
+		name          string
+		opts          DelegatedAWSCredentialOptions
+		expectError   bool
+		errorContains string
+	}{
+		{
+			name:        "When all component credentials provided, it should succeed",
+			opts:        allComponentCreds,
+			expectError: false,
+		},
+		{
+			name: "When component credentials mixed with aws-creds, it should fail",
+			opts: DelegatedAWSCredentialOptions{
+				AWSCredentialsOpts: &awsutil.AWSCredentialsOptions{
+					AWSCredentialsFile: "/tmp/global.ini",
+				},
+				AWSEbsCsiDriverControllerCredentialsFile:    "/tmp/ebs.ini",
+				CloudControllerCredentialsFile:              "/tmp/cloud.ini",
+				CloudNetworkConfigControllerCredentialsFile: "/tmp/net.ini",
+				ControlPlaneOperatorCredentialsFile:         "/tmp/cpo.ini",
+				NodePoolCredentialsFile:                     "/tmp/np.ini",
+				OpenshiftImageRegistryCredentialsFile:       "/tmp/registry.ini",
+			},
+			expectError:   true,
+			errorContains: "cannot set any --aws-creds.component flags at the same time as other credentials",
+		},
+		{
+			name: "When component credentials mixed with role-arn, it should fail",
+			opts: DelegatedAWSCredentialOptions{
+				AWSCredentialsOpts: &awsutil.AWSCredentialsOptions{
+					RoleArn: "arn:aws:iam::123456789012:role/test-role",
+				},
+				AWSEbsCsiDriverControllerCredentialsFile:    "/tmp/ebs.ini",
+				CloudControllerCredentialsFile:              "/tmp/cloud.ini",
+				CloudNetworkConfigControllerCredentialsFile: "/tmp/net.ini",
+				ControlPlaneOperatorCredentialsFile:         "/tmp/cpo.ini",
+				NodePoolCredentialsFile:                     "/tmp/np.ini",
+				OpenshiftImageRegistryCredentialsFile:       "/tmp/registry.ini",
+			},
+			expectError:   true,
+			errorContains: "cannot set any --aws-creds.component flags at the same time as other credentials",
+		},
+		{
+			name: "When partial component credentials provided, it should fail",
+			opts: DelegatedAWSCredentialOptions{
+				AWSCredentialsOpts:                          &awsutil.AWSCredentialsOptions{},
+				AWSEbsCsiDriverControllerCredentialsFile:    "/tmp/ebs.ini",
+				CloudControllerCredentialsFile:              "/tmp/cloud.ini",
+				CloudNetworkConfigControllerCredentialsFile: "/tmp/net.ini",
+			},
+			expectError:   true,
+			errorContains: "all --aws-creds.component flags must be set when using per-component credentials",
+		},
+		{
+			name: "When no component credentials provided, it should fall back to AWSCredentialsOpts.Validate",
+			opts: DelegatedAWSCredentialOptions{
+				AWSCredentialsOpts: &awsutil.AWSCredentialsOptions{},
+			},
+			expectError: false,
+		},
+		{
+			name: "When no component credentials and invalid AWSCredentialsOpts, it should propagate validation error",
+			opts: DelegatedAWSCredentialOptions{
+				AWSCredentialsOpts: &awsutil.AWSCredentialsOptions{
+					STSCredentialsFile: "/tmp/creds.json",
+				},
+			},
+			expectError:   true,
+			errorContains: "'role-arn' is required when 'sts-creds' is provided",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.opts.Validate()
+			if tc.expectError {
+				if err == nil {
+					t.Fatal("Expected error but got nil")
+				}
+				if tc.errorContains != "" && !strings.Contains(err.Error(), tc.errorContains) {
+					t.Errorf("Expected error containing %q, got %q", tc.errorContains, err.Error())
+				}
+			} else if err != nil {
+				t.Errorf("Unexpected error: %v", err)
+			}
+		})
+	}
+}
+
 func TestEmptyBucket(t *testing.T) {
 	tests := []struct {
 		name          string

cmd/infra/aws/util/util.go

@@ -2,13 +2,13 @@ package util
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"net"
 	"os"
 	"time"
 
 	"github.com/openshift/hypershift/cmd/util"
+	supportawsutil "github.com/openshift/hypershift/support/awsutil"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	awsmiddleware "github.com/aws/aws-sdk-go-v2/aws/middleware"
@@ -20,6 +20,8 @@ import (
 	"github.com/spf13/pflag"
 )
 
+var assumeRoleFn = supportawsutil.AssumeRole
+
 type AWSCredentialsOptions struct {
 	AWSCredentialsFile string
 
@@ -32,24 +34,31 @@ func (opts *AWSCredentialsOptions) Validate() error {
 		if opts.STSCredentialsFile != "" || opts.RoleArn != "" {
 			return fmt.Errorf("only one of 'aws-creds' or 'role-arn' and 'sts-creds' can be provided")
 		}
-
 		return nil
 	}
 
-	if err := util.ValidateRequiredOption("sts-creds", opts.STSCredentialsFile); err != nil {
-		return err
+	if opts.STSCredentialsFile != "" && opts.RoleArn == "" {
+		return fmt.Errorf("'role-arn' is required when 'sts-creds' is provided")
 	}
-	if err := util.ValidateRequiredOption("role-arn", opts.RoleArn); err != nil {
+
+	return nil
+}
+
+func (opts *AWSCredentialsOptions) ValidateProduct() error {
+	if err := opts.Validate(); err != nil {
 		return err
 	}
+	if opts.STSCredentialsFile == "" || opts.RoleArn == "" {
+		return fmt.Errorf("'sts-creds' and 'role-arn' are required")
+	}
 	return nil
 }
 
 func (opts *AWSCredentialsOptions) BindFlags(flags *pflag.FlagSet) {
 	opts.BindProductFlags(flags)
 
 	flags.StringVar(&opts.AWSCredentialsFile, "aws-creds", opts.AWSCredentialsFile, "Path to an AWS credentials file")
-	_ = flags.MarkDeprecated("aws-creds", "please use '--sts-creds' with '--role-arn' instead")
+	_ = flags.MarkDeprecated("aws-creds", "the AWS SDK default credential chain is used when no explicit credentials are provided; use '--role-arn' to assume a specific role")
 }
 
 func (opts *AWSCredentialsOptions) BindVPCOwnerFlags(flags *pflag.FlagSet) {
@@ -86,7 +95,30 @@ func (opts *AWSCredentialsOptions) GetSession(ctx context.Context, agent string,
 		return NewSTSSession(ctx, agent, opts.RoleArn, region, &v2Creds)
 	}
 
-	return nil, errors.New("could not create AWS session, no credentials were given")
+	// Fall back to the SDK default credential chain (env vars, AWS_PROFILE, instance profile, etc.)
+	cfg := NewSession(ctx, agent, "", "", "", region)
+	if opts.RoleArn != "" {
+		assumedCreds, err := assumeRoleFn(ctx, *cfg, agent, opts.RoleArn)
+		if err != nil {
+			return nil, err
+		}
+		var loadOptions []func(*config.LoadOptions) error
+		loadOptions = append(loadOptions, config.WithCredentialsProvider(
+			credentials.StaticCredentialsProvider{Value: *assumedCreds},
+		))
+		if region != "" {
+			loadOptions = append(loadOptions, config.WithRegion(region))
+		}
+		loadOptions = append(loadOptions, config.WithAPIOptions([]func(*middleware.Stack) error{
+			awsmiddleware.AddUserAgentKeyValue("openshift.io hypershift", agent),
+		}))
+		finalCfg, err := config.LoadDefaultConfig(ctx, loadOptions...)
+		if err != nil {
+			return nil, fmt.Errorf("failed to load config after assuming role: %w", err)
+		}
+		return &finalCfg, nil
+	}
+	return cfg, nil
 }
 
 func NewSession(ctx context.Context, agent, credentialsFile, credKey, credSecretKey, region string) *aws.Config {