-
Notifications
You must be signed in to change notification settings - Fork 32
AppCred service operator support #248
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
base: main
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Deydra71 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
controllers/barbican_controller.go
Outdated
| templateParameters["ACID"] = string(secret.Data["AC_ID"]) | ||
| templateParameters["ACSecret"] = string(secret.Data["AC_SECRET"]) | ||
| Log.Info("Using ApplicationCredentials auth") | ||
| } |
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.
How do we handle the case where we are unable to retrieve the ACSecret? Looks like we simply continue and use password auth instead. But maybe we should requeue? Or at the very least log the error?
| if res.RequeueAfter > 0 { | ||
| return res, nil | ||
| } | ||
| configVars["secret-"+secretName] = env.SetValue(hashAC) |
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.
Why do we need this logic? It appears to be different from all the other secret checks above. I'm referring to lines 639-642.
|
|
||
| Log.Info(fmt.Sprintf("[API] Got secrets '%s'", instance.Name)) | ||
|
|
||
| // check for ApplicationCredential |
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.
Should we be checking for both the admin secret and also the application credential (if it exists)?
Seems to me that if you are using appCreds - then you no longer need to watch the admin secret - and no longer need to reconcile if it changes.
Maybe the logic here should be more like:
check if app cred exists
if app_cred_exists {
get app_cred_secret()
if ! exists requeue
verify app_cred_secret
} else {
verify_admin_secre
}
Feels like this is long enough to warrant a new method verify_service_credentials()
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.
Also, the check below needs to be in barbican_controller too. See https://github.com/openstack-k8s-operators/barbican-operator/blob/main/controllers/barbican_controller.go#L257
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 added the verifyApplicationCredentials() func that implements this, see the comment below
| } | ||
| configVars["secret-"+secretName] = env.SetValue(hashAC) | ||
| } | ||
|
|
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.
Same comments as above, but actually, we should think about whether we need this logic at all. I don't think that the barbican-keystone-listener actually talks to keystone or uses the keystone creds. In which case, maybe we don't need to verify these parameters at all.
| return res, nil | ||
| } | ||
| configVars["secret-"+secretName] = env.SetValue(hashAC) | ||
| } |
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.
Same comments as above, but actually, we should think about whether we need this logic at all. I don't think that the barbican-worker actually talks to keystone or uses the keystone creds. In which case, maybe we don't need to verify these parameters at all.
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
|
This change depends on a change that failed to merge. Change openstack-k8s-operators/keystone-operator#567 is needed. |
293d380 to
d041d8c
Compare
c80184e to
cf5f6cb
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/6fce923fcd80456aa51169a94b122b46 ❌ openstack-k8s-operators-content-provider FAILURE in 5m 40s |
|
Update: Added missing Note: Edit: worker and KL pods don't restart, because their controllers don't add AC to |
d9a3d2e to
0532383
Compare
0532383 to
f70bfe9
Compare
f70bfe9 to
506faa1
Compare
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
506faa1 to
cbc0271
Compare
03426c5 to
4d423e7
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/6b11f1080b6f447b85a4c0e86b3b923d ❌ openstack-k8s-operators-content-provider FAILURE in 9m 07s |
d266335 to
cdeadd9
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/e385cc4d105f4ba1a97d159f49487abe ❌ openstack-k8s-operators-content-provider FAILURE in 9m 33s |
cdeadd9 to
023b322
Compare
023b322 to
9c65a73
Compare
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
9c65a73 to
46355b1
Compare
Jira: OSPRH-16622
This PR adds end-to-end support for consuming Keystone ApplicationCredentials (AC) in the Barbican operator, enabling Barbican API pods to use AC-based authentication when available.
Reconcile:
On each reconcile, the Barbican API controller checks for an AC Secret (
ac-{service}-secret) using theGetApplicationCredentialFromSecret()helper from keystone-operator API:AC_IDandAC_SECRETfields, templates AC credentials into Barbican configurationconfigVarsto trigger rolling updates when credentials rotateDepends-On: openstack-k8s-operators/keystone-operator#567