-
Notifications
You must be signed in to change notification settings - Fork 102
AppCred support #1430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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 |
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. |
|
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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed?
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, definitely
| acExists := err == nil | ||
|
|
||
| // Check if AC is enabled for this service | ||
| if !shouldEnableACForService(serviceKey, instance) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still needed?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should or could this be barbican.Name, could remove the need for another static string.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 |
b14002d to
abd35b4
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. |
|
[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. 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/045a5dddacfa477d9cb68393191d1719 ❌ openstack-k8s-operators-content-provider FAILURE in 12m 52s |
| replace github.com/cert-manager/cmctl/v2 => github.com/cert-manager/cmctl/v2 v2.1.2-0.20241127223932-88edb96860cf //allow-merging | ||
|
|
||
| // appcred related changes | ||
| replace github.com/openstack-k8s-operators/keystone-operator/api => github.com/Deydra71/keystone-operator/api v0.0.0-20260119142142-e3bd4fb9750f //allow-merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these lines after new versions are released and run tidy
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/b1b7e13ddaa04b02a720ca60197ce146 ❌ openstack-k8s-operators-content-provider FAILURE in 12m 34s |
Signed-off-by: Veronika Fisarova <vfisarov@redhat.com>
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/743bfa4caa564529bf798ba240bf1954 ❌ openstack-k8s-operators-content-provider FAILURE in 27m 28s |
|
@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. |
|
Note: we need to update cinder, manila and heat after they are bumped, because we changed the placement of |
|
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. |
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