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

Fix AWS session token support #5155

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

JacobHenner
Copy link
Contributor

@JacobHenner JacobHenner commented Nov 2, 2023

AWS session token authentication was inadvertently broken in a refactor of AWS auth handling. Restoring support for session tokens. This fixes a regression that broke the feature introduced in #2573.

Checklist

  • Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #5156

Copy link

github-actions bot commented Nov 2, 2023

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

@JacobHenner
Copy link
Contributor Author

I intend to add tests later this week, which should prevent a similar regression from recurring.

AWS session token authentication was inadvertently broken in a refactor
of AWS auth handling.

Signed-off-by: Jacob Henner <code@ventricle.us>
@JacobHenner JacobHenner force-pushed the fix-aws-session-tokens branch from b1749a0 to 4577dd5 Compare November 2, 2023 17:38
@JacobHenner JacobHenner marked this pull request as ready for review November 2, 2023 17:39
@JacobHenner JacobHenner requested a review from a team as a code owner November 2, 2023 17:39
@JacobHenner
Copy link
Contributor Author

I had considered adding tests along the lines of:

package scalers

import (
	"context"
	"testing"

	"github.com/stretchr/testify/assert"
)

const (
	testAWSCommonRoleArn         = "none"
	testAWSCommonAccessKeyID     = "none"
	testAWSCommonSecretAccessKey = "none"
	testAWSCommonSessionToken    = "none"
)

func TestCredentialPropagation(t *testing.T) {
	ctx := context.Background()
	awsRegion := "eu-west-1"
	awsAuthorization := awsAuthorizationMetadata{
		awsAccessKeyID:     testAWSCommonAccessKeyID,
		awsSecretAccessKey: testAWSCommonSecretAccessKey,
		awsSessionToken:    testAWSCommonSessionToken,
	}
	awsConfig, err := getAwsConfig(ctx, awsRegion, awsAuthorization)
	if err != nil {
		t.Errorf("unexpected error in getAwsConfig: %v", err)
	}
	creds, err := awsConfig.Credentials.Retrieve(ctx)
	if err != nil {
		t.Errorf("unexpected error while retrieving credentials: %v", err)
	}
	assert.Equal(t, testAWSCommonAccessKeyID, creds.AccessKeyID)
	assert.Equal(t, testAWSCommonSecretAccessKey, creds.SecretAccessKey)
	assert.Equal(t, testAWSCommonSessionToken, creds.SessionToken)

}

But awsConfig.Credentials.Retrieve(ctx) fails, as it errors while attempting to refresh credentials as a result of using non-valid test values in awsAuthorizationMetadata.

@JorTurFer
Copy link
Member

JorTurFer commented Nov 21, 2023

/run-e2e aws
Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Nov 21, 2023

Thanks for the fix @JacobHenner
Can we cover this scenario somehow using e2e tests?

@JorTurFer JorTurFer enabled auto-merge (squash) December 11, 2023 07:16
@JorTurFer JorTurFer merged commit 12963ab into kedacore:main Dec 11, 2023
19 checks passed
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
Signed-off-by: anton.lysina <alysina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS session token support broken
2 participants