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.

Sign up for GitHub

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

support auto generation of SinkBindings identity service account #7327

Merged
merged 11 commits into from
Oct 11, 2023
Merged

support auto generation of SinkBindings identity service account #7327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
71d68a4
support auto generation of SinkBindings identity service account
rahulii Oct 2, 2023
6409b61
pass serviceAccountLister to reconciler
rahulii Oct 2, 2023
88f52b4
give service account permission and minor fixes
rahulii Oct 4, 2023
7e48b41
fix review comments
rahulii Oct 4, 2023
910f581
fix linting and review comments
rahulii Oct 5, 2023
426c975
fix review comments
rahulii Oct 5, 2023
d0c24d0
fix test
rahulii Oct 5, 2023
650ae20
review comments
rahulii Oct 9, 2023
5bf370c
add handler to feature store
rahulii Oct 10, 2023
ac09c62
fix lint
rahulii Oct 10, 2023
fdda99e
remove comment
rahulii Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions config/core/roles/webhook-clusterrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ rules:
- "create"
- "patch"

rahulii marked this conversation as resolved.
Show resolved Hide resolved
# For the SinkBinding reconciler adding the OIDC identity service accounts
- apiGroups:
- ""
resources:
- "serviceaccounts"
verbs: *everything
# Necessary for conversion webhook. These are copied from the serving
# TODO: Do we really need all these permissions?
- apiGroups: ["apiextensions.k8s.io"]
Expand Down
17 changes: 17 additions & 0 deletions pkg/apis/sources/v1/sinkbinding_lifecycle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (

var sbCondSet = apis.NewLivingConditionSet(
SinkBindingConditionSinkProvided,
SinkBindingConditionOIDCIdentityCreated,
)

// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
Expand Down Expand Up @@ -95,6 +96,22 @@ func (sbs *SinkBindingStatus) MarkSink(addr *duckv1.Addressable) {
}
}

func (sbs *SinkBindingStatus) MarkOIDCIdentityCreatedSucceeded() {
sbCondSet.Manage(sbs).MarkTrue(SinkBindingConditionOIDCIdentityCreated)
}

func (sbs *SinkBindingStatus) MarkOIDCIdentityCreatedSucceededWithReason(reason, messageFormat string, messageA ...interface{}) {
sbCondSet.Manage(sbs).MarkTrueWithReason(SinkBindingConditionOIDCIdentityCreated, reason, messageFormat, messageA...)
}

func (sbs *SinkBindingStatus) MarkOIDCIdentityCreatedFailed(reason, messageFormat string, messageA ...interface{}) {
sbCondSet.Manage(sbs).MarkFalse(SinkBindingConditionOIDCIdentityCreated, reason, messageFormat, messageA...)
}

func (sbs *SinkBindingStatus) MarkOIDCIdentityCreatedUnknown(reason, messageFormat string, messageA ...interface{}) {
sbCondSet.Manage(sbs).MarkUnknown(SinkBindingConditionOIDCIdentityCreated, reason, messageFormat, messageA...)
}

// Do implements psbinding.Bindable
func (sb *SinkBinding) Do(ctx context.Context, ps *duckv1.WithPod) {
// First undo so that we can just unconditionally append below.
Expand Down
34 changes: 34 additions & 0 deletions pkg/apis/sources/v1/sinkbinding_lifecycle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,43 @@ func TestSinkBindingStatusIsReady(t *testing.T) {
s.InitializeConditions()
s.MarkSink(sink)
s.MarkBindingAvailable()
s.MarkOIDCIdentityCreatedSucceeded()
return s
}(),
want: true,
}, {
name: "mark OIDC identity created",
s: func() *SinkBindingStatus {
s := &SinkBindingStatus{}
s.InitializeConditions()
s.MarkSink(sink)
s.MarkBindingAvailable()
s.MarkOIDCIdentityCreatedSucceeded()
return s
}(),
want: true,
}, {
name: "mark OIDC identity created with reason",
s: func() *SinkBindingStatus {
s := &SinkBindingStatus{}
s.InitializeConditions()
s.MarkSink(sink)
s.MarkBindingAvailable()
s.MarkOIDCIdentityCreatedSucceededWithReason("TheReason", "feature is disabled")
return s
}(),
want: true,
}, {
name: "mark OIDC identity created failed",
s: func() *SinkBindingStatus {
s := &SinkBindingStatus{}
s.InitializeConditions()
s.MarkSink(sink)
s.MarkBindingAvailable()
s.MarkOIDCIdentityCreatedFailed("TheReason", "this is a message")
return s
}(),
want: false,
}}

