RFC: Deprecation and removal of ComponentConfig

[christopherhein]

This issue tracks the deprecation and removal of ComponentConfig from Controller Runtime.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[vincepri]

/lifecycle frozen
/milestone v0.7.x

@christopherhein Let me know if this should fall into 0.7, or we can wait for later as well

[vincepri]

/priority important-longterm

[christopherhein]

What is the v0.7 timeframe?

I'm working on updating the PR this week and will likely have something Friday or early next week… at least for #891

[vincepri]

1-2 months, probably, it all depends when 1.19 is release and when the current planned breaking changes will merge

[christopherhein]

Perfect, let's aim for that milestone since as we've discussed this will also have breaking changes. Thanks!

[christopherhein]

@vincepri I think we need to remove this tracking issue from the v0.7 milestone. For now, we should just have #891 give this is tracking the entire feature through alpha -> beta -> ga we'll want the api to bake a bit.

[christopherhein]

@DirectXMan12 @vincepri looking into at the next phases of this…

I'd originally documented moving to use configv1alpha1.ClientConnectionConfiguration - https://github.com/kubernetes/component-base/blob/master/config/v1alpha1/types.go#L69 as a standard type that we could configure rest.Config with. Looking into how this is implemented in manager.New() it takes in the *rest.Config, Options{} as parameters meaning if we included the fields from configv1alpha1.ClientConnectionConfiguration and supported nil values for rest.Config without erroring (https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/manager/manager.go#L306-L308) we could configure the *rest.Config with the values from the file as long as those same fields were defined on the Options struct…

For reference:

type ClientConnectionConfiguration struct {
	// kubeconfig is the path to a KubeConfig file.
	Kubeconfig string `json:"kubeconfig"`
	// acceptContentTypes defines the Accept header sent by clients when connecting to a server, overriding the
	// default value of 'application/json'. This field will control all connections to the server used by a particular
	// client.
	AcceptContentTypes string `json:"acceptContentTypes"`
	// contentType is the content type used when sending data to the server from this client.
	ContentType string `json:"contentType"`
	// qps controls the number of queries per second allowed for this connection.
	QPS float32 `json:"qps"`
	// burst allows extra queries to accumulate when a client is exceeding its rate.
	Burst int32 `json:"burst"`
}

The ux would end up being something like:

mgr, err := ctrl.NewManager(nil, ctrl.Options{Scheme: scheme}.AndFromOrDie(cfg.File()))
// handle error

Expecting that Options an/or the ComponentConfig type set the same defaults we have defined in https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/config/config.go#L85-L88

What do you think? Do you think this is/would be useful?

[errordeveloper]

Just notice there hasn't been much activity and recent deprecations add a bit of confusion (#2165 (comment)).

@christopherhein @vincepri is this issue still relevant?

[vincepri]

@errordeveloper Probably not, x-posting from the deprecation PR #2165 (comment)

The codebase around configuration package is missing maintainers and the general integration between the configuration types and the manager options or controllers can be very confusing to use.

There is currently no alternative implementation, folks could use their own json/yaml unmarshalling and configure manager options that way.

Happy to revisit if we have folks stepping up to review, cleanup, support, and maintain the codebase.

[vincepri]

/retitle RFC: Deprecation and removal of ComponentConfig

[errordeveloper]

/retitle RFC: Deprecation and removal of ComponentConfig

I'm afraid, but I have to say that it is now exteremely confusing to anyone reading this issue.

[terinjokes]

This is a bit annoying, as we had just finished migrating to these types, as they were what all the examples were showing We were following the "custom" example to embed ComponentConfig into our configuration, and using the file loader to load both our application configuration and to configure controller-runtime in one go.

We run about a dozen different controllers and having a consistent way to configure controller-runtime, for internally-developed controllers, as well as integrating open source ones, is extremely attractive for our operations. Now I have egg on my face to all my colleagues (and open source projects) because this was in no way well communicated as being removed on such short notice.

[errordeveloper]

@terinjokes I have also ended up doing that too, I think it should be actually fairly easy to either just move this code as is into another repo or create a minimalist config loader. Let me know if you want to work together on some kind of solution to this deprecation problem.

I find the current API a bit of an overkill for such a small task.

loader := ctrl.ConfigFile().AtPath(configFile).OfKind(config)
options, err := options.AndFrom(loader)

It could be replaced with something simpler. But it's not like it bothers me really, it's just that while figuring out how to use it I had to read what all these fancy functions do, and I wished it was just one simple struct and a loader method instead.

[terinjokes]

I can agree the file loader was a bit strange at first, and would be okay with improvements there. The real win is retaining a consistent way to configure controller-runtime across projects.

[terinjokes]

@vincepri I still would like to get config loading back into controller runtime. How could I begin working on a proposal?

[vincepri]

There is a need to push the original proposal further, and have folks be listed as maintainers long term. What in general hasn't worked well is large features without a clear maintainer, and where the current set of maintainers don't have bandwidth to carry.

What also would be ideal is that we restructure the codebase to have the component config as an optional pattern that lives outside the manager altogether.

[terinjokes]

There is a need to push the original proposal further, and have folks be listed as maintainers long term.

Was there an original proposal document? Or do you mean the alpha that got implemented? I haven’t seen the former, if it exists.

What also would be ideal is that we restructure the codebase to have the component config as an optional pattern that lives outside the manager altogether.

I can see this working fairly well, as a lot of this configuration is also exposed as part of the manager’s options. Especially with the improvements we’ve seen with those options in the last two releases.

[criscola]

Q: The old ComponentConfig was convenient for operator developers because it allowed them or their users to modify all standard operator configuration without having to deal with all the parsing and boilerplate themselves. Now, is there anything that replaces it, or should users simply embed manager.Config (or parts of it) into their own struct and deal with all the parsing themselves? Thanks!

[troy0820]

@sbueringer @vincepri Because PR #2648 was merged do we want to leave this issue open? 👀

[Vaniog]

Q: The old ComponentConfig was convenient for operator developers because it allowed them or their users to modify all standard operator configuration without having to deal with all the parsing and boilerplate themselves. Now, is there anything that replaces it, or should users simply embed manager.Config (or parts of it) into their own struct and deal with all the parsing themselves? Thanks!

@criscola Did you find answer for your question? I'm dealing with the same problem right now.

[sbueringer]

@sbueringer @vincepri Because PR #2648 was merged do we want to leave this issue open? 👀

Makes sense to close, given that we removed ComponentConfig.

/close

For folks looking for an alternative. I would recommend defining your own configuration struct with JSON/YAML tags and then use one of the usual libraries to parse it (shouldn't be more complicated than defining a CRD). I would not recommend embedding any of the CR structs as they have no JSON/YAML marshalling tags and some of them are having function fields etc.

[k8s-ci-robot]

@sbueringer: Closing this issue.

In response to this:

@sbueringer @vincepri Because PR #2648 was merged do we want to leave this issue open? 👀

Makes sense to close, given that we removed ComponentConfig.

/close

For folks looking for an alternative. I would recommend defining your own configuration struct with JSON/YAML tags and then use one of the usual libraries to parse it. I would not recommend embedding any of the CR structs as they have no JSON/YAML marshalling tags and some of them are having function fields etc.

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-sigs/prow repository.