Skip to content

Conversation

@Deydra71
Copy link
Contributor

@Deydra71 Deydra71 commented Apr 16, 2025

Jira: OSPRH-14737

This PR introduces a new ApplicationCredential (AC) controller in the keystone-operator. It watches ApplicationCredential custom resources and performs these actions:

  1. Creates Keystone ApplicationCredentials for each CR (authenticating as that user due to Keystone’s default policy)
  2. Stores the AC’s ID and Secret in a k8s secret
  3. Implements rotation logic based on expirationDays and gracePeriodDays:
    • Reconciles at least once a day, rotating any AC that’s within or past its grace window
    • If an AC is already in the grace period at the next reconcile, it rotates immediately
    • The old ApplicationCredential in Keystone is not revoked on rotation (it naturally expires)

Additionally:

  • The controller waits for a KeystoneAPI resource to be Ready before proceeding with AC operations

Notes:

  • CRD & RBAC for the ApplicationCredential resource are not automatically installed yet. These must be applied manually until openstack-operator integration is complete

To apply rbac permissions run oc edit clusterrole keystone-operator-manager-role and add:

- apiGroups:
  - keystone.openstack.org
  resources:
  - applicationcredentials
  verbs:
  - create
  - delete
  - get
  - list
  - patch
  - update
  - watch

- apiGroups:
  - keystone.openstack.org
  resources:
  - applicationcredentials/finalizers
  verbs:
  - patch
  - update

- apiGroups:
  - keystone.openstack.org
  resources:
  - applicationcredentials/status
  verbs:
  - get
  - patch
  - update

Example AC CR for barbican service user:

apiVersion: keystone.openstack.org/v1beta1
kind: ApplicationCredential
metadata:
  name: ac-barbican
  namespace: openstack
spec:
  expirationDays: 365
  gracePeriodDays: 182
  passwordSelector: BarbicanPassword
  roles:
  - service
  secret: osp-secret
  userName: barbican

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 16, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

) (string, error) {

// The name of the Secret containing the service passwords
const ospSecretName = "osp-secret"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've discussed this - but basically we provide the ability for the operators to specify the secret that contains the admin user password. This is osp-secret by default - but it need not be. See https://github.com/openstack-k8s-operators/barbican-operator/blob/main/api/v1beta1/common_types.go#L44 for instance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we should not hardcode any value like this. It could be part of the deployment YAML spec.

return "", fmt.Errorf("failed to get Secret/%s: %w", ospSecretName, err)
}

key := capitalizeFirst(userName) + "Password"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a default, but its not what is specified. In barbican, for instance we have a PasswordSelectors field - https://github.com/openstack-k8s-operators/barbican-operator/blob/main/api/v1beta1/common_types.go#L49 which identifies the correct key. But, this needn't be the case.

Ultimately, I think you are going to need to have the AC specification include the name of the user, the name of the user secret and the relevant field. You can set these appropriately in openstack-operator.

Comment on lines 279 to 276
// Always assign these roles:
Roles: []applicationcredentials.Role{
{Name: "admin"},
{Name: "service"},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes this controller less generic and therefore potentially less useful. Perhaps this is another parameter - like the access rules that should be passed in as part of the AC spec.

The same can also be said for the Unrestricted field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there is support for all of Roles, Unrestricted and AccessRules in the gophercloud call - https://pkg.go.dev/github.com/gophercloud/gophercloud/openstack/identity/v3/applicationcredentials#CreateOpts

}

// Otherwise check again in 24 hours
return defaultRequeue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be return rotateAt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rotateAt is a timestamp, we need return a duration

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK that makes sense. I (mis)understood this function when I first read it.

So if I understand this correctly now, we should be returning a recheck duration of 24 hours, unless we are already in the grace period - in which case we would be returning a 0 to immediately recheck.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's exactly right

