Adding a status for workload identity

[grac3gao-zz]

Fixes #668

Proposed Changes

• Add a status for workload identity

Release Note

Docs

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: grac3gao
To complete the pull request process, please assign harwayne
You can assign the PR to them by writing /assign @harwayne in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/duck/v1alpha1/identity.go	Do not exist	100.0%
pkg/apis/events/v1alpha1/cloudauditlogssource_types.go	71.4%	75.0%	3.6
pkg/apis/events/v1alpha1/cloudpubsubsource_types.go	84.6%	85.7%	1.1
pkg/apis/events/v1alpha1/cloudschedulersource_types.go	66.7%	71.4%	4.8
pkg/apis/events/v1alpha1/cloudstoragesource_types.go	66.7%	71.4%	4.8
pkg/reconciler/events/auditlogs/auditlogs.go	92.2%	89.7%	-2.5
pkg/reconciler/events/pubsub/pubsub.go	73.5%	68.6%	-5.0
pkg/reconciler/events/scheduler/scheduler.go	84.3%	81.7%	-2.6
pkg/reconciler/events/storage/storage.go	82.3%	80.0%	-2.3
pkg/reconciler/identity/reconciler.go	76.3%	75.3%	-1.0
pkg/reconciler/messaging/channel/channel.go	80.9%	79.7%	-1.1

[nachocano]

it seems that we should be able to reuse Condition from pkg for these?
See https://github.com/knative/pkg/blob/0840da9555a3a75f801abc1d654fb00dfe9a687a/apis/condition_types.go#L58?
And you will get more type-safety.
The duckv1.SourceStatus has a Status, which has a list of Conditions.
I'm not entirely sure we should be adding these ones here...

[nachocano]

I think I'd rather keep it simple for now to see how this workload identity places out.
And would just add a condition to the types called something like "WorkloadIdentityConfigured" or something like that, not part of the NewLivingConditionSet...
and provide methods to MarkWorkloadIdentityConfigured and not, with proper reasons....
We can use the Unknown when users don't specify a serviceAccount... nothing will be set, but that won't affect the readiness.
We can use True when users specified the serviceAccount and it configured correctly.
We can use False when users specified the serviceAccount and it wasn't configured properly. In this latter case, we should also Mark Ready = false to the object.

I don't think we need to save Enabled in status. But it would be nice if we save the k8s service account, what you are currently doing.

I think this will simplify things quite a bit... but want to hear your thoughts as you have done all the work here.

[grac3gao-zz]

I'll change that, and save the k8s service account name. I'll combine the new commit with change of k8s name.

[grac3gao-zz]

It seems I can only set the condition in every reconciler, rather than combine it to the identity reconciler, if we use the Conditions for WI.

The problem is still for the status. If I want to mark the condition status in the identity reconciler, I need to get sources/channel status and api.condtionSet. Then I can use sourceStatus.Manager(api.condtionSet).MarkTrue to mark the condition.

I can get the condSet, but I cannot create an identifiable interface function for getting all sources and channel’s status, the only common thing shared by sources and channel’s status is they all use duck.status. Then I have to write things like (s *duck.status)MarkworkloadIdentityReconciled. I am not sure if we can do that.

[nachocano]

it feels that the WorkloadIdentityCondition should be added to this Status...
And if you have workload identity enabled, but not working, then you can mark that condition failed, and also Mark the ready false...
not entirely sure though

[nachocano]

@grac3gao I created a sample PR here for you to take a look: #703. It was more involved than I initially thought.
There are a bunch of cleanups that are needed. Especially in the lifecycles of Sources, we have unnecessary methods...

fyi @Harwayne in case you pick up the v1beta1 task, there are a bunch of cleanups to do first..

[knative-prow-robot]

@grac3gao: PR needs rebase.

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.

[grac3gao-zz]

@nachocano Thank you for adding that sample PR! By including the pubsubStatus in the identifiable, I can mark the WI condition in the identity reconciler now! It seems I also need to change Channel status to use pubsubStatus?

For serviceAccount, I am fine with gsa, but I am worried about that it might be too brief for user to understand? Perhaps gServiceAccount would be a choice?

I wrote a rough change for the status condition last night, but that is about marking the status in every reconciler. I think it might be better to change all the things (adding status condition, change name) together in a later PR, after determining the name and add the E2E test.

I'll close this PR and focus on E2E test now. After finishing that, I'll go back to condition. The sample PR you provided is a good guide for me to add the condition :)

[nachocano]

Sounds like a plan! Yeah gsa might not be correct as well, and gServiceAccount sounds better. I think we need a third opinion, maybe Adam? I think Channel status shouldn't use pubsubStatus, might have missed that in the code, not sure. Agree let's do that in other PRs. E2E tests are top priority now. Thanks grace!

…

On Wed, Mar 25, 2020 at 9:24 AM Grace Gao ***@***.***> wrote: @nachocano <https://github.com/nachocano> Thank you for adding that sample PR! By including the pubsubStatus in the identifiable, I can mark the WI condition in the identity reconciler now! It seems I also need to change Channel status to use pubsubStatus? For serviceAccount, I am fine with gsa, but I am worried about that it might be too brief for user to understand? Perhaps gServiceAccount would be a choice? I wrote a rough change for the status condition last night, but that is about marking the status in every reconciler. I think it might be better to change all the things (adding status condition, change name) together in a later PR, after determining the name and add the E2E test. I'll close this PR and focus on E2E test now. After finishing that, I'll go back to condition. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#692 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABD65DAE4ESFKKMIWIIOWH3RJIV43ANCNFSM4LQZNLXQ> .