Skip to content

migrate aws secretsmanager and aws parameter store to go sdk v2#4484

Merged
Skarlso merged 61 commits intoexternal-secrets:mainfrom
Ilhan-Personal:ilu/mig-v2
May 27, 2025
Merged

migrate aws secretsmanager and aws parameter store to go sdk v2#4484
Skarlso merged 61 commits intoexternal-secrets:mainfrom
Ilhan-Personal:ilu/mig-v2

Conversation

@Ilhan-Personal
Copy link
Copy Markdown
Contributor

@Ilhan-Personal Ilhan-Personal commented Feb 27, 2025

Problem Statement

What is the problem you're trying to solve?

Related Issue

#4310
Fixes #...

Proposed Changes

How do you like to solve the issue and why?

Checklist

  • I have read the contribution guidelines
  • All commits are signed with git commit --signoff
  • My changes have reasonable test coverage
  • All tests pass with make test
  • I ensured my PR is ready for review with make reviewable

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
@Ilhan-Personal Ilhan-Personal changed the title migrate to go sdk v2 work in progress: migrate to go sdk v2 Feb 27, 2025
Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Mar 23, 2025

Well done, for soldering on with this. Really appreciate it!

@ivankatliarchuk
Copy link
Copy Markdown
Contributor

Maybe worth to slice a pull request 1. secretmanager 2. parameter?

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Apr 4, 2025

Sorry for the merge conflicts 🙇 I hope they aren't bad!

Other than these there will no change to AWS providers until this is done, I promise.

@Ilhan-Personal
Copy link
Copy Markdown
Contributor Author

Ilhan-Personal commented Apr 6, 2025

Heyy @Skarlso I totally understand, This is taking more time than I had anticipated mostly because work has been quite busy these past few weeks 😅

@Ilhan-Personal
Copy link
Copy Markdown
Contributor Author

Maybe worth to slice a pull request 1. secretmanager 2. parameter?

That's a good point but I need to check if there's some common code that both of these depend on. We'll split the PR after evaluating that. Thank you :)

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
@Ilhan-Personal
Copy link
Copy Markdown
Contributor Author

Thanks a lot @Skarlso !

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 24, 2025

/ok-to-test sha=7e62ccf8192e6c865106618d94790d38353f21c4

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 24, 2025

Once the tests are done, I'm going to put back enableSessionCache with a deprecation note, because it's breaking anyone's startup if it's removed suddenly. And we learned from 0.17.0 that even release notes won't help that. :P So we are going to give it a grace period of a couple releases.

@eso-service-account-app
Copy link
Copy Markdown
Contributor

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
@Skarlso Skarlso self-requested a review May 25, 2025 15:28
@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 25, 2025

Okay, so this all looks fine for me. One last test I want to perform is how about existing things.

I'll have a bunch of secrets with various tags and names and finds and dataFrom's created with old implementation. And the point is that it needs to behave the same way so that it stays backwards compatible once I switch over to v2. @Ilhan-Personal did you do some testing like that by any chance?

Also, this is a major update, so I'm wondering how to hell do we release this without causing chaos. If we really would want to be backwards compatible, it would mean duplicating somehow the provider. Basically, create a v2 provider. Take the current code. Make a new provider. Call it aws secrets manager | parameter store v2 and tell people to use that if they are adventurers. :) @gusfcarvalho what do you think? Do we just release this...? I mean it looks okay, it works the same way on the surface, and I'll perform some backwards checks ( create shit with v1, then switch over to this branch and see if things are still okay ). WDYT?

@gusfcarvalho
Copy link
Copy Markdown
Member

Tbh, tests passing, and given we didn’t change our tests that much, I think we can release this as a patch even. If something broke; then we fix it. But we have no reason to believe this will be breaking for users. It shouldn’t in almost any aspect (maybe the small api changes on pointer/non pointer?🤔).

in any case, I think this doesn’t really require a minor bump 😃

@gusfcarvalho
Copy link
Copy Markdown
Member

I know we might have some regression here and there 🤷🏽‍♂️ but tbh we cannot release based on things we don’t know.
From what we know, this should be a patch users won’t even feel

@lukaspj
Copy link
Copy Markdown

lukaspj commented May 26, 2025

If it would help move things along, we could assist with testing by rolling it into our medium sized setup (about 500 distinct deployment environments)

We would have to spend a day or two validating the changes before rolling it into production. So not sure if how that fits into your timelines.

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 26, 2025

@lukaspj That would be insanely appreciated! I tested with existing setup, but not like a larger thing with several scenarios. I can't test that. :) It would be very very helpful if you could do that please. :)

@lukaspj
Copy link
Copy Markdown

lukaspj commented May 26, 2025