// default requeue is 24h as minimal grace period is 1 day
defaultRequeue := 24 * time.Hour
if ac.Status.ExpiresAt == nil || ac.Status.ExpiresAt.IsZero() {
return defaultRequeue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious - why would we want to requeue if the application credential does not expire?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this as an ultimate fallback to wake the controller at least once a day. If because of some error the status of the CR is not updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

return "fallback" // placeholder for generating failure
}
s := hex.EncodeToString(b)
return s[:n]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its unlikely, but should we check for collisions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could check if the AC with same suffix already exists, but unless we are creating millions of AC in short period the chance is basically zero. Or we can increase n.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this function be a lib-common utility in the long term?


logger := r.GetLogger(ctx)

// Only if user explicitly does "oc delete" do we revoke the AC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its actually quite useful, I think, to have the delete revoke the application credential. This gives us a nice way to do a revocation.

The problem is, of course, that there will be some time between when the app cred is revoked and the new one is issued.

I wonder if there is a way to trigger a rotation without doing a delete. Could we implement a reconcileUpdate to do this instead? Then, the procedure when trying to revoke the cert would be to -

  1. patch the AC so that we are within the grace period. This triggers the creation of a new AC.
  2. Wait for the new AC to be propagated.
  3. Revoke the old AC by deleting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just concerned about step 2. - Wait for the new AC to be propagated. Because AC controller would need a feedback that the service deployment successfully rolled out with new credential, which would add another logic to watch the deployment status in AC controller. I’d prefer to leave revocation as a manual step for now.

Copy link
Contributor

@stuggi stuggi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not finished my initial review, but probably won't have time today to do so. will continue tomorrow, and just wanted to add what I have so far.

