Make the v1beta1 api package easier to consume

[johannesfrey]

User Story

As a user I would like to easily consume the CAPI v1beta1 API package without introducing unwanted transitive dependencies.

Detailed Description

Currently if one imports the CAPI v1beta1 API package, which in turn imports sigs.k8s.io/controller-runtime, the user transitively imports sigs.k8s.io/controller-runtime/pkg/client/config, which sets the string var kubeconfig to the default command line FlagSet. See:

• https://github.com/kubernetes-sigs/controller-runtime/blob/15b6689c55745514d6bade4c55efb3333c0da4d9/alias.go#L23
• https://github.com/kubernetes-sigs/controller-runtime/blob/15b6689c55745514d6bade4c55efb3333c0da4d9/pkg/client/config/config.go#L38
• PR that addresses the issue directly in controller-runtime: 🌱 Register kubeconfig flag variable via RegisterFlags func controller-runtime#1999

In the past this lead to redefined flag panics in our internal code base. A slack discussion revealed two possible solutions in CAPI:

• Use manager and builder pkgs of controller-runtime explicitly:
  • Less intrusive but skips using alias.go and directly depends on the packages itself.
• Move webhooks to internal:
  • This fixes the issue implicitly by moving the code parts that import controller-runtime away from the API package to the webhooks package. More intrusive but might align with the idea to move webhook code to internal. Though, there is no consensus if this should be done for existing code in the first place.

Anything else you would like to add:

It's probably most desirable to fix this directly in controller-runtime instead of CAPI and potential providers. Nevertheless, for both mentioned "CAPI solutions" there are corresponding WIP PRs in place. If one is more desirable the other one could be closed.

/kind feature

_{Johannes Frey <johannes.frey@mercedes-benz.com>, Mercedes-Benz Tech Innovation GmbH (Provider Information)}

[sbueringer]

I would prefer the first option, i.e. changing the imports.

But I wonder if there is a way to resolve the TODO in CR itself so using the top-level package doesn't lead to this problem.

Essentially resolving

// TODO: Fix this to allow double vendoring this library but still register flags on behalf of users
https://github.com/kubernetes-sigs/controller-runtime/blob/15b6689c55745514d6bade4c55efb3333c0da4d9/pkg/client/config/config.go#L38

in CR instead of changing the imports in CAPI.

No strong objections against changing the imports in CAPI in the meantime, but it would be more sustainable if it could be fixed in CR instead (not sure who remembers in the future which imports we have to use, and it would be nice to use the aliases for their intended purposes).

@vincepri WDYT?

[johannesfrey]

I would prefer the first option, i.e. changing the imports.

But I wonder if there is a way to resolve the TODO in CR itself so using the top-level package doesn't lead to this problem.

Essentially resolving

// TODO: Fix this to allow double vendoring this library but still register flags on behalf of users
https://github.com/kubernetes-sigs/controller-runtime/blob/15b6689c55745514d6bade4c55efb3333c0da4d9/pkg/client/config/config.go#L38

in CR instead of changing the imports in CAPI.

No strong objections against changing the imports in CAPI in the meantime, but it would be more sustainable if it could be fixed in CR instead (not sure who remembers in the future which imports we have to use, and it would be nice to use the aliases for their intended purposes).

@vincepri WDYT?

Thanks for the quick reply.

FYI: There have been related issues regarding this in CR:

• flag redefined: kubeconfig: allow double vendoring this library but still register flags on behalf of users controller-runtime#878
• Import of controller-runtime calls flag for "kubeconfig" controller-runtime#1396 (Still open, with an idea to define a RegisterFlag func, which would be a breaking change as the user must call this instead of relying on an init during the import.)

[sbueringer]

Thx!
Yup looks like it needs an idea on how to resolve this and potentially a first implementation PR to make it happen / to push it forward.

[fabriziopandini]

/triage accepted

I'm curious to understand if changing imports is enough to improve the situation/simplify the import graph.
+1 to fix the flag issue in CR

[johannesfrey]

FYI: The CR PR has been merged, which should resolve the issue as soon as CAPI bumps to a version of CR that contains the fix. Some follow-up steps/questions:

• I would then definitely close [WIP] 🌱 Use manager and builder pkgs of controller-runtime explicitly. #7175?!
• [WIP] 🌱 Move webhooks to internal #7157 follows another approach by consolidating the placement of webhooks in an internal package. Though, as this is a big change, I would propose to close this as well. WDYT?
• Should I create a follow-up issue to bump controller-runtime to a newer version containing the fix (Not sure if this will be part of e.g. v0.14.0)?

[fabriziopandini]

Thanks for the update!
+1 to close the two PRs
We are usually bumping CR as soon as a new release is out, but feel free to open an issue to track this bump specifically

[johannesfrey]

Thanks for the update! +1 to close the two PRs We are usually bumping CR as soon as a new release is out, but feel free to open an issue to track this bump specifically

Sure 🙂. Ok, if the bump is done regularly I won't open an issue. Thx!

[sbueringer]

Sounds good to me. The bump of CR will be mandatory for the Kubernetes 1.26 dependency bump anyway.

Both should easily make it into v1.4.0 (given that Kubernetes 1.26 is planned for December and CAPI 1.4 for end of March)

[sbueringer]

@johannesfrey Thx for your work on this!!