for _, test := range tests {
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/sources/v1/sinkbinding_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ const (
// SinkBindingConditionSinkProvided is configured to indicate whether the
// sink has been properly extracted from the resolver.
SinkBindingConditionSinkProvided apis.ConditionType = "SinkProvided"

// SinkBindingConditionOIDCIdentityCreated is configured to indicate whether
// the OIDC identity has been created for the sink.
SinkBindingConditionOIDCIdentityCreated apis.ConditionType = "OIDCIdentityCreated"
)

// SinkBindingStatus communicates the observed state of the SinkBinding (from the controller).
Expand Down
64 changes: 60 additions & 4 deletions pkg/reconciler/sinkbinding/controller.go
View file
Open in desktop
rahulii marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,33 @@ package sinkbinding
import (
"context"
"errors"
"fmt"

"knative.dev/eventing/pkg/auth"
sbinformer "knative.dev/eventing/pkg/client/injection/informers/sources/v1/sinkbinding"
"knative.dev/pkg/client/injection/ducks/duck/v1/podspecable"
"knative.dev/pkg/client/injection/kube/informers/core/v1/namespace"
"knative.dev/pkg/reconciler"
"knative.dev/pkg/resolver"
"knative.dev/pkg/system"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/scheme"
typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1"
corev1listers "k8s.io/client-go/listers/core/v1"
"k8s.io/client-go/tools/cache"
"k8s.io/client-go/tools/record"
"knative.dev/eventing/pkg/apis/feature"
v1 "knative.dev/eventing/pkg/apis/sources/v1"
"knative.dev/pkg/apis/duck"
duckv1 "knative.dev/pkg/apis/duck/v1"
kubeclient "knative.dev/pkg/client/injection/kube/client"
configmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap"
serviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount"
"knative.dev/pkg/configmap"
"knative.dev/pkg/controller"
"knative.dev/pkg/injection/clients/dynamicclient"
Expand All @@ -50,8 +59,11 @@ const (
)

type SinkBindingSubResourcesReconciler struct {
res *resolver.URIResolver
tracker tracker.Interface
res *resolver.URIResolver
tracker tracker.Interface
serviceAccountLister corev1listers.ServiceAccountLister
kubeclient kubernetes.Interface
featureStore *feature.Store
}

// NewController returns a new SinkBinding reconciler.
Expand All @@ -65,6 +77,9 @@ func NewController(
dc := dynamicclient.Get(ctx)
psInformerFactory := podspecable.Get(ctx)
namespaceInformer := namespace.Get(ctx)
serviceaccountInformer := serviceaccountinformer.Get(ctx)
configmapInformer := configmapinformer.Get(ctx)

c := &psbinding.BaseReconciler{
LeaderAwareFuncs: reconciler.LeaderAwareFuncs{
PromoteFunc: func(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error {
Expand Down Expand Up @@ -94,13 +109,22 @@ func NewController(
Logger: logger,
})

featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"), func(name string, value interface{}) {
impl.GlobalResync(sbInformer.Informer())
})

featureStore.WatchConfigs(cmw)

sbInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))
namespaceInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))

sbResolver := resolver.NewURIResolverFromTracker(ctx, impl.Tracker)
c.SubResourcesReconciler = &SinkBindingSubResourcesReconciler{
res: sbResolver,
tracker: impl.Tracker,
res: sbResolver,
tracker: impl.Tracker,
kubeclient: kubeclient.Get(ctx),
serviceAccountLister: serviceaccountInformer.Lister(),
featureStore: featureStore,
}

