Application Credential Support #317
Conversation
SeanMooney
left a comment
SeanMooney
left a comment
There was a problem hiding this comment.
i have not looked at he functional test but the current changes are no following the patter we use in the watcher operator.
we shoudl also have kuttl test ideally for this.
in would also like to see one of the tempest jobs use this pattern but that could be in a separate PR
| } | ||
|
|
||
| // Check for Application Credentials | ||
| templateParameters["UseApplicationCredentials"] = false |
There was a problem hiding this comment.
we normally do not use booleans for this
we isntead follow the pattern of chekcign if the parmares are empty in the template.
can you refactor this to use that pattern
in this case you shoudl generate the approate section if
"ACID" is populated
{{ if (index . "ACID") }}
...
{{ end}}
|
|
||
| // Check for Application Credentials | ||
| templateParameters["UseApplicationCredentials"] = false | ||
| if acData, err := keystonev1.GetApplicationCredentialFromSecret(ctx, r.Client, instance.Namespace, watcher.ServiceName); err != nil { |
There was a problem hiding this comment.
ok so keystone is going to provide a helper function it its api module instead of each fo the service operators using ensureSecret
this is not quite correct for watcher nova or placement because we create separate secrets for sub cr each so the top level watcher controller shoudl receive the application credential and look up the info form the keystone created secret and then populate that into the per CR secrete
so we should look this up once here
then extend createSubLevelSecret to propagate the application credential info into the service secrets.
https://github.com/openstack-k8s-operators/watcher-operator/blob/main/internal/controller/watcher_controller.go#L355-L360
https://github.com/openstack-k8s-operators/watcher-operator/blob/main/internal/controller/watcher_controller.go#L862
even if we were to ignore that pattenr which i dont think we should we would add a new fucntion to the base reconsolie instead to aovid repeating this per service contoler.
| }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
this looks incorrect to me.
we watcher secrets or other CRDs
genericly
the looping over the relevant objects and creation of reconsile request is don by
so you shoudl not be generating the reconsile reqeust hree
you would follw the same patthern as the passwordSecretFiled and return a slice of strings []string
// index passwordSecretField
if err := mgr.GetFieldIndexer().IndexField(context.Background(), &watcherv1beta1.WatcherAPI{}, passwordSecretField, func(rawObj client.Object) []string {
// Extract the secret name from the spec, if one is provided
cr := rawObj.(*watcherv1beta1.WatcherAPI)
if cr.Spec.Secret == "" {
return nil
}
return []string{cr.Spec.Secret}
}); err != nil {
return err
}
with that said as i indicated above the applcition credtial shoudl be manged at the toplevel contoler and the API/applier/descison engine should jus twhatc there input secret.
basiclly the top level contoler will watch the appcliatce credtial secret/CRD and update the subCR secrete, the subcr contoler like the water api contole only watch there secret not teh applcation credital one.
|
the current design regresses #30 by making each controller depend on the the application credential directly. |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/980f61d6994b45f090a0c385ce6ea6af ❌ openstack-meta-content-provider-master FAILURE in 35m 06s |
|
Hey @SeanMooney ! Thanks for the review. We decided today that we will refactor the app cred support in service operators to create a new CRD field for appCredSecret name. So, we will get rid of the watchers, and will use field index instead in only parent controller, as you suggested. I will prepare changes soon. |
Adds the end-to-end support for consuming Keystone ApplicationCredentials (AC) in the watcher-operator, enabling WatcherAPI, WatcherApplier, and WatcherDecisionEngine pods to use AC-based authentication when available. Signed-off-by: Veronika Fisarova <vfisarov@redhat.com>
Deydra71
force-pushed
the
appcred-support
branch
from
b7fa7ee to
a22b12b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@Deydra71: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ed381afd2cd041709b601f0b977e644f ❌ openstack-meta-content-provider-master FAILURE in 33m 55s |
2 participants
Jira: OSPRH-20524
Adds the end-to-end support for consuming Keystone ApplicationCredentials (AC) in the watcher-operator, enabling WatcherAPI, WatcherApplier, and WatcherDecisionEngine pods to use AC-based authentication when available.
API changes:
Adds an optional authentication field to the Watcher CRs:
spec.auth.applicationCredentialSecret— name of the Secret that contains the Keystone Application Credential ID and Secret (AC_ID and AC_SECRET).Reconcile behavior:
Reads
spec.auth.applicationCredentialSecretAttempts to load AC_ID / AC_SECRET from the referenced Secret (via the Keystone helper).
If the secret is missing or incomplete, it falls back to password authentication (the AppCred auth is optional, not an error).
AC_IDandAC_SECRETfields, templates AC credentials into the shared service configuration (00-default.conf)Both
[keystone_authtoken]and[watcher_clients_auth]sections in00-default.confare updated to support AC authentication with a block-based if-else structure.Depends-On: openstack-k8s-operators/keystone-operator#567