rbd: create token and use it for vault SA everytime possible #3377
Conversation
saiprashanth173
force-pushed
the
issue-3360
branch
from
de14a71
to
32dd191
Compare
There was a problem hiding this comment.
I think we would be completely missing the err which may have happened during createToken.
if it was something that would be helpful.
I'd prefer we make the requested changes, so we can capture both the errors if they occur,
wdyt @Madhu-1 @saiprashanth173 ?
| token, err := kms.createToken(sa, c) | ||
| if err == nil { | ||
| return token, nil | ||
| } |
There was a problem hiding this comment.
| token, err := kms.createToken(sa, c) | |
| if err == nil { | |
| return token, nil | |
| } | |
| token, createTokenErr := kms.createToken(sa, c) | |
| if createTokenErr == nil { | |
| return token, nil | |
| } |
There was a problem hiding this comment.
not sure what the change above will cover. If we want to be more specific. Continue to fetch token from the SA only if kubernetes doesn't support TokeRequest API else return all other errors.
There was a problem hiding this comment.
apologies, the other part of the requested changes was not posted by mistake.
There was a problem hiding this comment.
Great catch! Yes it makes sense to include this error as part of the final error. I updated the code to include it in the error being returned.
internal/kms/vault_sa.go
Outdated
| @@ -310,7 +314,7 @@ func (kms *vaultTenantSA) getToken() (string, error) { | |||
| } | |||
| } | |||
|
|
|||
| return kms.createToken(sa, c) | |||
| return "", fmt.Errorf("failed to find token in ServiceAccount %s/%s", kms.Tenant, kms.tenantSAName) | |||
There was a problem hiding this comment.
| return "", fmt.Errorf("failed to find token in ServiceAccount %s/%s", kms.Tenant, kms.tenantSAName) | |
| err = fmt.Errorf("failed to find token in ServiceAccount %s/%s", kms.Tenant, kms.tenantSAName) | |
| if createTokenErr != nil { | |
| err = fmt.Errorf(%w and failed to create token: %w",err, createTokenErr) | |
| } | |
| return "", err |
There was a problem hiding this comment.
We only reach here if createTokenErr is not nil. I felt if here is redundant. Included it as part of final error.
| token, err := kms.createToken(sa, c) | ||
| if err == nil { | ||
| return token, nil | ||
| } |
There was a problem hiding this comment.
apologies, the other part of the requested changes was not posted by mistake.
use TokenRequest API by default for vault SA even with K8s versions < 1.24 Signed-off-by: Prashanth Dintyala <vdintyala@nvidia.com>
saiprashanth173
force-pushed
the
issue-3360
branch
from
32dd191
to
ed0ea1a
Compare
mergify
bot
dismissed
Madhu-1’s stale review
Pull request has been modified.
Signed-off-by: Prashanth Dintyala vdintyala@nvidia.com
Describe what this PR does
Create short lived service account tokens and use them for vault SA approach even for K8s version <1.24; fall back to using token obtained from services account's secret reference (if exists).
Related issues
Fixes: #3360
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>: retest the<job-name>after unrelatedfailure (please report the failure too!)
/retest all: run this in case the CentOS CI failed to start/report any testprogress or results