c.WithContext = func(ctx context.Context, b psbinding.Bindable) (context.Context, error) {
Expand All @@ -114,6 +138,20 @@ func NewController(
},
}

// Reconcile SinkBinding when the OIDC service account changes
serviceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
FilterFunc: controller.FilterController(&v1.SinkBinding{}),
Handler: controller.HandleAll(impl.EnqueueControllerOf),
})

Comment on lines +141 to +146
Copy link
Member

Choose a reason for hiding this comment

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

@creydr it's a general common not specific to this PR, but we should evaluate if it makes sense to label all of our service accounts and use a "filtered informer" so that we're not watching all service accounts into a cluster which could be quite large but only "ours"

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 think it makes sense to label the service account 👍🏼

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point @pierDipi. I think we can do this in a separate PR

Copy link
Member

Choose a reason for hiding this comment

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

created #7341

// reconcile sinkindings on changes on the features configmap
configmapInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
FilterFunc: controller.FilterWithNameAndNamespace(system.Namespace(), feature.FlagsConfigName),
Handler: controller.HandleAll(func(i interface{}) {
impl.GlobalResync(sbInformer.Informer())
}),
})
Comment on lines +147 to +153
Copy link
Member

@pierDipi pierDipi Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of using a cluster-wide configmap informer, we can configure a callback function when calling NewStore, that saves quite a bit of memory since that watcher is watching only configmaps in the system namespace as opposed to watching every configmap in the cluster /cc @creydr

Copy link
Member

Choose a reason for hiding this comment

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

see example in pkg/reconciler/broker/controller.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like featureStore.WatchConfigs(cmw) , right ?

Copy link
Member

Choose a reason for hiding this comment

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

yes. featureStore.WatchConfigs(cmw) takes additional callback functions which get called on configmap updates. E.g.:

var globalResync func(obj interface{})
featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"), func(name string, value interface{}) {
	if globalResync != nil {
		globalResync(nil)
	}
})

with globalResync being something like:

globalResync = func(obj interface{}) {
	impl.GlobalResync(sbInformer.Informer())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @creydr


return impl
}

Expand Down Expand Up @@ -161,6 +199,24 @@ func (s *SinkBindingSubResourcesReconciler) Reconcile(ctx context.Context, b psb
Name: sb.Spec.Sink.Ref.Name,
}, b)
}

featureFlags := s.featureStore.Load()
if featureFlags.IsOIDCAuthentication() {
saName := auth.GetOIDCServiceAccountNameForResource(v1.SchemeGroupVersion.WithKind("SinkBinding"), sb.ObjectMeta)
sb.Status.Auth = &duckv1.AuthStatus{
ServiceAccountName: &saName,
}

if err := auth.EnsureOIDCServiceAccountExistsForResource(ctx, s.serviceAccountLister, s.kubeclient, v1.SchemeGroupVersion.WithKind("SinkBinding"), sb.ObjectMeta); err != nil {
sb.Status.MarkOIDCIdentityCreatedFailed("Unable to resolve service account for OIDC authentication", "%v", err)
return err
}
sb.Status.MarkOIDCIdentityCreatedSucceeded()
} else {
sb.Status.Auth = nil
sb.Status.MarkOIDCIdentityCreatedSucceededWithReason(fmt.Sprintf("%s feature disabled", feature.OIDCAuthentication), "")
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a constant string we can use across the board for this reason? /cc @creydr

Copy link
Member

Choose a reason for hiding this comment

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

Not yet. But makes sense to introduce one, as we have this at multiple places 👍

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 think that can be a part of seperate PR!

}

addr, err := s.res.AddressableFromDestinationV1(ctx, sb.Spec.Sink, sb)
if err != nil {
logging.FromContext(ctx).Errorf("Failed to get Addressable from Destination: %w", err)
Expand Down