📖Adding ComponentConfig proposal

[christopherhein]

This is a proposal for how we can integrate ComponentConfig into controller-runtime without breaking existing implementations and allows controller authors an easier way to configure their controllers.

This was originally documented in #518

This uses the new design proposals from designs/template.md for a working example see christopherhein/github-controller#5

Signed-off-by: Chris Hein [email protected]

[christopherhein]

/assign @vincepri

[stealthybox]

Small grammar edits + a question

[shawn-hurley]

I really like that we standardizing on a controller configuration format thanks for putting this together, sorry the review took so long.

Can we make it easier to use a default component config by providing the struct/methods and unmarshaling?

[christopherhein]

@stealthybox @shawn-hurley updated with new DefaultControllerConfig struct, and using unmarshalling built into the NewForComponentConfig() func.

[christopherhein]

@stealthybox @shawn-hurley updated again to include the embeddable type of metav1alpha1.RuntimeConfig and adding how that supports DefaultControllerConfiguration.

Thanks again for taking a look at this.

[shawn-hurley]

Just to make sure that I am following,

1. the first pass is to create the meta/config types and the default type and new NewManager method such that a user could use all the default stuff.

2. A new design on how a user could extend the default stuff including adding scaffolding for that?

[christopherhein]

2. A new design on how a user could extend the default stuff including adding scaffolding for that?

Correct and #2 will be another PR into Kubebuilder as I'd assume the scaffolding will live there. I'm holding off submitting that until we have this one ironed out.

[christopherhein]

@mtaufen @lavalamp @shawn-hurley @stealthybox thanks for all the feedback, this has been updated with the most recent changes:

• Renamed embedded RuntimeConfig to ControllerConfiguration
• Moved the embedded types into pkg/api/config/v1alpha1/types.go so they'll now will live with the DefaultControllerConfiguration
• Cleaned up some wording.

Appreciate the time reviewing this 😃

[djzager]

Nice work.

[christopherhein]

@shawn-hurley @mtaufen @stealthybox @vincepri any more feedback, I'd love to start moving this forward and get a implementation in place.

[mtaufen]

Forge ahead! :)

Do take a spin through the Versioned Component Configuration Files doc (2018) while you're implementing and see how this compares (kubernetes-dev membership grants access).

/lgtm

[mtaufen]

Just FYI for backwards compatibility flags usually need to take precedence over config (unless you can force users to use exclusively flags or config).

[christopherhein]

Good callout, since we're decoding in the same object that the flags could have already been set on there isn't a way I'm aware of to not override existing values while decoding. Have any suggestions?

[shawn-hurley]

save off the flags and apply them after the decode? (merge the options structs together more or less)?

[christopherhein]

Yea, that probably makes the most sense. I personally don't think this needs to be documented in the design doc, do you?

Feeling okay with the rest of this @shawn-hurley ?

[joelanford]

Instead of ctrl.NewFromComponentConfig() that returns Manager, would it be reasonable to have ctrl.NewOptionsFromComponentConfig() that returns manager.Options. And then let callers call manager.New() themselves so that they have more flexibility in updating manager.Options after it has been populated?

[christopherhein]

Totally a possibility. I'd originally started with that path and ended up combining the funcs to make the new API quicker to implement.

@shawn-hurley @mtaufen @DirectXMan12 @stealthybox Do you have a preference? The options would be:

1. Use a single function ctrl.NewFromComponentConfig() which takes the ComponentConfig type and returns a ctrl.Manager.

or

2. Separate the funcs and make it ctrl.NewOptionsFromComponentConfig() which returns manager.Options that implementation wise would be passed into manager.New().

[joelanford]

Another thought would be for the ComponentConfig-populating function to accept a pointer to a manager.Options struct so that something like this would be possible:

func main() {
   opts := manager.Options{}
   _ = flag.StringVar(&opts.Namespace, "namespace", "", "Namespace to watch.")

   ctrl.UpdateOptionsFromComponentConfig(&opts)

   // Parse flags after populating from ComponentConfig so that flags
   // take precedence
   _ = flag.Parse()
   _ = manager.New(ctrl.GetConfigOrDie(), opts)
}

[christopherhein]

We'd need to pass similar params into each func, EG parsing out the ComponentConfig type into the options we need to have the scheme, config filename, the component config type and the Options struct.

func main() {
   opts := manager.Options{}
   _ = flag.StringVar(&opts.Namespace, "namespace", "", "Namespace to watch.")
   _ = flag.StringVar(&filename, "config", "f", "ConmponentConfig type configuration.")

   _ = ctrl.UpdateOptionsFromComponentConfig(scheme, filename, &componentconfig, &opts)

   // Parse flags after populating from ComponentConfig so that flags
   // take precedence
   _ = flag.Parse()
   _ = manager.New(ctrl.GetConfigOrDie(), opts)
}

I think both are fair flows and we could achieve the same goals with a composite function that returns the manager or returns the options.

Does anyone have strong opinions against this? @stealthybox @johnsonj @joelanford @djzager @shawn-hurley @mtaufen

[joelanford]

/lgtm

[shawn-hurley]

/approve

If any other followups we can still discuss here, but let's get an implementation started and if more things come up we can discuss.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christopherhein, shawn-hurley

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 [shawn-hurley]

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