⚠️ ComponentConfig Implementation

[christopherhein]

This implements #818 but does not follow the design to the tee. Differences are called out below and the design doc will be updated once more eyes have been on the implementation.

Example Implementation

Example main.go

mgr, err := manager.New(config.GetConfigOrDie(), manager.Options{}.AndFromOrDie(cfg.FromFile()))

// set flag overrides
var namespace string
flag.StringVar(&namespace, "namespace", "default", "help message for flagname")

mgr, err := manager.New(config.GetConfigOrDie(), manager.Options{
  Namespace: namespace,
}.AndFromOrDie(cfg.FromFile()))

[christopherhein]

@mtaufen @stealthybox @shawn-hurley @djzager @joelanford I would love a review of this implementation, as stated in the PR desc if this seems like a reasonable approach before this is merged I will update the design document again.

[djzager]

Thank you for this. It matches what I expected it to look like and the more I see UpdateOptionsFromComponentConfig the more I prefer it over NewFromComponentConfig given the flexibility it provides (and how easy it is to add New if we decide later that is what we want).

[christopherhein]

Thank you for this. It matches what I expected it to look like and the more I see UpdateOptionsFromComponentConfig the more I prefer it over NewFromComponentConfig given the flexibility it provides (and how easy it is to add New if we decide later that is what we want).

@djzager Thanks for that. It's interesting after implementing UpdateXXX first I thought this experience take so much more upfront setup this really strips away the setup of the Options{} struct.

With update it looks more like this:

cfg := &ctrlv1alpha1.DefaultControllerConfiguration{}
options := &ctrl.Options{}
err := ctrl.UpdateOptionsFromComponentConfig(scheme, configfile, )
// err handling

// set flag overrides

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), *options)
// err handling

and the weird UX I didn't like is if you set values on Options{} in the first initialization (ie &ctrl.Options{SyncPeriod: xxx} the UpdateXXX func can override those values. This concerns me since folks might not realize that and have their flag settings first before update.

[christopherhein]

@joelanford @djzager I've updated this PR to use GenericControllerConfiguration as the ComponentConfig type.

I've also adjusted the structure of GenericControllerConfiguration and now embedd the ControllerConfiguration (this was called GenericControllerConfigurationSpec) instead of nesting under Spec. This allows us to define the manager.Configuration interface functions on the ControllerConfiguration type and still have them on the runtime.Object supported type and allows controller developers to do the same and not have to reimplement those funcs.

I'm open to discussion about the naming of ControllerConfiguration but I think this makes extending this config fairly natural feeling, eg:

type MyProjectConfiguration struct {
	metav1.TypeMeta `json:",inline"`

	configv1alpha1.ControllerConfiguration `json:",inline"`

        // Custom fields
}

WDYT?

[alexeldeib]

I explored this a bit over the weekend and familiarized myself with the kubelet config code as a reference. Generally LGTM. The interface is a little large, albeit simple, it'd be nice to see if we can do anything clever to reduce the size or default somehow (maybe something inject-y? idk). +1 for NewFromComponentConfig and the intuition around updating/initializing options.

[DirectXMan12]

Musings...

... On the Subject of Options from ComponentConfig

I wonder if we can actually invert NewFromComponentConfig into Options#AndFromConfig (name TDB, bear with me on that one ;-)), so we end up with

ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{Scheme: scheme}.AndFrom(cfg))

cfg could be either

cfg, err := config.FromFile("cfg.yaml")

or, for maximum inline (and probably the normal pattern):

ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
    Scheme: scheme,
}.AndFrom(config.FromFileOrDie("path")))

This means that we keep structured argument (i.e. no "scheme" positional argument floating around in the super-long-named NewOptionsFromComponentConfig), we don't need the extra line, and it flows nicely.

... On the Subject of GetConfigOrDie

We probably want to be able to RESTConfig from componentconfig no? Or is that always separate? Can definitely figure that out in a follow-up, but I'm curious what other things do.

P.S. sorry for not suggesting these during design review :-/

[DirectXMan12]

Oooh, also:

can we get an example in the examples folder and in the example_test.go file?

Would make the overall UX easier to see from the user side of things

[christopherhein]

