AppCred support #1430
AppCred support #1430
Conversation
|
Skipping CI for Draft Pull Request. |
|
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. |
|
Unable to freeze job graph: Job adoption-standalone-to-crc-ceph-provider depends on openstack-k8s-operators-content-provider which was not run. |
|
recheck |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/fb95ce639e164bb190aa3b41fcda82da ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 59m 19s |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/64ab1660f5ff460cb0bdb2682d3b4149 ❌ openstack-k8s-operators-content-provider FAILURE in 15m 15s |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/5ebbd01a8bc549b2956a87c3507363e2 ❌ openstack-k8s-operators-content-provider FAILURE in 13m 33s |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/67ea6f17ebce4477b46d077171537d98 ❌ openstack-k8s-operators-content-provider FAILURE in 13m 49s |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/65697c077ffe4630a698b4a94657a08a ❌ openstack-k8s-operators-content-provider FAILURE in 16m 43s |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/8b21d132d2c944f4bb8faccfff711e47 ❌ openstack-k8s-operators-content-provider FAILURE in 14m 09s |
Deydra71
force-pushed
the
appcred-support
branch
2 times, most recently
from
62fd3e5 to
27db2bd
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
b98631b to
5a36538
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/fe0e210d9f7d44429cfde38f83a800be ❌ openstack-k8s-operators-content-provider FAILURE in 18m 57s |
vyzigold
left a comment
vyzigold
left a comment
There was a problem hiding this comment.
In telemetry, we have an enabled switch for the whole telemetry similarly to other services to disable / enable the whole telemetry. But we also allow to enable / disable each individual part of telemetry, so we have additional enabled switches for aodh, ceilometer and cloudkitty, which I think should be also used to determine whether to create the application credentials. See my suggestions.
| {"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.ApplicationCredentialCeilometer}, |
There was a problem hiding this comment.
| {"ceilometer", instance.Spec.Telemetry.Enabled, instance.Spec.Telemetry.ApplicationCredentialCeilometer}, | |
| {"ceilometer", instance.Spec.Telemetry.Enabled && instance.Spec.Telemetry.Template.Ceilometer.Enabled, instance.Spec.Telemetry.ApplicationCredentialCeilometer}, |
There was a problem hiding this comment.
Yes, good catch. We don't want to rely on solely "disabled implies empty template fields"
There was a problem hiding this comment.
Note: I can't accept suggestions directly, because we need dereference the pointer:
{"ceilometer",
instance.Spec.Telemetry.Enabled &&
instance.Spec.Telemetry.Template.Ceilometer.Enabled != nil &&
*instance.Spec.Telemetry.Template.Ceilometer.Enabled,
instance.Spec.Telemetry.ApplicationCredentialCeilometer,
},
| {"manila", instance.Spec.Manila.Enabled, instance.Spec.Manila.ApplicationCredential}, | ||
| {"designate", instance.Spec.Designate.Enabled, instance.Spec.Designate.ApplicationCredential}, | ||
| {"watcher", instance.Spec.Watcher.Enabled, instance.Spec.Watcher.ApplicationCredential}, | ||
| {"aodh", instance.Spec.Telemetry.Enabled, instance.Spec.Telemetry.ApplicationCredentialAodh}, |
There was a problem hiding this comment.
| {"aodh", instance.Spec.Telemetry.Enabled, instance.Spec.Telemetry.ApplicationCredentialAodh}, | |
| {"aodh", instance.Spec.Telemetry.Enabled && instance.Spec.Telemetry.Template.Autoscaling.Enabled, instance.Spec.Telemetry.ApplicationCredentialAodh}, |
| {"designate", instance.Spec.Designate.Enabled, instance.Spec.Designate.ApplicationCredential}, | ||
| {"watcher", instance.Spec.Watcher.Enabled, instance.Spec.Watcher.ApplicationCredential}, | ||
| {"aodh", instance.Spec.Telemetry.Enabled, instance.Spec.Telemetry.ApplicationCredentialAodh}, | ||
| {"cloudkitty", instance.Spec.Telemetry.Enabled, instance.Spec.Telemetry.ApplicationCredentialCloudKitty}, |
There was a problem hiding this comment.
| {"cloudkitty", instance.Spec.Telemetry.Enabled, instance.Spec.Telemetry.ApplicationCredentialCloudKitty}, | |
| {"cloudkitty", instance.Spec.Telemetry.Enabled && instance.Spec.Telemetry.Template.CloudKitty.Enabled, instance.Spec.Telemetry.ApplicationCredentialCloudKitty}, |
Deydra71
force-pushed
the
appcred-support
branch
from
5a36538 to
062b528
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/097e3f645c1a4f4094a7fcfe49b94d04 ❌ openstack-k8s-operators-content-provider FAILURE in 11m 20s |
Deydra71
force-pushed
the
appcred-support
branch
from
062b528 to
cfde225
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/c0c013624c3444e9a20465996192328f ❌ openstack-k8s-operators-content-provider FAILURE in 12m 20s |
Deydra71
force-pushed
the
appcred-support
branch
from
cfde225 to
ea36bc5
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/a7e544c73217452b93106c7807a5aea4 ❌ openstack-k8s-operators-content-provider FAILURE in 11m 40s |
| // Enabled indicates whether an ApplicationCredential should be created | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default=false | ||
| Enabled bool `json:"enabled"` |
There was a problem hiding this comment.
wondering if we need this global switch, if you always have to enable it also on the per service level. if there is the req to always do it in each service do we need a global switch? wouldn't a global switch only make sense if we default the per service to true. like:
- switch global to true ac is enable for all services
- if you want to disable it for some services switch them to off
There was a problem hiding this comment.
I know having two switches might feel confusing, but I was thinking about them as two different layers of control. - per-service enablement should stay explicit so the operator isn’t surprised by authentication changing in a service they didn’t mean to migrate yet. The global switch is mainly there for ops convenience and safety: if we ever need to disable AppCred everywhere, we can flip one flag instead of going service-by-service again. If we made per-service default to true, it could be missed and then users would wonder why auth changed unexpectedly. With opt-in per service and global gate, rollout stays controlled and predictable, and we can still turn it off fast at once.
But this is just my idea, I'm open to suggestion based on better practices.
There was a problem hiding this comment.
I was inclined towards having a single switch too, but double knob makes sense now.
do you think we can enable a logs for same, so suppose operator only enabled in respective service but they wont see AC's until the global is enabled too.
to debug they will start with operator logs of respective service - will it make sense to add an warning or error log saying global one might not be enabled.
| // ServiceAppCredSection allows service-specific overrides of the global AC configuration | ||
| type ServiceAppCredSection struct { | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default=false |
There was a problem hiding this comment.
see my comment on the global parameter, should this one default to true?
| // ACRule describes a single access rule for an ApplicationCredential | ||
| // +k8s:openapi-gen=true | ||
| type ACRule struct { | ||
| // Service is the name of the service to target (e.g. "identity"). |
There was a problem hiding this comment.
from where to get the service name? can we link a reference or will it be in product doc or how to use ac rules?
There was a problem hiding this comment.
These access rules are a keystone upstream feature for app creds. It is documented directly in the OpenStackClient CLI docs and it will be documented in the product doc how to get the necessary fields (service, path, method) from the cluster.
@vakwetu Can you please provide better answer than me, I remember that I added the AccessRules based on your suggesitons.
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| Service string `json:"service"` | ||
| // Path is the HTTP path (e.g. "/v3/auth/tokens"). |
There was a problem hiding this comment.
from where you get this? you have to know or is there some doc? or do we document this as part of the AC feature in product doc?
| // Whether the AC should be unrestricted | ||
| Unrestricted *bool `json:"unrestricted,omitempty"` | ||
|
|
||
| // AccessRules lets the service override either the global rules |
| } | ||
| r.Spec.Cinder.Template.Default() | ||
| // Default Auth fields for Application Credentials | ||
| if r.Spec.Cinder.Template.CinderAPI.Auth.ApplicationCredentialSecret == "" { |
There was a problem hiding this comment.
we are setting it to a string of the secret name in the webhook, even the secret might not be there, not sure if I understand the intend of this. shouldn't we only set the ApplicationCredentialSecret in the controller if AC is enabled and the secret got created?
There was a problem hiding this comment.
YEs, we should set this only when app cred are enabled. The original intent was to declare where the secret is, not if it exists, because service operators are designed to handle this, eg in glance-operator/pull/812
But I will change it to appear only when app creds are enabled to not raise confusion.
There was a problem hiding this comment.
I think we should remove any defaulting via webhooks. if e.g. for glance AC gets enabled in its ReconcileGlance https://github.com/openstack-k8s-operators/openstack-operator/blob/main/internal/openstack/glance.go#L68 the operator should request the AC and check/wait until the AC is ready. if ready, it sets the parameter in the template. if it gets disabled, it removes the parameter in the template, or nils it. not sure what it is.
api/go.mod
Outdated
| @@ -1,34 +1,34 @@ | |||
| module github.com/openstack-k8s-operators/openstack-operator/api | |||
| module github.com/openstack-k8s-operators/openstack-operator/apis | |||
|
|
||
| // ReconcileApplicationCredentials ensures an AC CR per enabled service, | ||
| // propagating its secret name, passwordSelector, and serviceUser fields. | ||
| func ReconcileApplicationCredentials( |
There was a problem hiding this comment.
can you explain the process when AC get enabled. I was expecting that the creation of the AC for each service happens in each of the service reconciler logics to have it there as a pre-step if enabled from beginning or when enabled later. seems right now we have a dedicated ReconcileApplicationCredentials logic which then patches each service template with a lot of hard coded stuff. I don't think this is how we want to do it, or is there a need to do it that way?
There was a problem hiding this comment.
So, based on the agreed ZDPR design - the AC CR creation should happen in the opesntack-operator and service operators act only as consumers of the AC Secret. However what I didn't realize is that right now we don't take the actual service info from the actual service CR, btu from OpenStackControlPlane.Spec templates.
Would it be sufficient to get the service info from the actual service CR, would eliminate the hard-coded switch statement that reaches into template structures, eg:
// In openstack-operator
ffunc ReconcileApplicationCredentials(ctx, instance, helper) error {
// ... global enabled check ...
// Glance
if instance.Spec.Glance.Enabled && isACEnabledForService(instance, "glance") {
glanceCR := &glancev1.Glance{}
err := helper.GetClient().Get(ctx, types.NamespacedName{
Name: "glance",
Namespace: instance.Namespace,
}, glanceCR)
if err != nil {
if errors.IsNotFound(err) {
log.Info("Glance CR not found yet, skipping AC creation")
// AC will be created on next reconcile
} else {
return fmt.Errorf("failed to get Glance CR: %w", err)
}
} else {
acConfig := mergeAppCred(
instance.Spec.ApplicationCredential,
instance.Spec.Glance.ApplicationCredential,
)
err := reconcileApplicationCredential(
ctx, helper, instance,
"ac-glance", // One AC name
glanceCR.Spec.ServiceUser, // One service user
glanceCR.Spec.Secret,
glanceCR.Spec.PasswordSelectors.Service,
acConfig,
)
if err != nil {
return fmt.Errorf("failed to reconcile AC for glance: %w", err)
}
}
}
// other services
// ironic will be different, because it has two keystone service users
return nil
}
Of course we need to add some check if the service is not created yet, so it doesn't fail on the first reconcile. And the structure of each service might be little different.
There was a problem hiding this comment.
check #1430 (comment) . thats I think how we should do it. we can have some helpers in internal/openstack/applicationcredential.go, so that it is simple func call from each Reconcile func. which this we do not have to maintain any list of possible service, or have to add something to a template outside of where the template gets created and updated.
There was a problem hiding this comment.
this func could now be deleted?
Deydra71
force-pushed
the
appcred-support
branch
from
ea36bc5 to
22dd45d
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
22dd45d to
fc817bc
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f8ab8a85899342ce9ccc33f474348db3 ❌ openstack-k8s-operators-content-provider FAILURE in 14m 12s |
| // Update the glanceAPI with Auth defaults | ||
| r.Spec.Glance.Template.GlanceAPIs[name] = glanceAPI |
| // Initialize ApplicationCredential (watcher specific) field to avoid null value | ||
| // This ensures consistent behavior with other services. | ||
| if r.Spec.Watcher.ApplicationCredential == nil { | ||
| r.Spec.Watcher.ApplicationCredential = &ServiceAppCredSection{Enabled: false} | ||
| } | ||
|
|
There was a problem hiding this comment.
same, is this needed? seems to be the default from the kubebuilder annotation?
| name: openstack-operator-controller-operator | ||
| namespace: system | ||
| - patch: '[{"op": "replace", "path": "/spec/template/spec/containers/0/env/0", "value": | ||
| {"name": "OPENSTACK_RELEASE_VERSION", "value": "0.5.0-1767944881"}}]' | ||
| target: | ||
| kind: Deployment | ||
| name: openstack-operator-controller-operator |
There was a problem hiding this comment.
I think we don't want to include the file config/operator/deployment/kustomization.yaml in the PR.
There was a problem hiding this comment.
Yes, this is a generated remnant when I built the image, I will remove the changes here.
| switch serviceKey { | ||
| case "glance": | ||
| acSection = instance.Spec.Glance.ApplicationCredential | ||
| case "cinder": | ||
| acSection = instance.Spec.Cinder.ApplicationCredential | ||
| case "swift": | ||
| acSection = instance.Spec.Swift.ApplicationCredential | ||
| case "ironic": | ||
| acSection = instance.Spec.Ironic.ApplicationCredential | ||
| case "ironic-inspector": | ||
| // ironic-inspector shares the same AC configuration as ironic | ||
| acSection = instance.Spec.Ironic.ApplicationCredential | ||
| case "heat": | ||
| acSection = instance.Spec.Heat.ApplicationCredential | ||
| case "manila": | ||
| acSection = instance.Spec.Manila.ApplicationCredential | ||
| case "neutron": | ||
| acSection = instance.Spec.Neutron.ApplicationCredential | ||
| case "nova": | ||
| acSection = instance.Spec.Nova.ApplicationCredential | ||
| case "placement": | ||
| acSection = instance.Spec.Placement.ApplicationCredential | ||
| case "octavia": | ||
| acSection = instance.Spec.Octavia.ApplicationCredential | ||
| case "barbican": | ||
| acSection = instance.Spec.Barbican.ApplicationCredential | ||
| case "designate": | ||
| acSection = instance.Spec.Designate.ApplicationCredential | ||
| case "watcher": | ||
| acSection = instance.Spec.Watcher.ApplicationCredential | ||
| case "ceilometer": | ||
| acSection = instance.Spec.Telemetry.ApplicationCredentialCeilometer | ||
| case "aodh": | ||
| acSection = instance.Spec.Telemetry.ApplicationCredentialAodh | ||
| case "cloudkitty": | ||
| acSection = instance.Spec.Telemetry.ApplicationCredentialCloudKitty | ||
| } |
There was a problem hiding this comment.
I think we should not have service specific things in here. it should be just a generic func. if the services passes in its ApplicationCredential I think we can get rid of all these?
| acExists := err == nil | ||
|
|
||
| // Check if AC is enabled for this service | ||
| if !shouldEnableACForService(serviceKey, instance) { |
There was a problem hiding this comment.
as mentioned bellow, if we pass in the ApplicationCredential I don't think we need that complex shouldEnableACForService with different service names.
could be just return ac != nil && ac.Enabled, right?
|
|
||
| // servicesWithMultipleUsers defines services that have multiple Keystone users | ||
| // The function returns nil if the service is not enabled or template is not initialized | ||
| var servicesWithMultipleUsers = map[string]func(*corev1beta1.OpenStackControlPlane) []ServiceUserConfig{ |
internal/openstack/barbican.go
Outdated
|
|
||
| // Application Credential Management (Day-2 operation) | ||
| barbicanReady := barbican.Status.ObservedGeneration == barbican.Generation && barbican.IsReady() | ||
| acEnabled := shouldEnableACForService("barbican", instance) |
There was a problem hiding this comment.
if we already check it here, which is basically Template.Auth.ApplicationCredentialSecret != nil && Template.Auth.ApplicationCredentialSecret.Enabled lets pass it to EnsureApplicationCredentialForService. add a simple func where each service can pass in its Auth and get back the bool.
internal/openstack/barbican.go
Outdated
| if !acEnabled { | ||
| instance.Spec.Barbican.Template.Auth.ApplicationCredentialSecret = "" | ||
| } else if acReady { | ||
| instance.Spec.Barbican.Template.Auth.ApplicationCredentialSecret = keystonev1.GetACSecretName("barbican") |
There was a problem hiding this comment.
I think EnsureApplicationCredentialForService should return the AC and we then can just use the secret from the AC.
internal/openstack/barbican.go
Outdated
| ctx, | ||
| helper, | ||
| instance, | ||
| "barbican", |
There was a problem hiding this comment.
should or could this be barbican.Name, could remove the need for another static string.
Deydra71
force-pushed
the
appcred-support
branch
from
fc817bc to
bf826ed
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/5c42e7939c0a40fa92ed0b1ba8dfae74 ❌ openstack-k8s-operators-content-provider FAILURE in 12m 38s |
| if barbicanSecret == "" { | ||
| barbicanSecret = instance.Spec.Secret | ||
| } | ||
|
|
There was a problem hiding this comment.
I think to prevent the bellow mentioned flapping we'd have to do something like we do for certs to fetch the current auth from the running barbican
if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Barbican.ApplicationCredential) {
instance.Spec.Barbican.Template.Auth = barbican.Spec.Barbican.Template.Auth
}
internal/openstack/barbican.go
Outdated
| // Set or clear ApplicationCredentialSecret | ||
| // - If AC disabled: use password | ||
| // - If AC enabled AND ready: use AC | ||
| // - If AC enabled BUT not ready: leave unchanged to avoid flapping | ||
| if acSecretName == "" && !isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Barbican.ApplicationCredential) { | ||
| instance.Spec.Barbican.Template.Auth.ApplicationCredentialSecret = "" | ||
| } else if acSecretName != "" { | ||
| instance.Spec.Barbican.Template.Auth.ApplicationCredentialSecret = acSecretName | ||
| } |
There was a problem hiding this comment.
with the above comment, it might be ok to just assign what EnsureApplicationCredentialForService returns as the ApplicationCredentialSecret
instance.Spec.Barbican.Template.Auth.ApplicationCredentialSecret = acSecretName
- If AC disabled: use password, it returns ""
- If AC enabled AND ready. it returns the name
internal/openstack/barbican.go
Outdated
| ) | ||
| if err != nil { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
do we have to return for the Application Credential not ready yet case when we return with the RequeueAfter?
if (acResult != ctrl.Result{}) {
return acResult, nil
}
internal/openstack/barbican.go
Outdated
| barbicanSecret = instance.Spec.Secret | ||
| } | ||
|
|
||
| acSecretName, acResult, err := EnsureApplicationCredentialForService( |
There was a problem hiding this comment.
the default for AC is that they are disabled. wondering if we don't have to call it if ac is disabled and there is no current barbican.Spec.Barbican.Template.Auth.ApplicationCredentialSecret configured?
internal/openstack/barbican.go
Outdated
| if barbicanReady && (acResult != ctrl.Result{}) { | ||
| return acResult, nil | ||
| } |
There was a problem hiding this comment.
as commented above, wondering why we do it at the bottom and not right after the ensure func?
There was a problem hiding this comment.
I thought that if we returned early right after the rnsure, the service would never get created/udpated when AC is being providioned because we would skip CreateOrPatch?
|
this looks a lot cleaner!! just some questions for clarification, and maybe improving things |
Deydra71
force-pushed
the
appcred-support
branch
2 times, most recently
from
1378a63 to
b14002d
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/137c17c222454d38876bdbcd8e0a819a ❌ openstack-k8s-operators-content-provider FAILURE in 12m 25s |
Signed-off-by: Veronika Fisarova <vfisarov@redhat.com>
Deydra71
force-pushed
the
appcred-support
branch
from
b14002d to
abd35b4
Compare
|
PR needs rebase. 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. |
|
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: The following tests 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. |
6 participants
OSPRH-14738
This PR add ApplicationCredential support enabling both global defaults and service-specific overrides in OpenStackControlPlane.
CRD updates:
spec.applicationCredentialsection withenabled,expirationDaysandgracePeriodDaysenabled:falsein every supported service, whileexpirationDaysandgracePeriodDaysare hidden unless specified directly (in that case global values are used).Controller logic:
enable: trueenabledis turned offExample:
In the example barbican is using days overrides, while cinder is using default values.
Depends-On: openstack-k8s-operators/keystone-operator#567