-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Provide scaler for Amazon managed service for Prometheus #5315
Conversation
Thank you for your contribution! 🙏 We will review your PR as soon as possible. While you are waiting, make sure to:
Learn more about: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some e2e tests please
hi @tomkerkhove : Added e2e tests here https://github.com/sguruvar/keda/blob/main/tests/scalers/aws/aws_managed_prometheus/aws_managed_prometheus_test.go.. I believe this is part of the PR. Please let me know for any changes/issues. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement! some comments inline
tests/scalers/aws/aws_managed_prometheus/aws_managed_prometheus_test.go
Outdated
Show resolved
Hide resolved
Incorporated review comments. |
pkg/scalers/aws_sigv4.go
Outdated
cs, err := awsCfg.Credentials.Retrieve(context.Background()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
creds := credentials.NewStaticCredentials(cs.AccessKeyID, cs.SecretAccessKey, "") | ||
signer := v4.NewSigner(creds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you just do something like:
cs, err := awsCfg.Credentials.Retrieve(context.Background()) | |
if err != nil { | |
return nil, err | |
} | |
creds := credentials.NewStaticCredentials(cs.AccessKeyID, cs.SecretAccessKey, "") | |
signer := v4.NewSigner(creds) | |
signer := v4.NewSigner(awsCfg.Credentials) |
I'm afraid in case of podIdentity what will happen with cs.AccessKeyID, cs.SecretAccessKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewSigner doesnt take awsCfg.credentials.. this works with the pod identity e2e testing for aws_managed_prometheus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to use https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/amp as it's part of skd-v2.
You can init there client with NewFromConfig
and it uses aws.Config
, which is the output of https://github.com/kedacore/keda/blob/main/pkg/scalers/aws/aws_common.go#L53
/run-e2e aws_managed_prometheus |
please, execute also: go mod tidy
go mod vendor and push the changes too. |
Pushed |
Pusehd |
Appreciate any updates on the merge.. thanks |
Please rebase your PR. We've added a new authentication way. All the conflicts are related with deps (go.mod and go.sum + vendor). There is also an small change to be done in the authentication part. We've updated this method that has to be used because it supports all the podIdentities out-of-the-box and also it manages credentials caching/sharing to reduce the amount of tokens for the same roleArn. |
go.mod
Outdated
@@ -24,9 +24,11 @@ require ( | |||
github.com/IBM/sarama v1.41.2 | |||
github.com/antonmedv/expr v1.15.3 | |||
github.com/arangodb/go-driver v1.6.0 | |||
github.com/aws/aws-sdk-go-v2 v1.21.0 | |||
github.com/aws/aws-sdk-go v1.49.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have migrated to github.com/aws/aws-sdk-go-v2
, please don't use github.com/aws/aws-sdk-go
Semgrep found 1
odd hash.Sum call flow Ignore this finding from hash-sum-without-write. |
Semgrep found 1
odd hash.Sum call flow Ignore this finding from hash-sum-without-write. |
Provide a description of what has been changed
Checklist
Fixes #2214 (comment)
Relates to #