-
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
KEDA Hashicorp vault service account token request #6446
base: main
Are you sure you want to change the base?
KEDA Hashicorp vault service account token request #6446
Conversation
Signed-off-by: Bojan Zelic <[email protected]>
…ce-account-token-request
Signed-off-by: Bojan Zelic <[email protected]>
}, | ||
} | ||
|
||
if err := vh.k8sClient.SubResource("token").Create(context.TODO(), sa, tokenRequest); err != nil { |
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.
Signed-off-by: Bojan Zelic <[email protected]>
Signed-off-by: Bojan Zelic <[email protected]>
Could you also add some e2e tests? |
if err = vh.k8sClient.Get(context.Background(), saName, sa); err != nil { | ||
if apierrors.IsNotFound(err) { | ||
return token, errors.New(fmt.Sprintf("Failed to retreive service account name: %s namespace: %s", saName.Name, saName.Namespace)) | ||
} |
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.
Here should return other errs as well.
if err = vh.k8sClient.Get(context.Background(), secretName, secret); err != nil { | ||
if apierrors.IsNotFound(err) { | ||
return token, errors.New(fmt.Sprintf("Failed to retreive secret for service account name: %s namespace: %s", secretName.Name, secretName.Namespace)) | ||
} |
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.
Here should return other errs as well.
a8b484a
to
a5afafe
Compare
Signed-off-by: Bojan Zelic <[email protected]>
a5afafe
to
bfa2613
Compare
@SpiritZhou I fixed the PR from your comments and updated the helmchart & documentation 🙏 please take a look whenever you get a chance. |
/run-e2e hashicorp |
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.
LGTM! The only point I have is the one related with the other PR adding support for the same API for other usage
if err != nil { | ||
return token, err | ||
if vh.vault.Credential.ServiceAccountName != "" { | ||
// generate token from namespace |
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.
I think that this code is quite related with this other PR adding the same support in other part. Maybe we should merge one of them and rebase the other, unifying the code. @wozniakjan @zroubalik ?
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.
Agree!
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.
I'm assuming this is the PR you guys are talking about? #6272 - it's somewhat different usage from what I see; This PR exposes service account JWT to scalers, vs my PR exposes service account JWT to hashicorp vault.
Is the ask here to make it compatible with that API? Just thinking what would be a good way to expose this to users.
ex... instead of:
apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
spec:
hashiCorpVault:
address: {hashicorp-vault-address}
credential:
serviceAccountName: default
do something like:
apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
spec:
boundServiceAccountToken:
- serviceAccountName: default
parameter: null
credential:
useBoundServiceAccountToken: true
Allows users to authenticate to vault via a service account in the scaledObject's namespace;
ex:
would use the JWT token from the
default
service account in themynamespace
namespaceThis allows users to set more fine-grained permissions in vault.
Checklist
Fixes # #6153
Relates to #