Add AWS region to the AWS Config Cache key

[maxbog]

The PR adds the AWS region to the key used by the AWS config cache

Checklist

• When introducing a new scaler, I agree with the scaling governance policy
• I have verified that my change is according to the deprecations & breaking changes policy
• Tests have been added
• Changelog has been updated and is aligned with our changelog requirements
• A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
• A PR is opened to update the documentation on (repo) (if applicable)
• Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #6128

[zroubalik]

The build is broken

[maxbog]

@zroubalik I fixed the build

[zroubalik]

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

[zroubalik]

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

[maxbog]

@zroubalik Is something wrong with the e2e tests? I see they failed twice now :(

[zroubalik]

@zroubalik Is something wrong with the e2e tests? I see they failed twice now :(

yeah, issue on our side, will rerun once it is fixed

[ndlanier]

@zroubalik any updates on the e2e testing?

[JorTurFer]

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

[maxbog]

@JorTurFer I rebased the PR
Now the tests failed, but it looks like some random crash, can you please rerun the failing job and the e2e tests?

[JorTurFer]

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

[JorTurFer]

PTAL @kedacore/keda-core-contributors

[ndlanier]

Any insight into why it paniced/failed on the amd64 validation?

[ndlanier]

@JorTurFer is the validate - amd64 stage failure an issue with the action or an issue with the code changes in this pr?

[JorTurFer]

@JorTurFer is the validate - amd64 stage failure an issue with the action or an issue with the code changes in this pr?

I've triggered it again because probably it was a transient error

[JorTurFer]

@zroubalik @wozniakjan PTAL

[JorTurFer]

i would suggest to add Region in aws authorization metadata, rather than passing it all around.

5ea1eac/pkg/scalers/aws/aws_authorization.go#L18C1-L35

wdyt @JorTurFer ?

That's a good idea tbh. Could you update the PR @maxbog ?

[maxbog]

@JorTurFer @ThaSami
Thanks for the review.
At first glance, I also thought this might be a good idea. However, after digging into the code, I'm not that convinced anymore. Here are the reasons:

1. Although the AuthorizationMetadata struct has Authorization in the name, what it really represents is an AWS identity: a role + credentials coming from pod identity or the AccessKey/SecretAccessKey pair. Identities are global in AWS, so IMO, region does not belong in this struct. Region is actually only used when creating AWS clients, to specify, where the queried services are, not where the identity is located.
2. We would get rid of the awsRegion parameter in the getCacheKey function, but then, if we still want to add it to the AuthorizationMetadata, we need to add a awsRegion parameter to the GetAwsAuthorization function, which, in turn, means we have to pass it from the different types of scaler metadata (for example, here, here, or here, or 5 different places)
3. Having the awsRegion parameter in the getCacheKey actually makes sense when you look at the AuthorizationMetadata as an identity. The aws.Config struct represents a AWS service client configuration, so IMO it makes perfect sense to cache it for a specific identity and for a specific region. We lose expressiveness by hiding the region in the awsAuthorization parameter.

Please let me know, if, after taking the above into account, you still want to update the PR and add the region to the AuthorizationMetadata.

[ThaSami]

Although the AuthorizationMetadata struct has Authorization in the name, what it really represents is an AWS identity: a role + credentials coming from pod identity or the AccessKey/SecretAccessKey pair.

Exactly - that's what sparked this idea for me. Since credentials are part of identity, obtaining them in the AWS SDK requires specifying a region as STS endpoints are regional.
that's why i believe including AwsRegion within the AuthorizationMetadata struct makes architectural sense because:

1. keeps all authentication-related data in one cohesive unit
2. Ensures the region information is always available when making credential requests
3. maintains a clear relationship between authorization data and its associated AWS region

would like to hear from @JorTurFer

[zroubalik]

I like the proposal from @ThaSami

[JorTurFer]

I like the proposal from @ThaSami

Me too :). About passing the value, I'd say that Metadata config.TriggerMetadata always has the parameter awsRegion, so we can parse it from the map directly like we do with others

[maxbog]

Ok then, I'll make the change

[maxbog]

One more question:
Now that the region will be embedded in the AuthorizationMetadata, I think I should get rid of awsConfigMetadata, as it's now redundant. Do you agree?

keda/pkg/scalers/aws/aws_common.go

Lines 42 to 45 in 68b6ca8

	type awsConfigMetadata struct {
	awsRegion string
	awsAuthorization AuthorizationMetadata
	}

[JorTurFer]

Do you agree?

yeah, makes sense!

[maxbog]

@JorTurFer @ThaSami please take another look

[JorTurFer]

Unit test are failing, could you take a look?

[maxbog]

Unit test are failing, could you take a look?

The failing tests are for prometheus scaler, and the error message is: "HTTP transport must be Google OAuth2"
TBH, I'm not sure how this is related to the changes in the PR.

[JorTurFer]

Prometheus scaler uses multiple auth providers, maybe the region change of the latest commit has had a side effect here:

keda/pkg/scalers/prometheus_scaler.go

Lines 96 to 128 in 68b6ca8

	// could be the case of azure managed prometheus. Try and get the round-tripper.
	// If it's not the case of azure managed prometheus, we will get both transport and err as nil and proceed assuming no auth.
	azureTransport, err := azure.TryAndGetAzureManagedPrometheusHTTPRoundTripper(logger, config.PodIdentity, config.TriggerMetadata)
	if err != nil {
	logger.V(1).Error(err, "error while init Azure Managed Prometheus client http transport")
	return nil, err
	}
	// transport should not be nil if its a case of azure managed prometheus
	if azureTransport != nil {
	httpClient.Transport = azureTransport
	}
	gcpTransport, err := gcp.GetGCPOAuth2HTTPTransport(config, httpClient.Transport, gcp.GcpScopeMonitoringRead)
	if err != nil && !errors.Is(err, gcp.ErrGoogleApplicationCrendentialsNotFound) {
	logger.V(1).Error(err, "failed to get GCP client HTTP transport (either using Google application credentials or workload identity)")
	return nil, err
	}
	if err == nil && gcpTransport != nil {
	httpClient.Transport = gcpTransport
	}
	awsTransport, err := aws.NewSigV4RoundTripper(config)
	if err != nil {
	logger.V(1).Error(err, "failed to get AWS client HTTP transport ")
	return nil, err
	}
	if err == nil && awsTransport != nil {
	httpClient.Transport = awsTransport
	}
	}

In any case, I've triggered the tests again

[JorTurFer]

Most probably it's returning an error here:

keda/pkg/scalers/prometheus_scaler.go

Lines 119 to 123 in 68b6ca8

	awsTransport, err := aws.NewSigV4RoundTripper(config)
	if err != nil {
	logger.V(1).Error(err, "failed to get AWS client HTTP transport ")
	return nil, err
	}

An error in that line will make fail the test although is GCP because it returns error and not the GCP transport

[maxbog]

Unit tests are passing now

[zroubalik]

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

[maxbog]

Can we merge the PR? I'd rather not be stuck in the conflict -> merge -> e2e test -> conflict death spiral ;)

[votzest]

it seems e2e tests got stuck again? otherwise can you merge this pr @zroubalik, please?