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.

Sign up for GitHub

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

Jump to bottom

Add support aws secretmanager authentication #5162

Merged
merged 75 commits into from
Jan 12, 2024
Merged

Add support aws secretmanager authentication #5162

merged 75 commits into from
Jan 12, 2024

Conversation

geoffrey1330
Copy link
Contributor

@geoffrey1330 geoffrey1330 commented Nov 7, 2023 •
edited
Loading

Provide a description of what has been changed

Checklist

Fixes #4628

Relates to #

@geoffrey1330 geoffrey1330 requested a review from a team as a code owner November 7, 2023 09:00
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 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:

@semgrep-app semgrep-app
Copy link

semgrep-app bot commented Nov 7, 2023

Semgrep found 6 wrong-wrapped-error findings:

Semgrep found a possible error wrong wrapped.

Ignore this finding from wrong-wrapped-error.

@geoffrey1330 geoffrey1330 force-pushed the feat/support_aws_secretmanager_authentication branch from a25b5dd to 7e75b9b Compare November 7, 2023 10:51
@geoffrey1330 geoffrey1330 changed the title Feat/support aws secretmanager authentication Add support aws secretmanager authentication Nov 7, 2023
@geoffrey1330
Copy link
Contributor Author

Hi @JorTurFer could you please run /run-e2e

@zroubalik
Copy link
Member

zroubalik commented Nov 27, 2023 •
edited by github-actions bot
Loading

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

@zroubalik
Copy link
Member

unit test fail

