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

✨ adding APIReader to the manager and injection. #327

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions pkg/manager/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ type controllerManager struct {
// client is the client injected into Controllers (and EventHandlers, Sources and Predicates).
client client.Client

// apiReader is the reader that will make requests to the api server and not the cache.
apiReader client.Reader
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for not making it client.Client ? [implementation wise client.Client is already supported]. Though it does get tricky with the naming if we decide to change the type here apiClient ? maybe directClient ?

Copy link
Author

Choose a reason for hiding this comment

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

My thought process was

  1. When you need this field you really only need the reader. This still points people to the client for most use cases. I was hoping the fact it did not have any write mechanisms people would only use this when they needed it.

  2. Naming is the other reason. I think the idea that we don't really expose "direct" vs "cache" and try to hide this decision as much as possible is a good thing. This is just a step in adding an escape hatch but not exposing that decision still.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

those reasons @shawn-hurley make a lot of sense -- +1: from me to having it be a Reader.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for delayed response. Haven't come across write use case, so going with the restricted use-case sounds good.


// fieldIndexes knows how to add field indexes over the Cache used by this controller,
// which can later be consumed via field selectors from the injected client.
fieldIndexes client.FieldIndexer
Expand Down Expand Up @@ -121,6 +124,9 @@ func (cm *controllerManager) SetFields(i interface{}) error {
if _, err := inject.ClientInto(cm.client, i); err != nil {
return err
}
if _, err := inject.APIReaderInto(cm.apiReader, i); err != nil {
return err
}
if _, err := inject.SchemeInto(cm.scheme, i); err != nil {
return err
}
Expand Down Expand Up @@ -167,6 +173,10 @@ func (cm *controllerManager) GetRESTMapper() meta.RESTMapper {
return cm.mapper
}

func (cm *controllerManager) GetAPIReader() client.Reader {
return cm.apiReader
}

func (cm *controllerManager) serveMetrics(stop <-chan struct{}) {
handler := promhttp.HandlerFor(metrics.Registry, promhttp.HandlerOpts{
ErrorHandling: promhttp.HTTPErrorOnError,
Expand Down
11 changes: 11 additions & 0 deletions pkg/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ type Manager interface {

// GetRESTMapper returns a RESTMapper
GetRESTMapper() meta.RESTMapper

// GetAPIReader returns a reader that will be configured to use the API server.
// This should be used sparingly and only when the client does not fit your
// use case.
GetAPIReader() client.Reader
}

// Options are the arguments for creating a new Manager
Expand Down Expand Up @@ -179,6 +184,11 @@ func New(config *rest.Config, options Options) (Manager, error) {
return nil, err
}

apiReader, err := client.New(config, client.Options{Scheme: options.Scheme, Mapper: mapper})
if err != nil {
return nil, err
}

writeObj, err := options.NewClient(cache, config, client.Options{Scheme: options.Scheme, Mapper: mapper})
if err != nil {
return nil, err
Expand Down Expand Up @@ -217,6 +227,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
cache: cache,
fieldIndexes: cache,
client: writeObj,
apiReader: apiReader,
recorderProvider: recorderProvider,
resourceLock: resourceLock,
mapper: mapper,
Expand Down
5 changes: 5 additions & 0 deletions pkg/manager/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,11 @@ var _ = Describe("manger.Manager", func() {
Expect(err).NotTo(HaveOccurred())
Expect(m.GetEventRecorderFor("test")).NotTo(BeNil())
})
It("should provide a function to get the APIReader", func() {
m, err := New(cfg, Options{})
Expect(err).NotTo(HaveOccurred())
Expect(m.GetAPIReader()).NotTo(BeNil())
})
})

var _ reconcile.Reconciler = &failRec{}
Expand Down
14 changes: 14 additions & 0 deletions pkg/runtime/inject/inject.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,20 @@ func CacheInto(c cache.Cache, i interface{}) (bool, error) {
return false, nil
}

// APIReader is used by the Manager to inject the APIReader into necessary types.
type APIReader interface {
InjectAPIReader(client.Reader) error
}

// APIReaderInto will set APIReader on i and return the result if it implements APIReaderInto.
// Returns false if i does not implement APIReader
func APIReaderInto(reader client.Reader, i interface{}) (bool, error) {
if s, ok := i.(APIReader); ok {
return true, s.InjectAPIReader(reader)
}
return false, nil
}

// Config is used by the ControllerManager to inject Config into Sources, EventHandlers, Predicates, and
// Reconciles
type Config interface {
Expand Down
63 changes: 51 additions & 12 deletions pkg/runtime/inject/inject_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,27 @@ var _ = Describe("runtime inject", func() {
Expect(res).To(Equal(true))
})

It("should set api reader", func() {
apiReader := &client.DelegatingReader{}

By("Validating injecting client")
res, err := APIReaderInto(apiReader, instance)
Expect(err).NotTo(HaveOccurred())
Expect(res).To(Equal(true))
Expect(apiReader).To(Equal(instance.GetAPIReader()))

By("Returning false if the type does not implement inject.Client")
res, err = APIReaderInto(apiReader, uninjectable)
Expect(err).NotTo(HaveOccurred())
Expect(res).To(Equal(false))
Expect(uninjectable.GetAPIReader()).To(BeNil())

By("Returning an error if client injection fails")
res, err = APIReaderInto(nil, instance)
Expect(err).To(Equal(errInjectFail))
Expect(res).To(Equal(true))
})

It("should set dependencies", func() {

f := func(interface{}) error { return nil }
Expand All @@ -175,12 +196,13 @@ var _ = Describe("runtime inject", func() {
})

type testSource struct {
scheme *runtime.Scheme
cache cache.Cache
config *rest.Config
client client.Client
f Func
stop <-chan struct{}
scheme *runtime.Scheme
cache cache.Cache
config *rest.Config
client client.Client
apiReader client.Reader
f Func
stop <-chan struct{}
}

func (s *testSource) InjectCache(c cache.Cache) error {
Expand Down Expand Up @@ -223,6 +245,14 @@ func (s *testSource) InjectStopChannel(stop <-chan struct{}) error {
return fmt.Errorf("injection fails")
}

func (s *testSource) InjectAPIReader(reader client.Reader) error {
if reader != nil {
s.apiReader = reader
return nil
}
return fmt.Errorf("injection fails")
}

func (s *testSource) InjectFunc(f Func) error {
if f != nil {
s.f = f
Expand All @@ -247,6 +277,10 @@ func (s *testSource) GetClient() client.Client {
return s.client
}

func (s *testSource) GetAPIReader() client.Reader {
return s.apiReader
}

func (s *testSource) GetFunc() Func {
return s.f
}
Expand All @@ -256,12 +290,13 @@ func (s *testSource) GetStop() <-chan struct{} {
}

type failSource struct {
scheme *runtime.Scheme
cache cache.Cache
config *rest.Config
client client.Client
f Func
stop <-chan struct{}
scheme *runtime.Scheme
cache cache.Cache
config *rest.Config
client client.Client
apiReader client.Reader
f Func
stop <-chan struct{}
}

func (s *failSource) GetCache() cache.Cache {
Expand All @@ -280,6 +315,10 @@ func (s *failSource) GetClient() client.Client {
return s.client
}

func (s *failSource) GetAPIReader() client.Reader {
return s.apiReader
}

func (s *failSource) GetFunc() Func {
return s.f
}
Expand Down