Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 11 additions & 1 deletion cmd/cluster/aws/destroy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions cmd/cluster/aws/destroy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand Down
2 changes: 0 additions & 2 deletions cmd/infra/aws/create_cli_role.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
18 changes: 6 additions & 12 deletions cmd/infra/aws/create_operator_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
24 changes: 14 additions & 10 deletions cmd/infra/aws/destroy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
awsSession, err := o.AWSCredentialsOpts.AWSCredentialsOpts.GetSession(ctx, "cli-destroy-infra", o.CredentialsSecretData, o.Region)
if err != nil {
return err
Expand Down Expand Up @@ -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
Expand Down
102 changes: 102 additions & 0 deletions cmd/infra/aws/destroy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down
46 changes: 39 additions & 7 deletions cmd/infra/aws/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -20,6 +20,8 @@ import (
"github.com/spf13/pflag"
)

var assumeRoleFn = supportawsutil.AssumeRole

type AWSCredentialsOptions struct {
AWSCredentialsFile string

Expand All @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down
Loading