Application Credential support #812
Conversation
|
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
c523b51 to
72a7594
Compare
| } | ||
|
|
||
| // Check if this is a glance AC secret by name pattern (ac-glance-secret) | ||
| expectedSecretName := keystonev1.GetACSecretName("glance") |
There was a problem hiding this comment.
I'm not entirely sure we should hardcode glance here, especially because if this depends on the CR name, we might fail due to the fact we can append a suffix from the openstack-operator. If the openstack-operator ends up creating AppCred for glance using the CR name, you might end up having ac-glance-12345678-secret, and this might create side effects to this code (it wouldn't generate a reconcile loop).
Instead of matching a secret using the name, I think it might be solid to select if via LabelSelector, so you can remove the dependency on the name and any potential suffix on the CR.
I did something similar in [1], where you only reconcile if a LabelSelector matches.
However, I'm good with the current approach if we manage to resolve the problem with the hardcoded name, I just think we can make it more solid and less error prone.
Let's think a bit more about it and get some feedback from @stuggi too in case a better pattern already exists!
[1] #795
There was a problem hiding this comment.
Thanks @fmount for reviews! I still need to go through them.
But regarding this part - CR Names are not suffixed by openstack-operator [1]. The only place where a random suffix is added is in the Keystone Application Credential name (not the CR) [2].
Regarding matching by the label - currently we don't have any service-specific labels. All AppCred CRs share the same label structure, so using LabelSelector would not distinguish between services. But I'm open to create service specific labes, to bypass hardcoding, which can be always tricky... Let's see about Martin's additional comment.
[1] https://github.com/openstack-k8s-operators/openstack-operator/pull/1430/files#diff-c7b5c3051971f29e36eeddfd6ae38150becaabdf19a75569fb7c9e964c42e78eR113
[2] https://github.com/openstack-k8s-operators/keystone-operator/pull/567/files#diff-83ece155782727a148956f6e4b2b7203661c2ff5b9f128979326158f988bb777R233
There was a problem hiding this comment.
ah ok interesting, I didn't see the openstack-operator patch and now I see that the service list is pretty much static:
svcs := []svcAC{
{"glance", instance.Spec.Glance.Enabled, instance.Spec.Glance.ApplicationCredential},
{"nova", instance.Spec.Nova.Enabled, instance.Spec.Nova.ApplicationCredential},
{"swift", instance.Spec.Swift.Enabled, instance.Spec.Swift.ApplicationCredential},
{"ceilometer", instance.Spec.Telemetry.Enabled, instance.Spec.Telemetry.ApplicationCredential},
{"barbican", instance.Spec.Barbican.Enabled, instance.Spec.Barbican.ApplicationCredential},
{"cinder", instance.Spec.Cinder.Enabled, instance.Spec.Cinder.ApplicationCredential},
{"placement", instance.Spec.Placement.Enabled, instance.Spec.Placement.ApplicationCredential},
{"neutron", instance.Spec.Neutron.Enabled, instance.Spec.Neutron.ApplicationCredential},
}LabelSelector would involve modifying [1] to pass at least something like service: <service, but if we do not have any aliasing problem then we can go with this version, where you can simply replace "glance" with glance.ServiceName.
Still let's wait for @stuggi's feedback as well, but this clarifies part of my concerns about using the name instead of something different.
controllers/glanceapi_controller.go
Outdated
| // verifyApplicationCredentials checks if ApplicationCredential secret exists and adds it to configVars | ||
| // The AC secret is created by the keystone-operator's AC controller when the AC is ready. | ||
| // If the secret exists and is valid, we use AC auth. Otherwise, we fall back to password auth. | ||
| func (r *GlanceAPIReconciler) verifyApplicationCredentials( |
There was a problem hiding this comment.
This functions seems very generic to me: if we do not pass instance *glancev1.GlanceAPI, but we simply pass a namespace or a serviceName there would be nothing Glance specific here, and the service could consume it from keystone-operator.
In addition, this might result in a lot of code duplication across the board, so I would invest some time to make it generic and remove the instance dependency.
There was a problem hiding this comment.
100% agree, good point. I will put this function to api/v1beta1/keystoneapplicationcredential.go in keystone-operator and make be usable in all service operators, similarly as we do now with GetApplicationCredentialFromSecret
controllers/glanceapi_controller.go
Outdated
| // If the secret exists and is valid, we use AC auth. Otherwise, we fall back to password auth. | ||
| func (r *GlanceAPIReconciler) verifyApplicationCredentials( | ||
| ctx context.Context, | ||
| _ *helper.Helper, |
There was a problem hiding this comment.
maybe we should not pass _ *helper.Helper if not required
controllers/glanceapi_controller.go
Outdated
| log := r.GetLogger(ctx) | ||
|
|
||
| // Check if AC secret exists (created by keystone AC controller) | ||
| acSecretName := keystonev1.GetACSecretName(glance.ServiceName) |
There was a problem hiding this comment.
ServiceName is a const that identifies the openstack image service [1] but this does not ensure this code will work if the CR name is different (e.g. glance-12345678 instead of glance).
If we are looking for the current glanceAPI name here, we usually expose the APIName() function [2], so you can technically pass it to the function and consume it here (this would help to make the function generic and not have dependencies on instance type).
[1] https://github.com/openstack-k8s-operators/glance-operator/blob/main/pkg/glance/const.go#L34
[2] https://github.com/openstack-k8s-operators/glance-operator/blob/main/api/v1beta1/glanceapi_types.go#L174
controllers/glanceapi_controller.go
Outdated
| secretKey, | ||
| []string{"AC_ID", "AC_SECRET"}, | ||
| r.Client, | ||
| 10*time.Second, |
There was a problem hiding this comment.
Note you can reference 10*time.Second with glance.NormaDuration [1]
| g.Expect(conf).To(ContainSubstring( | ||
| "application_credential_secret = test-ac-secret"), | ||
| ) | ||
| }, 30*time.Second, interval).Should(Succeed()) |
There was a problem hiding this comment.
is timeout [1] not sufficient here?
| www_authenticate_uri={{ .KeystonePublicURL }} | ||
| auth_url={{ .KeystoneInternalURL }} | ||
| auth_type=password | ||
| auth_type={{ if .UseApplicationCredentials }}v3applicationcredential{{ else }}password{{ end }} |
There was a problem hiding this comment.
to keep things cleaner here I would try:
{{ if .UseApplicationCredentials -}}
auth_type = v3applicationcredential
application_credential_id = {{ .ACID }}
application_credential_secret = {{ .ACSecret }}
{{ else -}}
auth_type = password
username={{ .ServiceUser }}
password = {{ .ServicePassword }}
{{- end }}
your version is correct, but this would improve readability, other than saving an if
| [oslo_limit] | ||
| auth_url={{ .KeystoneInternalURL }} | ||
| auth_type = password | ||
| auth_type = {{ if .UseApplicationCredentials }}v3applicationcredential{{ else }}password{{ end }} |
Deydra71
force-pushed
the
appcred-support
branch
from
72a7594 to
9a85e7f
Compare
|
[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. |
Signed-off-by: Veronika Fisarova <[email protected]>
Deydra71
force-pushed
the
appcred-support
branch
from
9a85e7f to
64b4e37
Compare
|
@Deydra71: The following test failed, say
Full PR test history. Your PR dashboard. Instructions 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. |
3 participants
Jira: OSPRH-16625
This PR adds end-to-end support for consuming Keystone ApplicationCredentials (AC) in the Glance operator, enabling Glance API pods to use AC-based authentication when available.
Reconcile:
On each reconcile, the Glance API controller checks for an AC Secret (ac-{service}-secret) using the GetApplicationCredentialFromSecret() helper from keystone-operator API:
If the secret is missing or incomplete, continues using password authentication
Once the AC Secret is ready with valid AC_ID and AC_SECRET fields, templates AC credentials into Glance configuration
Computes hash of Secret contents and stores in configVars to trigger rolling updates when credentials rotate
RBAC permissions:
Added read permissions for secrets resources to allow fetching AC secrets.
Depends-On: openstack-k8s-operators/keystone-operator#567