I explored this a bit over the weekend and familiarized myself with the kubelet config code as a reference. Generally LGTM. The interface is a little large, albeit simple, it'd be nice to see if we can do anything clever to reduce the size or default somehow (maybe something inject-y? idk). +1 for NewFromComponentConfig and the intuition around updating/initializing options.

I had a good chat with @mtaufen about the multiple approaches and if there is a standard, I committed to writing a doc that explains the three different approaches we're viewing for getting this information out of the config file. Including using apimachinery conversions, using the kubebuilder webhook style conversions - POC - christopherhein@878f3cb and last using this interface style and discussing pros and cons of each approach.

[christopherhein]

Oooh, also:

can we get an example in the examples folder and in the example_test.go file?

Would make the overall UX easier to see from the user side of things

I can definitely do that, a quick example if you want to see sooner than that I've implemented this in one of my test controllers - https://github.com/christopherhein/github-controller/pull/6/files I will write up an example in that file too.

[christopherhein]

@DirectXMan12 some replies inline. I like the angle you are coming from. That leads to a really nice UX. One thing I've been considering is what the long-term plan is with this, I believe this is only the first step, with my hope that we can in a major release that we can replace Options with the component config type, I've started to document a couple steps to get there - #895

ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{Scheme: scheme}.AndFrom(cfg))

This feels simple, I could see how this would support flag overrides. Worth exploring.

cfg, err := config.FromFile("cfg.yaml")

I really like this instead of NewFromComponentConfig() and since CR doesn't have a config package this could be a useful want to package it. 👍

ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
    Scheme: scheme,
}.AndFrom(config.FromFileOrDie("path")))

This means that we keep structured argument (i.e. no "scheme" positional argument floating around in the super-long-named NewOptionsFromComponentConfig), we don't need the extra line, and it flows nicely.

Agreed. This is a really clean way of handling it.

... On the Subject of GetConfigOrDie

We probably want to be able to RESTConfig from componentconfig no? Or is that always separate? Can definitely figure that out in a follow-up, but I'm curious what other things do.

Yea, there is a way we can do this, I hadn't implemented it in this PR, trying to keep it simple and add this on later once we had more of the structure in-place. It generally should be pretty easy, we can use https://github.com/kubernetes/component-base/blob/master/config/v1alpha1/types.go#L69-L82 to setup some of the structure and then we'll just need to create the client in the NewFrom*

P.S. sorry for not suggesting these during design review :-/

No worries, I know you are incredibly busy. 🙏 thank you for giving it a thorough review.

[christopherhein]

/hold

[DirectXMan12]

I can definitely do that, a quick example if you want to see sooner than that I've implemented this in one of my test controllers

Cool. Was mainly looking at seeing the process of integrating custom configuration fields (didn't see that on the linked PR, but I might've missed something)

Yea, there is a way we can do this, I hadn't implemented it in this PR, trying to keep it simple and add this on later once we had more of the structure in-place. It generally should be pretty easy, we can use https://github.com/kubernetes/component-base/blob/master/config/v1alpha1/types.go#L69-L82 to setup some of the structure and then we'll just need to create the client in the NewFrom*

Cool, can definitely do that in a follow-up, just want to make sure we plan how that interacts with things API ergonomics-wise -- do we have folks pass nil to the config argument of NewManager and have it pull it from options? Do we make options override whatever is passed there? Do we add a method? Just things to think about for the future.

Thanks for the awesome work. Looking forward to this :-)

[christopherhein]

/test pull-controller-runtime-test-master

[DirectXMan12]

minor questions and one oversight it looks like. looks pretty good though.

[vincepri]

/approve

[vincepri]

Last changes LGTM, leaving the last word to @DirectXMan12 @alvaroaleman

[alvaroaleman]

One comment about naming, other than that lgtm

[alvaroaleman]

I think a better name would be Controller_Manager_Configuration, none of the settings are controller-level settings. Same comment applies for the types

[christopherhein]

Snake case like that?

[alvaroaleman]

No sorry, the underscores where just to highlight the difference

[christopherhein]

Great, I'll get this updated. Thanks for the feedback :)

[christopherhein]

All updated. Thanks again, @alvaroaleman

[alvaroaleman]

/lgtm
Thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, christopherhein, vincepri

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman,vincepri]

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