Comment on lines 144 to 152
// Decide if we need to create (or rotate)
needsRotation := false
if instance.Status.ACID == "" {
needsRotation = true
logger.Info("AC does not exist, creating")
} else if r.shouldRotateNow(instance) {
needsRotation = true
logger.Info("AC is within grace period, rotating")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets split this out into a func

// Check if KeystoneAPI is ready
keystoneAPI, err := keystonev1.GetKeystoneAPI(ctx, helperObj, instance.Namespace, nil)
if err != nil {
logger.Info("KeystoneAPI not found, requeue", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should set KeystoneAPIReadyCondition

return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}
if !keystoneAPI.IsReady() {
logger.Info("KeystoneAPI not ready, requeue")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


// Ensure we have an initial ReadyCondition
condList := condition.CreateList(
condition.UnknownCondition(condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ReadyCondition gets auto added in Init.

should init KeystoneAPIReadyCondition and set it when checking for the keystoneapi bellow

			condition.UnknownCondition(keystonev1.KeystoneAPIReadyCondition, condition.InitReason, keystonev1.KeystoneAPIReadyInitMessage),

I think we could just use the same condition init as in https://github.com/openstack-k8s-operators/keystone-operator/blob/main/controllers/keystoneendpoint_controller.go#L111-L127 and just init it with what is used in this controller?

}

// Check if KeystoneAPI is ready
keystoneAPI, err := keystonev1.GetKeystoneAPI(ctx, helperObj, instance.Namespace, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logger.Info("KeystoneAPI not ready, requeue")
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I'd move all the above tasks into the main reconcile func as these are also the steps done for handling deletion. then you won't need login to check for the keystoneapi in the cleanup method. like https://github.com/openstack-k8s-operators/keystone-operator/blob/main/controllers/keystoneendpoint_controller.go#L225 where the admin client got checked before

logger := r.GetLogger(ctx)
adminOS, ctrlResult, err := keystonev1.GetAdminServiceClient(ctx, helperObj, keystoneAPI)
if err != nil {
return "", err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if err != nil {
logger.Error(err, "Failed to find user ID")
instance.Status.Conditions.Set(condition.FalseCondition(
condition.ReadyCondition,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be not the correct condition. the ReadyCondition is only set in the defer function based on the sub condition status during the reconcile

}
savedConditions := instance.Status.Conditions.DeepCopy()

// Defer patch logic (skips if we are deleting)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keystoneAPI, err := keystonev1.GetKeystoneAPI(ctx, helperObj, instance.Namespace, nil)
if err == nil && keystoneAPI.IsReady() {
userID, userErr := r.findUserIDAsAdmin(ctx, helperObj, keystoneAPI, instance.Spec.UserName)
if userErr == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try to reduce those amount of nested ifs.

what if there is an userErr, which is not "user not found"? is it ok continue and remove the finalizer? shouldn't we return the actual error if it is not "user not found" ?

can't we just do

userID, userErr := r.findUserIDAsAdmin(ctx, helperObj, keystoneAPI, instance.Spec.UserName)
if userErr != nil {
   return ctrl.Result{}, err
}
userOS, userRes, userErr2 := keystonev1.GetUserServiceClient(ctx, helperObj, keystoneAPI, instance.Spec.UserName)
...

) (string, error) {

// The name of the Secret containing the service passwords
const ospSecretName = "osp-secret"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we should not hardcode any value like this. It could be part of the deployment YAML spec.

return "", fmt.Errorf("failed to get Secret/%s: %w", ospSecretName, err)
}

key := capitalizeFirst(userName) + "Password"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to capitalize the username's first letter? Also, why are you harcoding "Password"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just to filter user password from osp-secret. Now, passwordSelector will be passed to AC CR, so that will be used for filtering

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. Can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous code assumed this password scheme in osp-secret:

BarbicanPassword: <secret_data>
CinderPassword: <secret_data>
GlancePassword: <secret_data>
…

So to pick the right field we had to:

  1. Take the userName ("barbican")
  2. Capitalize the first letter → "Barbican"
  3. Append "Password" → "BarbicanPassword"

and then look up that key in the secret. Of course that'd force everyone to use that convention. So now openstack-operator extracts and passes into AC CR these as well:

spec:
  secret: # the Secret name (by default it's osp-secret)
  passwordSelector:  # how we extract this key, e.g. BarbicanPassword
  userName: # e.g. barbican, user can customize this in control plane spec
  …

@Deydra71 Deydra71 force-pushed the underlined-ac-support branch from 38cb512 to 15fcdb9 Compare May 14, 2025 11:50
@Deydra71
Copy link
Contributor Author

The latest update makes the controller take into consideration the custom password secret, custom service user name, and password selectors. Continuing to address other reviews.

@Deydra71 Deydra71 force-pushed the underlined-ac-support branch from 15fcdb9 to 2d8a489 Compare May 16, 2025 13:50
@Deydra71
Copy link
Contributor Author

Deydra71 commented May 16, 2025

Latest update includes corrections based on some reviews (not all, will still continue), and also adds support for the Unrestricted, Roles and AccessRoles fields.

Currently AccessRoles field is not correctly passed to the AC CR by openstack-operator, so it's missing in the AC CR, and for services this field is specified in the openstackcontrolplane spec, ACs are not generated. We are continuing on Slack what should be in AccessRules fields in the first place.

And also automatic revocation is disabled for now for testing, based on what we will agree it will be enabled again.

@Deydra71 Deydra71 force-pushed the underlined-ac-support branch from 2d8a489 to 4962009 Compare May 19, 2025 12:35
scope := &gophercloud.AuthScope{
ProjectName: "service",
DomainName: "Default",
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very wary about hard-coding stuff here. I think that;

  1. there may be cases where either the service project or DomainName may be different
  2. we might be interested in obtaining an app cred for a non-service user or for a different project. For example, I know that there is work ongoing upstream to do manila share encryption where the user would use app creds to retrieve a barbican secret.

In any case, I think it makes sense to make this function more generic - maybe call it GetUserClient() and pass in the domain and project name. You could then add these parameters to the app cred spec, and default to "service" and "Default". This will future-proof the interface a bit.

Copy link
Contributor Author

@Deydra71 Deydra71 May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, but right now we don't have any spec.ServiceProject or spec.ServiceDomain. So, that's why this serves as a mean to only get scoped token for our service.

Since FR3 is explicitly about wiring up service-account auth, I suggest we leave this helper as is for now, and add a // TODO pointing at the future work to extend the CRD with project/domain fields and refactor into a generic GetUserClient(…, project, domain…)

The domain name is actually hard coded for admin as well - https://github.com/openstack-k8s-operators/keystone-operator/blob/main/api/v1beta1/keystoneapi.go#L157

Copy link
Contributor Author

@Deydra71 Deydra71 May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there may be cases where either the service project or DomainName may be different

Is this still true if we take into account only service users?

AuthURL: authURL,
Username: userName,
Password: password,
TenantName: "service",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto comment as above.

secretName := fmt.Sprintf("%s-secret", instance.Name)
if err := r.storeACSecret(ctx, helperObj, instance, secretName, newID, newSecret); err != nil {
return ctrl.Result{}, err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The keystone-operator is somewhat unique in the operators in that there isn't a lot of documentation of what it does. Just looking at the code , for instance, it wasn't completely clear to me what the name of the ac would be, and the name of the secret etc. I'm sure I could figure it out - but I shouldn't have to work so hard.

It would be great if there were a small doc (maybe in a docs directory like the other operators) which contains a description of what we'd expect this operator to produce. It doesn't have to be fancy, but it will help future reviewers and coders who want to enhance this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the doc dir and a new file describing basic functionality of the AC controller - will add more details soon.

@softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/179a05bbe7c94c938c402967d4766f8f

openstack-k8s-operators-content-provider FAILURE in 12m 20s
⚠️ keystone-operator-kuttl SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider (non-voting)
⚠️ keystone-operator-tempest SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider (non-voting)

@apevec
Copy link
Contributor

apevec commented Nov 26, 2025

why does this have do-not-merge label?

@Deydra71
Copy link
Contributor Author

why does this have do-not-merge label?

Thanks, it was a remnant when stuff fro FR4 were getting merged

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 1, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Deydra71
Once this PR has been reviewed and has the lgtm label, please assign dprince for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@softwarefactory-project-zuul
Copy link

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.
Warning:
Error merging github.com/openstack-k8s-operators/keystone-operator for 567,02bb4f592a31b8627ea60250fc494cc2b4096ebd

…r functions for service operators.

Signed-off-by: Veronika Fisarova <[email protected]>
@Deydra71 Deydra71 force-pushed the underlined-ac-support branch from 2823ce6 to 5937d02 Compare December 4, 2025 08:07
@softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/be4da1f98b18431ebbffeb9f6b27f75b

openstack-k8s-operators-content-provider FAILURE in 6m 45s
⚠️ keystone-operator-kuttl SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider (non-voting)
⚠️ keystone-operator-tempest SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider (non-voting)

Comment on lines +192 to +193
ProjectName: "service",
DomainName: "Default",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the details for keystone related to Domains, therefore maybe a bad question. In a multi region setup, would it still be the default Domain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, still Default domain. Domain is a global concept iin Keystone, and in the SKMO design the user uniqueness is achieved by unique usernames (nova-region1, nova-region2...). Also the domain is hardcoded to default across controlplane and not configurable atm (except for heat_stack_domain_admin, but we are not creating AppCred for this user because it's domain-based, and KEystone doesn't allow to create AppCred for domain-based users, only project-scoped, or unscoped).

Comment on lines +222 to +226
if ospSecretName == "" {
return "", ctrl.Result{}, fmt.Errorf("secret name is empty, cannot retrieve password for selector %q", passwordSelector)
}
if passwordSelector == "" {
return "", ctrl.Result{}, fmt.Errorf("password selector is empty, cannot retrieve password from secret %q", ospSecretName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't hurt, but in theory we should never hit that since afaik they either have a default or are required fields.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment on the keystoneapplicationcredential_types.go would that remove the need for these checks here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agree, will add the kube validations for PasswordSelector and Secret fields.


// PasswordSelector for extracting the service password
// +kubebuilder:validation:Optional
PasswordSelector string `json:"passwordSelector"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could add // +kubebuilder:validation:MinLength=1 to not allow empty string "". this would remove the need for checking for the empty things in the funcs above.

// +kubebuilder:validation:Optional
// +kubebuilder:default=365
// +kubebuilder:validation:Minimum=2
ExpirationDays int `json:"expirationDays"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what should be the right value for an edpm service, like nova-compute. do they need a dedicated credential which expires really long in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expirationDays and gracePeriodDays have global defaults, but both are configurable per service in the controlplane CR, so each service user can be configured differently. I think we should keep the decision about the expiration of AppCred that affects EDPM on the cluster admin - with a documentation note that they should set expiration and grace periods based on their case.

Also when the AppCred rotates, the controller emits a kube Event that this credential was rotated, when it expires, and that it may affect EDPM redeploy. User will also see right away in status fields the last rotated and expiration timestamps to be aware of that:

   status:
     createdAt: "2025-12-05T10:00:00Z"
     expiresAt: "2026-12-05T10:00:00Z"
     lastRotated: "2025-12-05T10:00:00Z"
     rotationEligibleAt: "2026-06-05T10:00:00Z"  # expiresAt - gracePeriod

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, they could see it there, if they check those status. I think we'd have to highlight this with a big note in the edpm deployment step as I am just not sure if 365 days with GracePeriodDays 182 is ok for really big envs and that they'd have to customize it if needed. Or maybe it is just me :)


// VerifySecret returns res.RequeueAfter > 0 when secret not found (not an error)
// For AC, this is optional, so we just skip it instead of requeueing
if res.RequeueAfter > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if you'd call secret.VerifySecret with a 0 timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a really good catch I didn't notice... With the current sctructure passing timeout equal to 0 breaks the logic if the secret is not found, which means that verifySecret returns res == ctrl.Result{RequeueAfter: timeout} we skip the res.RequeueAfter > 0 check, so we end up with empty hash. We can change it to:

if hash == "" {
        // AC secret not found - this is OK, service will use password auth
        return ctrl.Result{}, nil
    }

And not rely on the requeue value returned. Does it make sense?

Comment on lines +103 to +124
// Setup helper for use in reconciles
helperObj, err := helper.NewHelper(instance, r.Client, r.Kclient, r.Scheme, logger)
if err != nil {
return ctrl.Result{}, err
}

// Save and defer patching all subconditions and overall Ready
saved := instance.Status.Conditions.DeepCopy()
defer func() {
if instance.DeletionTimestamp.IsZero() {
condition.RestoreLastTransitionTimes(&instance.Status.Conditions, saved)
// ApplicationCredentialReady being True implies KeystoneAPIReady is True,
// so when ApplicationCredential is ready, setup is complete
if instance.Status.Conditions.IsTrue(keystonev1.KeystoneApplicationCredentialReadyCondition) {
instance.Status.Conditions.MarkTrue(condition.ReadyCondition, keystonev1.KeystoneApplicationCredentialReadyMessage)
} else {
instance.Status.Conditions.MarkUnknown(condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage)
instance.Status.Conditions.Set(instance.Status.Conditions.Mirror(condition.ReadyCondition))
}
_ = helperObj.PatchInstance(ctx, instance)
}
}()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment above on the reordering

instance *keystonev1.KeystoneApplicationCredential,
) (ctrl.Result, error) {

// Before removing our CR finalizer, allow ApplicationCredential Secret to be deleted by removing its protection finalizer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we first have to clean it up in keystone by doing an keystoneapi call, similar to how we do it in the keustoneendpoint https://github.com/openstack-k8s-operators/keystone-operator/blob/main/internal/controller/keystoneendpoint_controller.go#L255 ? otherwise we end up in the AC still be there in keystone? or is there some automatic cleanup in keystone and we just let it expire?


createOpts := applicationcredentials.CreateOpts{
Name: newACName,
Description: fmt.Sprintf("Created by keystone-operator for user %s", ac.Spec.UserName),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this have the owning object name as a reference?

Comment on lines +437 to +438
"AC_ID": newID,
"AC_SECRET": newSecret,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the keys should be const? can then be used in the other places.

}

instance.Status.Conditions.MarkTrue(keystonev1.KeystoneApplicationCredentialReadyCondition, keystonev1.KeystoneApplicationCredentialReadyMessage)
nextCheck := r.computeNextRequeue(instance)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this if the min ExpirationDays is 2 days? per default SyncPeriod, the operators reconcile is every 10 or 12h. is cert-manager doing something like this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants