Skip to content
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

Add AuthStatus and add to OIDC identity providing resources #7173

Closed
creydr opened this issue Aug 15, 2023 · 13 comments · Fixed by knative/pkg#2829 or #7287
Closed

Add AuthStatus and add to OIDC identity providing resources #7173

creydr opened this issue Aug 15, 2023 · 13 comments · Fixed by knative/pkg#2829 or #7287
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines.

Comments

@creydr
Copy link
Member

creydr commented Aug 15, 2023

Problem

As the Eventing OIDC feature track describes, the AuthStatus is meant to provide the generated service account name in the resource status.

We should:

Time Estimate (optional)

1

Additional context (optional)

/good-first-issue

@creydr creydr added the kind/TBD Parked issue that required triaging/revisit in a near future. label Aug 15, 2023
@knative-prow
Copy link

knative-prow bot commented Aug 15, 2023

@creydr:
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:

Problem

As the Eventing OIDC feature track describes, the AuthStatus is meant to provide the generated service account name in the resource status.

We should:

  • add the AuthStatus struct which will look the following:
type AuthStatus struct {
	// ServiceAccountName is the name of the generated service account 
	// used for this components OIDC authentication.
	ServiceAccountName *string `json:"serviceAccountName,omitempty"`
}
  • add the AuthStatus API in sending resources:
  • Trigger
  • Subscription
  • Sequence
  • Parallel
  • Sources of eventing core (ApiServerSource, PingSource, ContainerSource, SinkBinding)

Time Estimate (optional)

1

Additional context (optional)

/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.

@knative-prow knative-prow bot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Aug 15, 2023
@creydr creydr removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Aug 17, 2023
@creydr creydr changed the title [DRAFT] Add AuthStatus and add to sending resources [DRAFT] Add AuthStatus and add to OIDC identity providing resources Sep 5, 2023
@creydr creydr changed the title [DRAFT] Add AuthStatus and add to OIDC identity providing resources Add AuthStatus and add to OIDC identity providing resources Sep 5, 2023
@creydr creydr added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. and removed kind/TBD Parked issue that required triaging/revisit in a near future. labels Sep 5, 2023
@karthikmurali60
Copy link
Contributor

/assign

@creydr
Copy link
Member Author

creydr commented Sep 18, 2023

Hello @karthikmurali60,
just wanted to check, if you're still interested in this issue and when you're planning to work on it roughly?

@karthikmurali60
Copy link
Contributor

Hi @creydr
Sorry for the delay, I will start working on this issue now

@creydr
Copy link
Member Author

creydr commented Sep 19, 2023

Hey @karthikmurali60,
thanks for the update. Just FYI: we had a quick discussion on Slack where to place the AuthStatus struct best (https://cloud-native.slack.com/archives/C04LMU33V1S/p1695117591666649). And we agreed that pkg/apis/duck/v1/auth_types.go in the pkg repo might be a good place for the struct.

@karthikmurali60
Copy link
Contributor

Thanks for the update, will make the changes accordingly

@karthikmurali60
Copy link
Contributor

karthikmurali60 commented Sep 20, 2023

@creydr I need some help.

I have added AuthStatus struct in the pkg repo. Should I add any helper/wrapper functions along with it ?

Also, I am a bit confused regarding what needs to be done in the 2nd part. I am not able to figure out where the AuthStatus API needs to be added for the list mentioned (Trigger, Subscription, Sequence, Parallel and sources of eventing core). Can you please point out where these changes need to be done ?

@creydr
Copy link
Member Author

creydr commented Sep 20, 2023

@creydr I need some help.

I have added AuthStatus struct in the pkg repo. Should I add any helper/wrapper functions along with it ?

I think we should be fine without any helper/wrapper for now.

Also, I am a bit confused regarding what needs to be done in the 2nd part. I am not able to figure out where the AuthStatus API needs to be added for the list mentioned (Trigger, Subscription, Sequence, Parallel and sources of eventing core). Can you please point out where these changes need to be done ?

Hello @karthikmurali60,
sure. Let me try to explain a bit more: The above mentioned resources (Triggers, Subscriptions, ...) need to announce later their service account name which they will use for OIDC authentication. The auth struct is a new struct, which contains this field (where the name later gets written to). The resources will provide this in their .status. In the example of the trigger, this would be in the TriggerStatus struct, something like the following:

type TriggerStatus struct {

        ...

	// Auth provides the relevant information for OIDC authentication.
	// +optional
	Auth *eventingduckv1.AuthStatus `json:"auth,omitempty"`

Afterwards you need to run the ./hack/update-codegen.sh to update the generated files (e.g. for the deepcopy functions).

Unfortunately we then have to add this information in the CRD manually too. In the case of the Trigger this would be in the config/core/resources/trigger.yaml, by adding something like the following to its status section:

(under ...status.properties:)
...

auth:
  description: Auth provides the relevant information for OIDC authentication.
  type: object
  properties:
    serviceAccountName:
      description: ServiceAccountName is the name of the generated service account used for this components OIDC authentication.
      type: string

This then needs to be added to the Subscriptions (type and CRD), Sequences (type and CRD) and Parallels (type and CRD) too.

For the sources of eventing core, you could reference the AuthStruct in the SourceStatus (in pkg repo - would do it as a separate PR to adding the AuthStruct type in the pkg repo), because the core sources use this as their status. Then you would only need to get the latest changes of pkg into eventing repo (you can use the ./hack/update-deps.sh script) and update the CRDs of the sources.

For channels, we also need to add it to the SubscriberStatus and update the CRDs (channel.yaml and in-memory-channel.yaml)

Does this help you to get started?
If you want we can also break this issue up into multiple to have a finer breakdown of the tasks.

@pierDipi do you have anything to add? Or did I miss something in your opinion?

@pierDipi
Copy link
Member

That seems accurate @creydr.

@karthikmurali60 the pkg changes you've done need to be pushed/merged first before moving forward with the 2nd part, so first I'd open the PR in knative/pkg with:

  1. the addition of the AuthStatus type
  2. the addition of the Auth *eventingduckv1.AuthStatus `json:"auth,omitempty" field to SourceStatus

After the PR is merged with the above changes we can continue with the second part which will cover the remaining resources in knative/eventing with similar changes to the point 2 above.

Hope that helps

@creydr
Copy link
Member Author

creydr commented Sep 22, 2023

Reopening this, as knative/pkg#2829 was only a part of this

@creydr creydr reopened this Sep 22, 2023
@karthikmurali60
Copy link
Contributor

For the sources of eventing core, you could reference the AuthStruct in the SourceStatus (in pkg repo - would do it as a separate PR to adding the AuthStruct type in the pkg repo), because the core sources use this as their status. Then you would only need to get the latest changes of pkg into eventing repo (you can use the ./hack/update-deps.sh script) and update the CRDs of the sources.

@creydr for sources of eventing core, I have gotten the latest changes of pkg into eventing repo using the update-deps script. Which are the CRDs of sources that i need to update? (is it pingsource.yaml ?)

@creydr
Copy link
Member Author

creydr commented Sep 22, 2023

Seems the changes from knative/pkg#2829 are on their way into the eventing main branch 😃 #7284

@creydr
Copy link
Member Author

creydr commented Sep 22, 2023

Which are the CRDs of sources that i need to update? (is it pingsource.yaml ?)

ApiServerSource, PingSource, ContainerSource and SinkBinding (you can find the CRDs in config/core/resources)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines.
Projects
Status: ✅ Done
3 participants