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. |
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 |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ed381afd2cd041709b601f0b977e644f ❌ openstack-meta-content-provider-master FAILURE in 33m 55s |
| description: APITimeout for Route and Apache | ||
| minimum: 10 | ||
| type: integer | ||
| auth: |
There was a problem hiding this comment.
im not sure this is the design we want
this is workable but it imples that each of the watcher components could have different credentials
the WatcherSpecCore
should have the new auth section like the database account fields
but the sub crds shoudl not require it.
the top level watcher contoler will extract teh applciation credtials secret data and embded it in the secrete it passes to the other watcher CRs.
so the other crs should not need to be modifeded.
not that even if we were to add the auth filed to the sub crds you have not
extened the WatcherAPITemplate or WatcherSubCrsTemplate
https://github.com/openstack-k8s-operators/watcher-operator/blob/main/api/v1beta1/watcherapi_types.go#L96-L115
https://github.com/openstack-k8s-operators/watcher-operator/blob/main/api/v1beta1/common_types.go#L169
so there is no way for the openstack operator or end user to pass the auth parmater.
the Watcher CRD is the public interface to the watcher operator the CRDs you are modifying in this PR are the internal crs that the watcher contoler creates based on the templates in the watcher CRD.
so even if we were to proceed with adding the Auth to each of the internal CRDs this pr is incomplete.
There was a problem hiding this comment.
Agreed on the design concern. I centralized it, so that the AC is managed only by top level CR. If we ever want per-cell or per-component AppCreds , that could eventually be a separate design discussion and API change.
Deydra71
force-pushed
the
appcred-support
branch
from
a22b12b to
74d0cda
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. |
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 <[email protected]>
Deydra71
force-pushed
the
appcred-support
branch
from
74d0cda to
2486fc8
Compare
|
@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. |
| DeferCleanup(k8sClient.Delete, ctx, acSecret) | ||
| }) | ||
|
|
||
| It("renders watcher.conf using v3applicationcredential auth", func() { |
There was a problem hiding this comment.
this is not really correct.
this file shoudl not assert the output of the other contorlers i.e. the config secrets.
in the watcher_controler fucntional tests we shoudl assert that the applcation credetial data is put in the secrete that is generated by the watcher controller we can also make assertions related to the CRs we create but not the sideffect fo those crs.
separately in the waterapi controler env tests we shoudl assert that it generate the relevant config files with the approated content if the secrete contains the applciation credentials.
i.e. you shoudl add a version of this with applciation credentials.
https://github.com/openstack-k8s-operators/watcher-operator/blob/main/test/functional/watcherapi_controller_test.go#L264
addtionally it woudl be nice to have a reconfiguartion test for both the topo level contoler and the service cofntolers.
we need to validate that we can trivially adopt or discontinue useing applcaition credetails on exsting deployments and rotate them as well as part of this testing.
i.e. make sure that when you update teh appclation credtial refence (add it to a existign CR, remove it form a cr and update the content) that the apprate values propagate to the service secrets and configs.
nova has a dedeicated test modeul for reconfiguraiton tests
but we can inline them into the exitsiting ones too it just requirest that update the cr in the test.
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ffe0922c30104f57a5c97bb6a380b416 ❌ openstack-meta-content-provider-master FAILURE in 30m 41s |
3 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