Application Credential Support#317
Application Credential Support#317openshift-merge-bot[bot] merged 1 commit intoopenstack-k8s-operators:mainfrom
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
|
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. |
Deydra71
force-pushed
the
appcred-support
branch
from
74d0cda to
2486fc8
Compare
| 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 |
Deydra71
force-pushed
the
appcred-support
branch
from
2486fc8 to
cc0dc05
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/385cb1d8da464412bca0e2328fa52557 ❌ openstack-meta-content-provider-master FAILURE in 35m 55s |
| // +kubebuilder:validation:Optional | ||
| // +operator-sdk:csv:customresourcedefinitions:type=spec | ||
| // Auth - Parameters related to authentication | ||
| Auth AuthSpec `json:"auth,omitempty"` |
There was a problem hiding this comment.
I'd say we don't need this in the watcherAPITemplate, only in the WatcherSPECCore. The watcher operator should take care of making available the required info to the 2nd level controllers via config secret.
There was a problem hiding this comment.
correct we do not.
i want to avoid this in the sub CRs to reduce load on the k8s api/etd
if the sub CRs have the auth spec each of the contole need to monitor the index of Authspec for change which significanlty increase the load.
if only the top levell watcher contoler is monitoring the AuthSpec then that is 1 contoelr per proejct which is much better.
the service contoelr for the api/applier/decsion enginew will just continut ot monitor the config sectreate that they are already tracking so no increased load is created.
amoralej
left a comment
amoralej
left a comment
There was a problem hiding this comment.
Thanks for your functional testing coverage. I'd like to also include AC in at least one of the existing kuttl test scenarios in the repo.
Actually, we may also include it in one of the scenario testing later.
| auth: | ||
| description: Auth - Parameters related to authentication | ||
| properties: | ||
| applicationCredentialSecret: | ||
| description: ApplicationCredentialSecret - Secret containing | ||
| Application Credential ID and Secret | ||
| type: string | ||
| type: object |
There was a problem hiding this comment.
Why to add auth to have only one parameter? we could have applicationCredentialSecret as top level param. Do we expect more auth parameters? is for consistency with some other operator?
There was a problem hiding this comment.
Hello @amoralej Thank you for the reviews, I'm working on them now, I understand now how the parent watcher controller is intertwined with the children services.
It was suggestred by @stuggi to nest the AppCred secret field under the Auth field for future purposes. because otherwise it'd be only possible to add new possible Auth parameters with new API version.
| // AuthSpec defines authentication parameters | ||
| type AuthSpec struct { | ||
| // +kubebuilder:validation:Optional | ||
| // +operator-sdk:csv:customresourcedefinitions:type=spec | ||
| // ApplicationCredentialSecret - Secret containing Application Credential ID and Secret | ||
| ApplicationCredentialSecret string `json:"applicationCredentialSecret,omitempty"` | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this is not needed?
| // Add Application Credential data if configured | ||
| if instance.Spec.Auth.ApplicationCredentialSecret != "" { | ||
| acSecret := &corev1.Secret{} | ||
| key := types.NamespacedName{Namespace: instance.Namespace, Name: instance.Spec.Auth.ApplicationCredentialSecret} | ||
| if err := r.Client.Get(ctx, key, acSecret); err != nil { | ||
| if !k8s_errors.IsNotFound(err) { | ||
| Log.Error(err, "Failed to get ApplicationCredential secret", "secret", key) | ||
| } | ||
| } else { | ||
| acID, okID := acSecret.Data[keystonev1.ACIDSecretKey] | ||
| acSecretData, okSecret := acSecret.Data[keystonev1.ACSecretSecretKey] | ||
| if okID && len(acID) > 0 && okSecret && len(acSecretData) > 0 { | ||
| data["ACID"] = string(acID) | ||
| data["ACSecret"] = string(acSecretData) | ||
| Log.Info("Using ApplicationCredentials auth (centralized from parent Watcher CR)", "secret", key) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
For consistency with how the rest of secrets are consumed in watcher_controller, I'd prefer to do this differently:
- Get the AplicationCrecentialSecret in the Reconcile function, right after using ensureSecret (similar to transporturlSecret).
watcher-operator/internal/controller/watcher_controller.go
Lines 303 to 317 in cc0dc05
- Add new parameter AplicationCrecentialSecret to function createSubLevelSecret with the secret object.
- Add the ACID and ACSecret information to the secret in createSubLevelSecret
That way, we check that the secret exists before processing any configuration, and we can exit early.
There was a problem hiding this comment.
@amoralej I used the keystone function GetApplicationCredentialFromSecret instead of the generic ensureSecret that could block the reconcile. The app cred is optional, so if it's missing services cna use password auth and adopt only app cred when available.
| // Application Credential secret watching function | ||
| acSecretFn := func(_ context.Context, o client.Object) []reconcile.Request { | ||
| result := []reconcile.Request{} | ||
|
|
||
| // Check if this is a watcher AC secret by name pattern (ac-watcher-secret) | ||
| expectedSecretName := keystonev1.GetACSecretName(watcher.ServiceName) | ||
| if o.GetName() == expectedSecretName { | ||
| // get all WatcherApplier CRs in this namespace | ||
| watcherAppliers := &watcherv1beta1.WatcherApplierList{} | ||
| listOpts := []client.ListOption{ | ||
| client.InNamespace(o.GetNamespace()), | ||
| } | ||
| if err := r.Client.List(context.Background(), watcherAppliers, listOpts...); err != nil { | ||
| return nil | ||
| } | ||
|
|
||
| // Enqueue reconcile for all WatcherApplier instances | ||
| for _, cr := range watcherAppliers.Items { | ||
| result = append(result, reconcile.Request{ | ||
| NamespacedName: types.NamespacedName{ | ||
| Namespace: o.GetNamespace(), | ||
| Name: cr.Name, | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
There was a problem hiding this comment.
No need to do this. The ACID and ACSecret is consumed by the applier controller via the SubLevel Secret which is managed by the watcher_controller. All the second level controllers (api, applier and decisionengine) are already monitoring the sublevel secret and will trigger a reconcile if it's changed.
| Watches( | ||
| &corev1.Secret{}, | ||
| handler.EnqueueRequestsFromMapFunc(acSecretFn), | ||
| builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}), | ||
| ). |
There was a problem hiding this comment.
No needed, see my previous comment.
| Eventually(func(g Gomega) { | ||
| watcher := GetWatcher(watcherTest.Instance) | ||
| watcher.Spec.Auth.ApplicationCredentialSecret = appCredSecretName | ||
| g.Expect(k8sClient.Update(ctx, watcher)).Should(Succeed()) | ||
| }, timeout, interval).Should(Succeed()) |
There was a problem hiding this comment.
You are creating a Watcher instance without AC and then, creating the AC and updating the Watcher. Is that what you want to test here? or you'd prefer to create the Watcher with the AC from scratch?
There was a problem hiding this comment.
Yes, that is intended, because the service password is needed first to create the AC in Keystone. The service password is retrieved from the service CR by openstack-operator, then openstack-operator creates the AC CR, which is watched by keystone-operator and based on which keystone-operator creates specific AC and the AC kubernetes Secret, which is consumed hby the service operators.
There was a problem hiding this comment.
we will need to supprot creating the water CRD with AC crediatlas without first deploying it with a password too.
so we shoudl test both
this is testign updatign to using AC form password but we also need to supprot greanfiled deployment with AC for day one.
Deydra71
force-pushed
the
appcred-support
branch
from
cc0dc05 to
ae27d94
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/b04e1c74303c4df29cd4616873737a98 ❌ openstack-meta-content-provider-master FAILURE in 30m 27s |
Deydra71
force-pushed
the
appcred-support
branch
from
ae27d94 to
2fa23d1
Compare
|
The fix for the openstack-meta-content-provider-master job is openstack-k8s-operators/keystone-operator#662 |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/cbd2af7d40d845e981c1d8800971c24f ❌ openstack-meta-content-provider-master FAILURE in 29m 13s |
| acSecret = &corev1.Secret{ | ||
| Data: map[string][]byte{ | ||
| keystonev1.ACIDSecretKey: []byte(acData.ID), | ||
| keystonev1.ACSecretSecretKey: []byte(acData.Secret), | ||
| }, | ||
| } |
There was a problem hiding this comment.
Given that GetApplicationCredentialFromSecret returns a ApplicationCredentialData object, we may pass it directly to the createSubLevelSecret function, no need to make it into a secret. Actually, you may create a map or something like that if you prefer.
| key := types.NamespacedName{Namespace: instance.Namespace, Name: instance.Spec.Auth.ApplicationCredentialSecret} | ||
| if err := r.Client.Get(ctx, key, secret); err != nil { | ||
| if !k8s_errors.IsNotFound(err) { | ||
| Log.Error(err, "Failed to get ApplicationCredential secret", "secret", key) | ||
| } |
There was a problem hiding this comment.
For consistency, you could do the same here, add an acSecret parameter to generateServiceConfigDBJobs instead of reading the secret again.
| // +kubebuilder:validation:Optional | ||
| // +operator-sdk:csv:customresourcedefinitions:type=spec | ||
| // ApplicationCredentialSecret - Secret containing Application Credential ID and Secret | ||
| ApplicationCredentialSecret string `json:"applicationCredentialSecret,omitempty"` |
There was a problem hiding this comment.
According to the current implementation of GetApplicationCredentialFromSecret in openstack-k8s-operators/keystone-operator#567 the, name of the secret is hardcoded to ac-{servicename}-secret , so there is no need to be able to parametrize it. We may need only a boolean ApplicationCredentialEnabled to specify that services should use an AC and fail if the secret do not exist.
There was a problem hiding this comment.
isnt that a problem for roation.
the expected way to roate the applciation credital is to update the name of the Applaication credential CR or i guess the secret
ideally i owuld have prefered to have teh applciate credital CR name not the secret name here but either works.
hardcodeing however does not.
we have to be able to create a new application credential and update this to poitn to it to do the rotation.
that less imporant for watcher then for nova or other service on the edpm node because we need to do a edpm deployment and should not allow the applciation credital to be deleted until that happens.
There was a problem hiding this comment.
said another way i would expect to take the name of the CR here or the secreat and hardcodeing it is not somethign that shoudl be done in keystone. so i belive this crd change is valid.
removign it an realying on hardcoded name i dont think woudl be
There was a problem hiding this comment.
The rotation mechanism works via the KeystoneApplicationCredential CR, not by changing secret names. Keystone-oeprator automatically rotates when the App cred is within the grace period gracePeriodDays.
Then it creates new AC in keystone, updates the same Secret with the new credentials. And old credential stays valid in keystone until its natural expiration.
There's always option to trigger rotation manually by patching expiration time to the past date, eg:
oc patch -n openstack keystoneapplicationcredential ac-barbican \
--type=merge --subresource=status \
-p '{"status":{"expiresAt":"2001-05-19T00:00:00Z"}}'
There was a problem hiding this comment.
we cant asume its safe to deallocate teh old applcation credtial because edpm nodes cannot be rotated automiaticlly like that. we need to do a dataplane deployment
and it woudl be a security issue in my view to leak the old appclation credital without deleitng it in keystone ocne the refence to it is remvoed in openshift
| // Default ApplicationCredentialSecret to standard AC secret name if not specified | ||
| if spec.Auth.ApplicationCredentialSecret == "" { | ||
| spec.Auth.ApplicationCredentialSecret = keystonev1.GetACSecretName("watcher") | ||
| } |
There was a problem hiding this comment.
Maybe I'm missing something but, if we are defaulting the spec.Auth.applicationCredentialSecret, how will it work in password mode? I understood that empty ACSecret means it uses password authentication.
Also, given that we are using GetApplicationCredentialFromSecret in the controlloer, this will only work if the spec.Auth.ApplicationCredentialSecret has ac-watcher-secret, I'd say it will not work with any other name.
There was a problem hiding this comment.
You are right that the current implementation hardcodes the secret name,so the field value itself isn't used for the lookup. However, I think it's clearer than using a boolean because users can see which secret name is expected and it future proofs for potentially supporting custom secret names or multiple credentials.
Re the password auth logic: even though the webhook defaults this field, password vs AC auth mode is determined by if the secret exists in the cluster, not by the spec field value. If the secret doesn't exist, GetApplicationCredentialFromSecret returns nil and the service uses password auth. This allows services to start with password auth and then adopt AC when the secret becomes available.
There was a problem hiding this comment.
TBH, for the behaviour you mentions I would do it differently. The actual behavior you are describing is that watcher is going to automatically discover if there is an AC or not by checking if the secret exists and it will use it if so. There is no real way of modifying the secret name or even to using password mode in case the AC secret has been created by the openstack-operator.
For me, for this behavior we don't need to add any parameter to the spec at all, only to implement automatically discover and use of the secret. Actually, the existing parameter is really not meaningful. I'd say that it may be confusing or missleading for the users because we may have a deployment where the param auth.applicationCredentialSecret has a value but watcher is using password mode because the secret didn't exist and the user has no way to know it.
If the desired workflow is what I understood (automatic detection via hardcoded named secret), I would not add any parameter at all to the spec, but only a field to the status of the watcher object specifying if AC is used or even the name of the secret if that's something that we think the cloud admin should be aware of (I'm not sure, tbh).
IMO, there is no point in adding a parameter for something that can not be changed in case it's needed it future, we may always add the parameter when it's actually needed.
There was a problem hiding this comment.
I understand that it might be confusing to see the secret field defined while service pods are using password auth. However I don't think we can afford to change the implementation across service operators now. Also, previously we used the secret watchers, that are expensive, so it was suggested to go with the similar pattern as TLS --> openstack-k8s-operators/glance-operator#812 (comment).
But for the specific services user can also see if the app cred auth is enabled or not based on the spec.watcher.applicationCredential.enabled field present in the control plane CR, example (reduced):
apiVersion: core.openstack.org/v1beta1
kind: OpenStackControlPlane
metadata:
name: openstack
spec:
applicationCredential:
enabled: true # Global AC feature switch and global settings
expirationDays: 365
gracePeriodDays: 182
roles:
- service
- admin
unrestricted: false
.....
watcher:
enabled: true
applicationCredential:
enabled: true # Enable AC for Watcher service and override global settings
expirationDays: 180
....
There was a problem hiding this comment.
About checking the AC enablement in the OpenStackControlPlane, works for me, that should be the main config point fot users, so I think it makes sense.
WRT the usage of auth.applicationCredentialSecret in service operators, I still think that the implementation could be better according to the current implementation. Said so, I agree that the implementation has to be consistent accross operators, if it's not possible to change or reconsider at this point, we can live with it.
Just to be clear, with this implementation, service operators have no way to know if the user requested the AC in the ControlPlane, i.e. if for any reason the openstack-operator fails to create the AC Secret, watcher will simply configure watcher in password mode and the error will not be handled. The openstack-operator should properly handle that case (i.e. making sure the ac secret is created before updating/creating the watcher CR) in its reconcile loop.
IMO, it'd be be better to not default auth.applicationCredentialSecret with the webhook and use it as a mark of the AC has been enabled for Watcher in the OpenStackControlPlane. Then, If auth.applicationCredentialSecret != "" and the secret does not exist, watcher-operator should fail to reconciliate.
There was a problem hiding this comment.
so this has not been merged in other operatrs yet at least not in nova, watcher or placemetn so i dotn think we are past the poitn of no return
the aguremnt of the secreate watching being expsinsive dose not hold for watcher or nova becuase we already have a secreate that we are wathching that we woudl just be extending
having the watcher-api watcher-decision-enginge and wather appler contoler watch the service cofnig secreat and the applciation credetial instead of just the secrete is always goign to be more expensive.
"""
auth.applicationCredentialSecret != "" and the secret does not exist, watcher-operator should fail to reconciliate.
"""
yes in thei case watcher should fail to reconfile and set the input condtion to false with an error ro intoduce a new condition.
it is an error to refence a applcaition credial that does not exsit.
however we are not refenicg an application Credential Secret
we are referenicn an applicationCredential CR which shoudl have a status and ready condtion.
we shoudl not be passign in a direct refence to the secret in the watcher CRD.
There was a problem hiding this comment.
by the way i previsouly objected to having an enabled flag for the appclation credital
the usage of appclaition credtials shoudl be determiend solely by if you pass one to the watcher CR or not. that is the pattern that we use for this in general.
There was a problem hiding this comment.
Passing CR or secret name (I agree CR is better) as a way to express AC is enabled would work for me but IMO then the name should be actually relevant and not hardcoded, IMO
There was a problem hiding this comment.
right hardcodeign is a bug in keystone IMO
Deydra71
force-pushed
the
appcred-support
branch
from
2fa23d1 to
f5b9cd3
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. |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f02ca7f11bca49b297f46af62470bfe8 ❌ openstack-meta-content-provider-master FAILURE in 37m 37s |
amoralej
left a comment
amoralej
left a comment
There was a problem hiding this comment.
I've successfully tested this locally. It worked fine with both password and AC. Also I checked it manages the lack of AC secret when configured as expected. It sets InputReady to False and keeps testing.
I've also tested the password rotation workflow by running
oc patch -n openstack keystoneapplicationcredential ac-watcher --type=merge --subresource=status watcher-test -p '{"status":{"expiresAt":"2001-05-19T00:00:00Z"}}'
A new application credential is created in Keystone, the secret is updated and the watcher-operator automatically reconciles the new configuration.
Just one note, the previous AC still exists in keystone with the original expiration time. I understand this is expected and the user would have to manually delete it from keystone if he's willing to invalidate it. correct?
I just left some minor comments, most of it related to kuttl and one about the condition message on retry condition. Other than that, lgtm.
| condition.InputReadyCondition, | ||
| condition.RequestedReason, | ||
| condition.SeverityInfo, | ||
| condition.InputReadyWaitingMessage)) |
There was a problem hiding this comment.
I'd use something WatcherApplicationCredentialSecretErrorMessage also here to give more information about the error.
| @@ -0,0 +1,56 @@ | |||
| apiVersion: kuttl.dev/v1beta1 | |||
There was a problem hiding this comment.
You can first validate that watcher deployment is Ready and the spec properly modified, similar to https://github.com/openstack-k8s-operators/watcher-operator/blob/main/test/kuttl/test-suites/default/watcher-notification/01-assert.yaml#L1-L83 (conditions need to be slightly adjusted). No need to wait for the rollouts, once watcher is in ready, deployments should be finished.
You could validate the KeystoneApplicationCredential and the secret just by adding the object fields you want to validate in yaml format as i.e. https://github.com/openstack-k8s-operators/watcher-operator/blob/main/test/kuttl/test-suites/default/watcher/01-assert.yaml#L133-L159
| old_api_uid=$(oc get pod -n "${NS}" watcher-kuttl-api-0 -o jsonpath='{.metadata.uid}') | ||
| old_applier_uid=$(oc get pod -n "${NS}" watcher-kuttl-applier-0 -o jsonpath='{.metadata.uid}') | ||
| old_de_uid=$(oc get pod -n "${NS}" watcher-kuttl-decision-engine-0 -o jsonpath='{.metadata.uid}') | ||
| oc create configmap appcred-watcher-pre \ | ||
| --from-literal=api_uid="${old_api_uid}" \ | ||
| --from-literal=applier_uid="${old_applier_uid}" \ | ||
| --from-literal=decision_engine_uid="${old_de_uid}" \ | ||
| --dry-run=client -o yaml | oc apply -n "${NS}" -f - |
There was a problem hiding this comment.
IMO, this is not needed as explained before. Said this, no problem if you want to keep it.
| echo "Waiting for ac-watcher.status.acID to change..." | ||
| for _ in $(seq 1 60); do | ||
| ac_id=$(oc get -n "${NS}" appcred/ac-watcher -o jsonpath='{.status.acID}') | ||
| if [ -n "${ac_id}" ] && [ "${ac_id}" != "${prev_acid}" ]; then | ||
| break | ||
| fi | ||
| sleep 5 | ||
| done |
There was a problem hiding this comment.
kuttl itself implements it's own retries on failures and timeout mechanism on asserts. I'd say you don'to need to do this wait here.
| echo "Waiting for watcher rollouts after rotation..." | ||
| oc rollout status -n "${NS}" statefulset/watcher-kuttl-api --timeout=300s | ||
| oc rollout status -n "${NS}" statefulset/watcher-kuttl-applier --timeout=300s | ||
| oc rollout status -n "${NS}" statefulset/watcher-kuttl-decision-engine --timeout=300s |
There was a problem hiding this comment.
You may just add a previous validation step for Watcher to be Ready.
| oc exec -n "${NS}" pod/watcher-kuttl-api-0 -c watcher-api -- \ | ||
| bash -c "grep -q \"^application_credential_id = ${ac_id}$\" /etc/watcher/watcher.conf.d/00-default.conf" | ||
| oc exec -n "${NS}" pod/watcher-kuttl-applier-0 -c watcher-applier -- \ | ||
| bash -c "grep -q \"^application_credential_id = ${ac_id}$\" /etc/watcher/watcher.conf.d/00-default.conf" | ||
| oc exec -n "${NS}" pod/watcher-kuttl-decision-engine-0 -c watcher-decision-engine -- \ | ||
| bash -c "grep -q \"^application_credential_id = ${ac_id}$\" /etc/watcher/watcher.conf.d/00-default.conf" | ||
| echo "✓ watcher config contains expected application_credential_id" |
| old_api_uid=$(oc get -n "${NS}" configmap/appcred-watcher-pre -o jsonpath='{.data.api_uid}') | ||
| old_applier_uid=$(oc get -n "${NS}" configmap/appcred-watcher-pre -o jsonpath='{.data.applier_uid}') | ||
| old_de_uid=$(oc get -n "${NS}" configmap/appcred-watcher-pre -o jsonpath='{.data.decision_engine_uid}') | ||
|
|
||
| new_api_uid=$(oc get pod -n "${NS}" watcher-kuttl-api-0 -o jsonpath='{.metadata.uid}') | ||
| new_applier_uid=$(oc get pod -n "${NS}" watcher-kuttl-applier-0 -o jsonpath='{.metadata.uid}') | ||
| new_de_uid=$(oc get pod -n "${NS}" watcher-kuttl-decision-engine-0 -o jsonpath='{.metadata.uid}') | ||
|
|
||
| if [ "${old_api_uid}" = "${new_api_uid}" ] || [ "${old_applier_uid}" = "${new_applier_uid}" ] || [ "${old_de_uid}" = "${new_de_uid}" ]; then | ||
| echo "ERROR: expected watcher pods to restart after appcred secret became available" | ||
| echo " api: ${old_api_uid} -> ${new_api_uid}" | ||
| echo " applier: ${old_applier_uid} -> ${new_applier_uid}" | ||
| echo " decision-engine: ${old_de_uid} -> ${new_de_uid}" | ||
| exit 1 | ||
| fi | ||
| echo "✓ watcher pods restarted after appcred secret became available" |
There was a problem hiding this comment.
IMO, you could simplify these tests. You could just check that the content of the config file is correct as you do in following lines. If the pod is not restared after adding the ac in 02-deploy-appcred.yaml, the content of the config file whould still be the old one.
|
check-rdo |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/42d3795150b042fdae67f925bd22f0e2 ❌ openstack-meta-content-provider-master FAILURE in 34m 35s |
|
check-rdo |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/0f49d0f817b74152b152108ba272de96 ❌ openstack-meta-content-provider-master FAILURE in 32m 13s |
Deydra71
force-pushed
the
appcred-support
branch
from
86c4a3b to
1ad5bbe
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/6a54cae9f1e246648efa77a639c4f729 ✔️ openstack-meta-content-provider-master SUCCESS in 2h 42m 39s |
Deydra71
force-pushed
the
appcred-support
branch
from
1ad5bbe to
cf0c613
Compare
|
@amoralej Hi! Can you please re review this patch. I corrected the log to use Re kuttl tests: I tried to refactor to declarative YAML checks, but it caused timing issues with the AC rotation scenario - the script-based approach seem tio handle the sequential pod restarts and config updates better. I've reverted to the original script version, if it's okay with you? |
Thanks
Yes, it's ok. I may give it a try in follow-up patch With this, I think the patch is fine to merge. Once we have all AC related patches merged, I will try to move one of the watcher deployments jobs to use AC unless you have some AC job in other operator where watcher could be enabled. WRT the issue of the applicationCredentialSecret being hardcoded in keystone-operator instead of using the one in the parameter, It'd be great to get it fixed at some point but I understand that will not be covered in this initial implementation. |
| condition.RequestedReason, | ||
| condition.SeverityInfo, | ||
| watcherv1beta1.WatcherApplicationCredentialSecretErrorMessage)) | ||
| return ctrl.Result{RequeueAfter: time.Second * 10}, nil |
There was a problem hiding this comment.
Ideally, we shouldn't need this RequeueAfter as we have a watcher in the Secret, this has performance impact. However, given that we are actually using a hardcoded secret name that even may be different that the one in the parameter, it's good to check. We should remove and just return ctrl.Result{} if we implement using the actual secret at some point.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amoralej The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Deydra71
force-pushed
the
appcred-support
branch
from
cf0c613 to
c2719a9
Compare
|
Hi @amoralej ! I updated the AC implementation to correctly read the secret name from |
|
/retest 502 Bad Gateway |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ed93897045e5458bb48ba53009df9693 ✔️ openstack-meta-content-provider-master SUCCESS in 2h 40m 07s |
|
recheck retry limit before the actual watcher deployment |
Co-authored-by: Veronika Fisarova <vfisarov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Deydra71
force-pushed
the
appcred-support
branch
from
c772d17 to
e4db9c1
Compare
amoralej
left a comment
amoralej
left a comment
There was a problem hiding this comment.
This is go to good for me
openshift-merge-bot
Bot
merged commit 3cd4f14
into
openstack-k8s-operators:main
5 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