--- FAIL: TestEnvWithRestrictSecretAccess (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_key (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x11fdf54]

goroutine 258 [running]:
testing.tRunner.func1.2({0x1361320, 0x2728e20})
	/usr/local/go/src/testing/testing.go:1526 +0x1c8
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1529 +0x364
panic({0x1361320, 0x2728e20})
	/usr/local/go/src/runtime/panic.go:884 +0x1f4
github.com/kedacore/keda/v2/pkg/scaling/resolver.resolveSecretValue({0x1a21570, 0x400004a090}, {0xffff8c521fe8, 0x40007827d0}, {{0x1a246b8?, 0x4000c32740?}, 0x0?}, 0x4000c82fc0, {0x156f3df, 0xb}, ...)
	/__w/keda/keda/pkg/scaling/resolver/scale_resolvers.go:467 +0xd4
github.com/kedacore/keda/v2/pkg/scaling/resolver.resolveEnv({0x1a21570, 0x400004a090}, {0xffff8c521fe8, 0x40007827d0}, {{0x1a246b8?, 0x4000c32740?}, 0x599?}, 0x40005e5080, {0x1573c35, 0xe}, ...)
	/__w/keda/keda/pkg/scaling/resolver/scale_resolvers.go:354 +0x2f4
github.com/kedacore/keda/v2/pkg/scaling/resolver.TestEnvWithRestrictSecretAccess.func1(0x4000c86820)
	/__w/keda/keda/pkg/scaling/resolver/scale_resolvers_test.go:712 +0x174
testing.tRunner(0x4000c86820, 0x4000c326c0)
	/usr/local/go/src/testing/testing.go:1576 +0x104
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1629 +0x370
FAIL	github.com/kedacore/keda/v2/pkg/scaling/resolver	0.152s

@geoffrey1330
Copy link
Contributor Author

unit test fail

--- FAIL: TestEnvWithRestrictSecretAccess (0.00s)
    --- FAIL: TestEnvWithRestrictSecretAccess/env_reference_secret_key (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x11fdf54]

goroutine 258 [running]:
testing.tRunner.func1.2({0x1361320, 0x2728e20})
	/usr/local/go/src/testing/testing.go:1526 +0x1c8
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1529 +0x364
panic({0x1361320, 0x2728e20})
	/usr/local/go/src/runtime/panic.go:884 +0x1f4
github.com/kedacore/keda/v2/pkg/scaling/resolver.resolveSecretValue({0x1a21570, 0x400004a090}, {0xffff8c521fe8, 0x40007827d0}, {{0x1a246b8?, 0x4000c32740?}, 0x0?}, 0x4000c82fc0, {0x156f3df, 0xb}, ...)
	/__w/keda/keda/pkg/scaling/resolver/scale_resolvers.go:467 +0xd4
github.com/kedacore/keda/v2/pkg/scaling/resolver.resolveEnv({0x1a21570, 0x400004a090}, {0xffff8c521fe8, 0x40007827d0}, {{0x1a246b8?, 0x4000c32740?}, 0x599?}, 0x40005e5080, {0x1573c35, 0xe}, ...)
	/__w/keda/keda/pkg/scaling/resolver/scale_resolvers.go:354 +0x2f4
github.com/kedacore/keda/v2/pkg/scaling/resolver.TestEnvWithRestrictSecretAccess.func1(0x4000c86820)
	/__w/keda/keda/pkg/scaling/resolver/scale_resolvers_test.go:712 +0x174
testing.tRunner(0x4000c86820, 0x4000c326c0)
	/usr/local/go/src/testing/testing.go:1576 +0x104
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1629 +0x370
FAIL	github.com/kedacore/keda/v2/pkg/scaling/resolver	0.152s

I will look into it.

@geoffrey1330
Copy link
Contributor Author

Hi @zroubalik could you please rerun the /run-e2e as i have a made some fix.

@zroubalik
Copy link
Member

zroubalik commented Nov 27, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer mentioned this pull request Nov 27, 2023
Closed
@zm-alex
Copy link

zm-alex commented Nov 27, 2023

@geoffrey1330 thanks for the hardwork! looking forward for this feature to be available

@JorTurFer
Copy link
Member

JorTurFer commented Nov 27, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member

JorTurFer commented Nov 28, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member

JorTurFer commented Nov 28, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member

JorTurFer commented Nov 28, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member

JorTurFer commented Nov 28, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member

JorTurFer commented Nov 29, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member

JorTurFer commented Nov 29, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member

JorTurFer commented Nov 29, 2023 •
edited by github-actions bot
Loading

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

@JorTurFer
Copy link
Member

JorTurFer commented Nov 30, 2023 •
edited by github-actions bot
Loading

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

@zm-alex
Copy link

zm-alex commented Dec 4, 2023

@JorTurFer @zroubalik Any chance you could run e2e? Thanks!

@zroubalik
Copy link
Member

zroubalik commented Dec 4, 2023 •
edited by github-actions bot
Loading

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

@geoffrey1330
Copy link
Contributor Author

Hi @JorTurFer @zroubalik could you please review my PR.

JorTurFer
JorTurFer reviewed Dec 8, 2023
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, but you used aws-sdk-go and we are using aws-sdk-go-v2. Could you update the code to use the new SDK?
Could you also add an e2e test case (split from current) which uses podIdentity instead of credentials? The code will be almost the same, so just copying and pasting you could have the majority of the work done

apis/keda/v1alpha1/triggerauthentication_types.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/scaling/resolver/aws_secretManager_handler.go Outdated Show resolved Hide resolved
pkg/scaling/resolver/scale_resolvers.go Outdated Show resolved Hide resolved
tests/secret-providers/aws_secretmanager/aws_secretmanager_test.go Outdated Show resolved Hide resolved
tests/secret-providers/aws_secretmanager/aws_secretmanager_test.go Outdated Show resolved Hide resolved
tests/secret-providers/aws_secretmanager/aws_secretmanager_test.go Outdated Show resolved Hide resolved
geoffrey1330 and others added 7 commits January 10, 2024 20:01
@geoffrey1330 @JorTurFer
Update CHANGELOG.md
f5d631c 
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
@geoffrey1330
Merge branch 'main' into feat/support_aws_secretmanager_authentication
1a0863f
@geoffrey1330
Update triggerauthentication_types.go
80119b6 
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
@geoffrey1330
Update aws_secretmanager_handler.go
8eae174 
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
@geoffrey1330
Update aws_secretmanager_test.go
a0e30d7 
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
@geoffrey1330
Add support for aws secret manager authentication
ef7ae28 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@geoffrey1330
Add support for aws secret manager authentication
0e17801 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@JorTurFer
Copy link
Member

JorTurFer commented Jan 10, 2024 •
edited by github-actions bot
Loading

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

@geoffrey1330
Add support for aws secret manager authentication
8b4af1b 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@JorTurFer
Add unit tests
7918898 
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Add unit tests
59969b0 
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member

JorTurFer commented Jan 11, 2024 •
edited by github-actions bot
Loading

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

JorTurFer
JorTurFer approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Let's wait until the documentation is ready to merge

zroubalik
zroubalik reviewed Jan 12, 2024
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

There are just a few minor nits in go imports and code comments. We can go ahead once we fix these.

go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/scaling/resolver/scale_resolvers.go Show resolved Hide resolved
pkg/scaling/resolver/aws_secretmanager_handler.go Show resolved Hide resolved
pkg/scaling/resolver/aws_secretmanager_handler.go Show resolved Hide resolved
@geoffrey1330
Add support for aws secret manager authentication
0e57fd8 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@geoffrey1330
Add support for aws secret manager authentication
d235635 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@geoffrey1330
Add support for aws secret manager authentication
c472ce3 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
zroubalik
zroubalik reviewed Jan 12, 2024
View reviewed changes
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@geoffrey1330
Add support for aws secret manager authentication
540a26d 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@geoffrey1330
Add support for aws secret manager authentication
cc96980 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@geoffrey1330
Add support for aws secret manager authentication
7b7c4d9 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@geoffrey1330
Copy link
Contributor Author

Hi @zroubalik I've fixed all suggested changes could you please review the PR again.

zroubalik
zroubalik reviewed Jan 12, 2024
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@geoffrey1330
Add support for aws secret manager authentication
8dc32d7 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented Jan 12, 2024 •
edited by github-actions bot
Loading

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

zroubalik
zroubalik approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Great job @geoffrey1330

Once all comments in docs PR kedacore/keda-docs#1289 are resolved, we can go ahead and merge this PR.

@geoffrey1330
Copy link
Contributor Author

Looking good! Great job @geoffrey1330

Once all comments in docs PR kedacore/keda-docs#1289 are resolved, we can go ahead and merge this PR.

All suggested comment resolved.

@zroubalik zroubalik merged commit ef23ace into kedacore:main Jan 12, 2024
19 of 21 checks passed
sguruvar pushed a commit to sguruvar/keda that referenced this pull request Jan 15, 2024
@geoffrey1330 @JorTurFer @sguruvar
Add support aws secretmanager authentication (kedacore#5162)
dd24cc2 
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Co-authored-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

@JorTurFer JorTurFer JorTurFer approved these changes

@zroubalik zroubalik zroubalik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Authentication: support AWS secret manager
4 participants
@geoffrey1330 @zroubalik @zm-alex @JorTurFer