Ah unfortunately, I wasn't aware that v1beta1 had been removed in v0.17 and we have a couple of thousand external secrets we need to update so I can't promise that we can do this in due time :(.

Very unfortunate

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 26, 2025

Ah, no worries then. I'll do some testing on my side then.

@Ilhan-Personal
Copy link
Copy Markdown
Contributor Author

I'll have a bunch of secrets with various tags and names and finds and dataFrom's created with old implementation. And the point is that it needs to behave the same way so that it stays backwards compatible once I switch over to v2. @Ilhan-Personal did you do some testing like that by any chance?

Heyy @Skarlso, I unfortunately did not do that but I'll be happy to run this locally and do what you mentioned sometime tomorrow if that works

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 26, 2025

Sure, we can both verify it to see that everything works fine. :)

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 27, 2025

Everything is synced...

➜ k get pushsecret,externalsecret -A
NAMESPACE   NAME                                                AGE   STATUS
default     pushsecret.external-secrets.io/pushsecret-example   26s   Synced

NAMESPACE   NAME                                                       STORETYPE     STORE                REFRESH INTERVAL   STATUS         READY
default     externalsecret.external-secrets.io/aws-secretsmanager-es   SecretStore   aws-secretsmanager   1h                 SecretSynced   True
default     externalsecret.external-secrets.io/example                 SecretStore   aws-secretsmanager   1h                 SecretSynced   True

Switching to this branch...

...

➜ k get pushsecret,externalsecret -A
NAMESPACE   NAME                                                AGE     STATUS
default     pushsecret.external-secrets.io/pushsecret-example   2m54s   Synced

NAMESPACE   NAME                                                       STORETYPE     STORE                REFRESH INTERVAL   STATUS         READY
default     externalsecret.external-secrets.io/aws-secretsmanager-es   SecretStore   aws-secretsmanager   10s                SecretSynced   True
default     externalsecret.external-secrets.io/example                 SecretStore   aws-secretsmanager   10s                SecretSynced   True

Everything remained in-sync. Yay. :)

Deleting pushsecret also removed the remote secret. ✅

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 27, 2025

Testing parameter store as well... Everything synced up. :) Nice ✅

@sonarqubecloud
Copy link
Copy Markdown

@Skarlso Skarlso merged commit 2f8b37c into external-secrets:main May 27, 2025
24 checks passed
@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 27, 2025

@Ilhan-Personal Thank you so much for the hard work! This was one amazing job! :) 🙇 I tested it as much as I can, so hopefully, we don't have any breaking changes. Nevertheless I want to release this in a minor version in 0.18.0 later. :)

Hopefully, people can now start doing some AWS stuff, and see some updates being made and new regions being available. :) 🤞

@zmudm
Copy link
Copy Markdown

zmudm commented Jun 5, 2025

Hi!
Firstly, thank you for your amazing work on this PR!
I saw that it's planned to be included in version 0.18.0.
Do you have an estimated timeline for when this release might happen?

@Ilhan-Personal @Skarlso

@gusfcarvalho
Copy link
Copy Markdown
Member

Somewhere next week, hopefully. We need to fix a very weird release bug before we release any other versions.

so you should expect a 0.18.0-rc1 (and maybe rc2, rc3, rc4) before we actually cut 0.18.0

@yo-ga
Copy link
Copy Markdown
Contributor

yo-ga commented Jun 5, 2025

Fixed #3319, too.

pepordev pushed a commit to pepordev/external-secrets that referenced this pull request Jun 11, 2025
…rnal-secrets#4484)

* initial commit

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* some fixes

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* fix

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* make changes to private functions

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* auth

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* fix

* change fakses

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* satisfy interface

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* rename function calls

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* revert

* Migrate parameterstore to aws sdk v2

* fixes in tests

* Fix parameter store tests

* attempt to migrate auth

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* attempt to migrate auth

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* Partial Auth migration

* change token fetcher logic

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* migrate token fetch logic

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* nit

* migrate assumeroler

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* migrate auth_test

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* migrate auth_test

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* Wrap up auth & resolvers

* migrate ecr

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* migrate sts

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* migrate e2e

* migrate aws error

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* small change

* update go.mod

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* fix auth tests

* revert V1beta1 spec changes

* attempt to fix unit tests

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* use const for resourcenotfoundexception

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* lint

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* more lint fixes

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* more lint fixes

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* remove session cache in auth.go

* fix lint

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* fix lint

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* commit changes made to doc

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* Fix AWS provider tests for SDK v2 migration

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* check if there's a direct fake function to call in fake client

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* fix

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* address failing test

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* nit

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>

* go mod tidy

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

* make check-diff

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

* fixed the test

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

* fix e2e test case configuration

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

* session cache option is removed

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

* using pointers instead

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

* fix using the right name type for the tag parameter

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

* refactor to remove the two complexity warnings

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

* put the experimental flag back and mark it as deprecated

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

---------

Signed-off-by: SYSHIL <ilhan.syed@gmail.com>
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Co-authored-by: AkshithaRajavel <71207279+AkshithaRajavel@users.noreply.github.com>
Co-authored-by: AkshithaRajavel <2407akshi@gmail.com>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: Pedro Parra Ortega <pedro.parraortega@enreach.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.

8 participants