Refactor the AuthStatus Logic in Eventing OIDC Feature Track to reduce repetitive code

[Leo6Leo]

Overview: Refactor the AuthStatus Logic in Eventing OIDC Feature Track to reduce repetitive code

Background:

The Eventing OIDC feature track introduces the AuthStatus with the aim to reflect the generated service account name within the resource status.

Current Behavior of a reconciler:

Based on the feature flag status:

• Disabled: No behavior changes.
• Enabled:
  1. Creation of a service account for the managing resource's identity (e.g., Triggers' identity).
  2. Service account's name is displayed under .status.auth.serviceAccountName.

Problem:

There's evident code duplication across different reconcilers, implying redundancy:

eventing/pkg/reconciler/broker/trigger/trigger.go

Lines 147 to 161 in f9314d8

	if featureFlags.IsOIDCAuthentication() {
	saName := auth.GetOIDCServiceAccountNameForResource(eventingv1.SchemeGroupVersion.WithKind("Trigger"), t.ObjectMeta)
	t.Status.Auth = &duckv1.AuthStatus{
	ServiceAccountName: &saName,
	}
	if err := auth.EnsureOIDCServiceAccountExistsForResource(ctx, r.serviceAccountLister, r.kubeclient, eventingv1.SchemeGroupVersion.WithKind("Trigger"), t.ObjectMeta); err != nil {
	t.Status.MarkOIDCIdentityCreatedFailed("Unable to resolve service account for OIDC authentication", "%v", err)
	return err
	}
	t.Status.MarkOIDCIdentityCreatedSucceeded()
	} else {
	t.Status.Auth = nil
	t.Status.MarkOIDCIdentityCreatedSucceededWithReason(fmt.Sprintf("%s feature disabled", feature.OIDCAuthentication), "")
	}

eventing/pkg/reconciler/apiserversource/apiserversource.go

Lines 103 to 117 in f9314d8

	if featureFlags.IsOIDCAuthentication() {
	saName := auth.GetOIDCServiceAccountNameForResource(v1.SchemeGroupVersion.WithKind("ApiServerSource"), source.ObjectMeta)
	source.Status.Auth = &duckv1.AuthStatus{
	ServiceAccountName: &saName,
	}
	if err := auth.EnsureOIDCServiceAccountExistsForResource(ctx, r.serviceAccountLister, r.kubeClientSet, v1.SchemeGroupVersion.WithKind("ApiServerSource"), source.ObjectMeta); err != nil {
	source.Status.MarkOIDCIdentityCreatedFailed("Unable to resolve service account for OIDC authentication", "%v", err)
	return err
	}
	source.Status.MarkOIDCIdentityCreatedSucceeded()
	} else {
	source.Status.Auth = nil
	source.Status.MarkOIDCIdentityCreatedSucceededWithReason(fmt.Sprintf("%s feature disabled", feature.OIDCAuthentication), "")
	}

Proposal:

For maintainability and DRY (Don't Repeat Yourself) principles, it's suggested that this logic should be refactored into a common utility or module. By doing so, reconcilers, especially those under the Eventing Sender Identity Project, would be able to leverage this shared functionality.

Additional Notes & Reference

          That's a good point. As we have this in multiple places (each reconciler which creates an OIDC SA), we could track this also in a separate issue (and reuse it here already)

Originally posted by @creydr in #7344 (comment)

/good-first-issue

[creydr]

Thanks for creating it @Leo6Leo. As this could be a good-first-issue: may I ask you to add a few more details what to achieve, so others not following the discussions have it easier to get started with it?

[Leo6Leo]

/good-first-issue

[knative-prow]

@Leo6Leo:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

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/test-infra repository.

[xiangpingjiang]

/assign

[Leo6Leo]

Hey @xiangpingjiang, Thank you for your interest in this issue! Just a heads-up: it's currently in the draft phase, as indicated by the project status. Once we've triaged and accepted it, the status will shift to "Ready". At that point, you're good to dive in and begin your work!

Thanks again for your contribution and patience!

[creydr]

@Leo6Leo thanks for adjusting the description. I think this is ready to be worked then. I'll move it to ready status then

[Leo6Leo]

Hey @xiangpingjiang , are there anything that I could help with your ticket?

[xiangpingjiang]

Hello @Leo6Leo
Thanks a lot ,where could I put the common utility or module?, in knative.dev/eventing/pkg/auth module ?

[creydr]

Hello @Leo6Leo Thanks a lot ,where could I put the common utility or module?, in knative.dev/eventing/pkg/auth module ?

Hey @xiangpingjiang,
Yes, I think knative.dev/eventing/pkg/auth would be a good start

[xiangpingjiang]

hello @creydr
Differant status, such as TriggerStatus and ApiServerSourceStatus, they both have MarkOIDCIdentityCreatedSucceeded, MarkOIDCIdentityCreatedSucceededWithReason, MarkOIDCIdentityCreatedFailed func , TriggerStatus and ApiServerSourceStatus are as Pointer receivers. For Differant status , how can I put the in one func or utility or module ? Thank you

[creydr]

Hey @xiangpingjiang,
you could define a new interface having the required methods (MarkOIDCIdentityCreatedSucceeded, MarkOIDCIdentityCreatedSucceededWithReason, MarkOIDCIdentityCreatedFailed) and then use this as a parameter.
As the TriggerStatus, SubscriptionStatus, ... have those methods, they comply with this interface.

So maybe something like the following?

type OIDCStatusMarker interface { <- maybe find a better name :)
  MarkOIDCIdentityCreatedSucceeded()
  MarkOIDCIdentityCreatedSucceededWithReason(reason, messageFormat string, messageA ...interface{})
  MarkOIDCIdentityCreatedFailed(reason, messageFormat string, messageA ...interface{})
}

func yourUtilityFunction(someArgs..., marker OIDCStatusMarker) {
  ...
  marker.MarkOIDCIdentityCreatedSucceeded()
}

and in the reconcilers pass the status, as this complies with your interface:

auth.yourUtilityFunction(..., &trigger.status)

[xiangpingjiang]

@creydr
Thanks a lot, I will have a try.