If different SecretStores bound with different authentication methods and different permissions are used in a given context, won't effectively only make one of them to work (since then, the session will be saved globally?). The origin of my comment is because we actually send the creds within the Config: *config line

to add to that: we also must accomodate for changes in the SecretStore spec: i.e. if a different secret (possibly updated) shall be used we must not re-use the old session but create a new one.

Thanks @gusfcarvalho @moolen
I have updated the PR, can you please take a look.

Also, I am not sure if makes sense to add the AWS session cache logic to DefaultJWTProvider function.

thanks, that looks good so far. I'm a little bit concerned with rolling this out as it touches the very core of the authentication mechanics with aws. I would like to hide this feature behind a feature gate so that a user has to opt-in in order to use it. Once we gather enough data that this works with all the different integration options we can promote it to being a opt-out feature. But still, before merging this we should either implement soak tests that run for a longer period of time to verify that we don't produce broken session from which we can't recover without doing a hard reboot of the controller OR doing lots of manual tests.

DefaultJWTProvider

I think i'll leave it as it is for now. It doesn't do harm if we don't cache the session there.

Sure @moolen let me know if I can help anyhow

@moolen I have update the PR for this feature to be a feature gate. Please check if PR looks good. Thanks.

hey @albertollamaso, sorry i was on vacation the last dew days. With feature gate i meant to add a command line flag, e.g. --experimental-enable-aws-session-cache or similar.
I don't want to add that to the CRD, because this is (at least for now) a experimental thing and i don't want this to be covered by the deprecation policy. By prefixing the flag with experimental it should be obvious that this is a use-it-at-your-own-risk feature :)

edit
I quickly skimmed over the cert-manager feature gate implementation, and it seems that k8s/component-base provides roughly what we need: see here and here. I think this is roughly what we need in future, but is out of scope for this PR as this will likely require a lot of refactoring

thanks @moolen for the clarification. I agreed that the feature gate implementation using k8s/component-base is out of scope of this PR.

I have update the PR adding the feature gate the way I think you are suggesting. Please feel free to comment and flag any wrong implementation you see in the PR since I am still in the process of learning go.

I am pushing for this feature to be implemented, at least as experimental at the beginning since it will save a lot of API calls for large implementations of ExternalSecrets as we currently have in our insfrastructure.

the session construction logic is duplicated here and below. Can you please change it so there's just one place where the session gets constructed?

e.g. (pseudocode)

if EnableCache {
  sess, ok := sessions[tmpSession]
  if ok {
    return sess
  }
}

// construct session..

if EnableCache {
  sessions[key] = sess
} 
return sess

Users can set extraArgs: ["--experimental-enable-aws-session-cache"], i would like to avoid adding it in the values.yaml and the extra if branch in the deployment.yaml above. (helm values is sort of part of our deprecation policy)