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

[Umbrella] Make client-go lighter and easier to consume #127888

Open
4 tasks
aojea opened this issue Oct 6, 2024 · 18 comments
Open
4 tasks

[Umbrella] Make client-go lighter and easier to consume #127888

aojea opened this issue Oct 6, 2024 · 18 comments
Labels
area/code-organization Issues or PRs related to kubernetes code organization sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@aojea
Copy link
Member

aojea commented Oct 6, 2024

Description

The current structure of client-go, with its dependencies on k8s.io/api and k8s.io/apimachinery present some significant challenges:

  • Dependency bloat: projects end up pulling in a large number of transitive dependencies, increasing binary size and potentially leading to conflicts.
  • Versioning headaches: Updating client-go often necessitates updating the dependent projects as well, which can be complex and time-consuming, especially with the need for coordinated releases.
  • Tight coupling: This structure creates tight coupling between client-go and the underlying API definitions, making necessary to update the clients to be able to use the new API types.

How to repro

Using the workqueue example https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/client-go/examples/workqueue and creating a new project, after doing go mod tidy the go.mod file looks like:

go.mod
module client-go-example

go 1.23

require (
        k8s.io/api v0.31.1
        k8s.io/apimachinery v0.31.1
        k8s.io/client-go v0.31.1
        k8s.io/klog/v2 v2.130.1
)

require (
        github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
        github.com/emicklei/go-restful/v3 v3.11.0 // indirect
        github.com/fxamacker/cbor/v2 v2.7.0 // indirect
        github.com/go-logr/logr v1.4.2 // indirect
        github.com/go-openapi/jsonpointer v0.19.6 // indirect
        github.com/go-openapi/jsonreference v0.20.2 // indirect
        github.com/go-openapi/swag v0.22.4 // indirect
        github.com/gogo/protobuf v1.3.2 // indirect
        github.com/golang/protobuf v1.5.4 // indirect
        github.com/google/gnostic-models v0.6.8 // indirect
        github.com/google/go-cmp v0.6.0 // indirect
        github.com/google/gofuzz v1.2.0 // indirect
        github.com/google/uuid v1.6.0 // indirect
        github.com/imdario/mergo v0.3.6 // indirect
        github.com/josharian/intern v1.0.0 // indirect
        github.com/json-iterator/go v1.1.12 // indirect
        github.com/mailru/easyjson v0.7.7 // indirect
        github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
        github.com/modern-go/reflect2 v1.0.2 // indirect
        github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
        github.com/spf13/pflag v1.0.5 // indirect
        github.com/x448/float16 v0.8.4 // indirect
        golang.org/x/net v0.26.0 // indirect
        golang.org/x/oauth2 v0.21.0 // indirect
        golang.org/x/sys v0.21.0 // indirect
        golang.org/x/term v0.21.0 // indirect
        golang.org/x/text v0.16.0 // indirect
        golang.org/x/time v0.3.0 // indirect
        google.golang.org/protobuf v1.34.2 // indirect
        gopkg.in/inf.v0 v0.9.1 // indirect
        gopkg.in/yaml.v2 v2.4.0 // indirect
        gopkg.in/yaml.v3 v3.0.1 // indirect
        k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect
        k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 // indirect
        sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
        sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
        sigs.k8s.io/yaml v1.4.0 // indirect
)

and its size is

ls -lah main
-rwxr-x--- 1 aojea primarygroup 51M Oct  6 08:41 main

Proposal

  • Modularization: Breaking down client-go into smaller, more focused modules would allow users to import only the functionalities they need, reducing dependency bloat and improving maintainability.
  • Generic clients and Versioned APIs: Providing generic clients that can use versioned API clients would enable users to target specific Kubernetes versions without being forced to update the entire client-go library.
  • Reducing dependencies: eliminate unnecessary dependencies within client-go and its related projects.

Design Details

k8s.io/api

Ideally this should be a self contained module without dependencies, but it depends on apimachinery for the generated code and registering the types

module k8s.io/api
go 1.23.0
godebug default=go1.23
require (
github.com/gogo/protobuf v1.3.2
github.com/stretchr/testify v1.9.0
k8s.io/apimachinery v0.0.0
)

It requires k8s.io/apimachinery because of:

  • schema
register.go:     "k8s.io/apimachinery/pkg/runtime"
register.go:     "k8s.io/apimachinery/pkg/runtime/schema"
zz_generated.deepcopy.go:        runtime "k8s.io/apimachinery/pkg/runtime"
  • metav1
register.go:     metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
zz_generated.deepcopy.go:        v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  • utils for types
"k8s.io/apimachinery/pkg/util/validation"
zz_generated.deepcopy.go:  intstr "k8s.io/apimachinery/pkg/util/intstr"
resource/v1alpha3/types.go:     "k8s.io/apimachinery/pkg/util/validation"

NOTE This validation can be easily avoided and it seems is only to match a constant (cc: @pohly )

const PoolNameMaxLength = validation.DNS1123SubdomainMaxLength // Same as for a single node name.

k8s.io/apimachinery

This is the more tricky, and it is bringing a lot of dependencies

module k8s.io/apimachinery
go 1.23.0
godebug default=go1.23
require (
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc
github.com/fxamacker/cbor/v2 v2.7.0
github.com/gogo/protobuf v1.3.2
github.com/golang/protobuf v1.5.4
github.com/google/gnostic-models v0.6.8
github.com/google/go-cmp v0.6.0
github.com/google/gofuzz v1.2.0
github.com/google/uuid v1.6.0
github.com/moby/spdystream v0.4.0
github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f
github.com/onsi/ginkgo/v2 v2.19.0
github.com/spf13/pflag v1.0.5
github.com/stretchr/testify v1.9.0
golang.org/x/net v0.28.0
golang.org/x/time v0.3.0
gopkg.in/evanphx/json-patch.v4 v4.12.0
gopkg.in/inf.v0 v0.9.1
k8s.io/klog/v2 v2.130.1
k8s.io/kube-openapi v0.0.0-20240827152857-f7e401e7b4c2
k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd
sigs.k8s.io/structured-merge-diff/v4 v4.4.1
sigs.k8s.io/yaml v1.4.0

k8s.io/client-go

These module contains all the generated code, the typed clients and the dynamic client. Thanks to @skitt we already have Generics to share code in client-go

In addition to the dynamic client we could have a Generic client, that will be able to work with different versions of k8s.io/api, allowing users to work with skew between k8s.io/client-go and k8s.io/api, though this requires a very high bar on these modules to avoid breaking clients and provide stability across the ecosystem.

module k8s.io/client-go
go 1.23.0
godebug default=go1.23
require (
github.com/gogo/protobuf v1.3.2
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da
github.com/golang/protobuf v1.5.4
github.com/google/gnostic-models v0.6.8
github.com/google/go-cmp v0.6.0
github.com/google/gofuzz v1.2.0
github.com/google/uuid v1.6.0
github.com/gorilla/websocket v1.5.0
github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7
github.com/peterbourgon/diskv v2.0.1+incompatible
github.com/spf13/pflag v1.0.5
github.com/stretchr/testify v1.9.0
go.uber.org/goleak v1.3.0
golang.org/x/net v0.28.0
golang.org/x/oauth2 v0.21.0
golang.org/x/term v0.23.0
golang.org/x/time v0.3.0
google.golang.org/protobuf v1.34.2
gopkg.in/evanphx/json-patch.v4 v4.12.0
k8s.io/api v0.0.0
k8s.io/apimachinery v0.0.0
k8s.io/klog/v2 v2.130.1
k8s.io/kube-openapi v0.0.0-20240827152857-f7e401e7b4c2
k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd
sigs.k8s.io/structured-merge-diff/v4 v4.4.1
sigs.k8s.io/yaml v1.4.0
)

Ideally we should be able to split the API types from the tooling and allow consumers to use generics, something like

import (
  corev1 "k8s.io/api/core/v1"
	k8s.io/client-go/generic
)


func main(
	client := generic.NewClient[corev1.Pod]
	list, err := client.Namespace("foo").List()

)

and use generic controllers, we already have some intree

type Controller[T runtime.Object] interface {
// Meant to be run inside a goroutine
// Waits for and reacts to changes in whatever type the controller
// is concerned with.
//
// Returns an error always non-nil explaining why the worker stopped
Run(ctx context.Context) error
// Retrieves the informer used to back this controller
Informer() Informer[T]
// Returns true if the informer cache has synced, and all the objects from
// the initial list have been reconciled at least once.
HasSynced() bool
}
type NamespacedLister[T any] interface {
// List lists all ValidationRuleSets in the indexer for a given namespace.
// Objects returned here must be treated as read-only.
List(selector labels.Selector) (ret []T, err error)
// Get retrieves the ValidationRuleSet from the indexer for a given namespace and name.
// Objects returned here must be treated as read-only.
Get(name string) (T, error)
}
type Informer[T any] interface {
cache.SharedIndexInformer
Lister[T]
}
// Lister[T] helps list Ts.
// All objects returned here must be treated as read-only.
type Lister[T any] interface {

Tasks

References

Notes

I don't create a KEP because there are a considerable number of dimensions in this problem, so as first step I just want to gather feedback and take a more iterative approach, there are things like dependency pruning that can be worked out without a KEP, once we reach a point we need to implement some user facing change I will open the corresponding KEP

/area code-organization
/sig architecture
/sig api-machinery

@k8s-ci-robot k8s-ci-robot added area/code-organization Issues or PRs related to kubernetes code organization sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 6, 2024
@aojea
Copy link
Member Author

aojea commented Oct 6, 2024

Tagging some of the people I know will be interested/impacted
/cc @deads2k @liggitt @dims @fedebongio @jpbetz @BenTheElder @thockin @pohly @alvaroaleman @howardjohn

@pohly
Copy link
Contributor

pohly commented Oct 6, 2024

Here's one more thought: k8s.io/client-go contains lots of loosely connected packages. Would it make sense to break it apart into smaller modules?

If a simple Go client doesn't need k8s.io/client-go/tools/record, it shouldn't depend on github.com/golang/groupcache/lru.

I'm aware that this raises challenges for our publishing bot because we don't want to change the import paths. For example, if k8s.io/client-go/tools/record was its own module, where do we publish the code? Can k8s.io/client-go/tools/record redirect to github.com/kubernetes/client-go-tools-record or does github.com/kubernetes/client-go have to become a multi-module repository?

Can Go handle the transition of a package into its own module? For v0.31.0, k8s.io/client-go/tools/record has to be found as package in client-go. For a future release, it would have to be found as module.

@pohly
Copy link
Contributor

pohly commented Oct 6, 2024

Here's one more thought: k8s.io/client-go contains lots of loosely connected packages. Would it make sense to break it apart into smaller modules?

Let me take this one step further: if don't do this, how much can we really reduce the dependencies of client-go? It doesn't help to remove a dependency from e.g. k8s.io/api if k8s.io/client-go then still depends on that same dependency through something else. For example, how useful is #127889 for client-go? It will still depend on apimachinery.

@aojea
Copy link
Member Author

aojea commented Oct 6, 2024

It will still depends on apimachinery.

that is the thing, if it does not depend on apimachinery, or if apimachinery is lighter, or if we break apimachinery in smaller modules, .... but we should not use as excuse that there is already dependencies to add new, it is in the best for all of us maintainers to untangle things as much as possible

@pohly
Copy link
Contributor

pohly commented Oct 7, 2024

I agree that it makes sense to simplify.

But removing a dependency on "bar" from "foo" should not be based on the argument that it's being done to remove "bar" from the client-go dependency tree unless there really is a plan how that can be achieved.

@aojea
Copy link
Member Author

aojea commented Oct 7, 2024

why not, everything that removes dependencies that are not necessary helps

@pohly
Copy link
Contributor

pohly commented Oct 7, 2024

If some package becomes cleaner because it avoids some dependency, that's fine. But if it becomes more complex and client-go still depends on "bar", then it's a net loss.

@aojea
Copy link
Member Author

aojea commented Oct 7, 2024

If some package becomes cleaner because it avoids some dependency, that's fine. But if it becomes more complex and client-go still depends on "bar", then it's a net loss

absolutely, goal is simplifying across the stack , as soon as things require more design and consensus we need a KEP with details ... but per example, since we have now a fork of go-yaml kubernetes-sigs/yaml#76 we can switch all the dependencies to it to have just one path and also avoid possible code drifting ... things like this can help to reduce the problem scope

@skitt
Copy link
Member

skitt commented Oct 7, 2024

Related to simplifying across the stack, there are a number of in-progress transitions or duplications of code/effort in the tree. They don’t get much attention, and we tend to frown upon “blanket deprecation” PRs, but I do think it would be good to make progress on some of them, especially if they can help reduce dependency trees. For example, k8s.io/utils/pointer to k8s.io/utils/ptr (not significant here because it’s the same dependency); or k8s/io/apimachinery/pkg/util/sets and k8s.io/utils/set (client-go currently uses the former; the two aren’t quite equivalent, see kubernetes/utils#294 in particular).

@skitt
Copy link
Member

skitt commented Oct 7, 2024

Can Go handle the transition of a package into its own module? For v0.31.0, k8s.io/client-go/tools/record has to be found as package in client-go. For a future release, it would have to be found as module.

In my experience it works reasonably well. In theory, when a module is split out, the “main” module must declare a dependency on the new module, so that existing users aren’t broken on upgrades; but that defeats the purpose if the goal is to simplify dependency trees. go get usually figures out what’s going on as long as it doesn’t run into ambiguous imports.

@skitt
Copy link
Member

skitt commented Oct 7, 2024

#127889 makes me wonder if we could move parts of apimachinery that meet k8s.io/utils’ requirements to k8s.io/utils? apimachinery has a big set of dependencies, k8s.io/utils has a minimal set and is supposed to keep it that way. For example, all of apimachinery validation could be moved to k8s.io/utils if we move validation/fields, errors and sets too (for sets, replace it with k8s.io/utils sets).

Put another way, we’ve ended up with two repositories of dependency-less utilities in k8s.io, apimachinery and sets, sometimes with duplicates. I think it would help this overall endeavour if we decided where these should live.

@skitt
Copy link
Member

skitt commented Oct 7, 2024

Consolidating on sigs.k8s.io/yaml doesn’t help with the dependency tree, but it does improve consistency. See #127901

@howardjohn
Copy link
Contributor

From my POV there are two aspects to 'light' -- low dependencies (less chance of dep hell, less licenses, etc), and smaller size (impacting final binary size which impacts end users).

WRT to the smaller size, a very substantial portion of this is due to basically any import of client-go bringing in everything in istio.io/api; you cannot just import core/v1 for example. This requires pretty deep changes throughout a lot of things - the schema, setting up the base client stuff, etc. Somehow fixing this would cut binary sizes in half, even for packages that actually depend on a large number of APIs.

(Unfortunately, even in an optimal package layout, if you depend on the tiny ConfigMap you also depend on the huge Pod)

@howardjohn
Copy link
Contributor

From my POV there are two aspects to 'light' -- low dependencies (less chance of dep hell, less licenses, etc), and smaller size (impacting final binary size which impacts end users).

WRT to the smaller size, a very substantial portion of this is due to basically any import of client-go bringing in everything in istio.io/api; you cannot just import core/v1 for example. This requires pretty deep changes throughout a lot of things - the schema, setting up the base client stuff, etc. Somehow fixing this would cut binary sizes in half, even for packages that actually depend on a large number of APIs.

(Unfortunately, even in an optimal package layout, if you depend on the tiny ConfigMap you also depend on the huge Pod)

To make this more concrete, this example https://github.com/howardjohn/k8s-client-go/blob/e0023d6daa2795430e532f5813108f8087ec2014/generics/example/main.go#L19 (which compiles) is 9.8Mb stripped, 15M unstripped. With an import of k8s.io/client-go/kubernetes its instead 30M/44M.

FWIW in a not-direct comparison, using kube-rs (rust), it is more like 7.6M/11M.

@alvaroaleman
Copy link
Member

alvaroaleman commented Oct 7, 2024

To make this more concrete, this example https://github.com/howardjohn/k8s-client-go/blob/e0023d6daa2795430e532f5813108f8087ec2014/generics/example/main.go#L19 (which compiles) is 9.8Mb stripped, 15M unstripped. With an import of k8s.io/client-go/kubernetes its instead 30M/44M.

And the main reason for importing k8s.io/client-go/kubernetes even when using some sort of dynamic client is the scheme. The scheme in turn is needed to go from go type to GVK and vice versa. I don't know if there is much use-case for the latter outside of the kube-apiserver and the former would be much better solved by adding this info to the types rather than to a global (ideally together with restmapping, to avoid needing that lookup for dynamic clients).

@Jefftree
Copy link
Member

Jefftree commented Oct 8, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 8, 2024
@BenTheElder
Copy link
Member

From my POV there are two aspects to 'light' -- low dependencies (less chance of dep hell, less licenses, etc), and smaller size (impacting final binary size which impacts end users).

One other aspect we're looking for is ease of switching Kubernetes API versions specifically, ideally the transport layer etc is pretty stable and you should be able to quickly adopt and implement custom behaviors for the new types without needing to update any mess of dependencies, just a package with the types.

I've been thinking a bit about how we might approach this and avoid breaking anyone at the same time.

Here's one more thought: k8s.io/client-go contains lots of loosely connected packages. Would it make sense to break it apart into smaller modules?

I would go a step further actually: some of this should be in non-staged libraries that are actually versioned and developed with stable consumption in mind in their own repos, rather than being "staged" out as unstable v0.x

But I think the first step is making it easier to consume the API types, with which you could bring your own client or skew the client (and use dynamic or a new generic API).

If I read #127888 (comment) right, I think alvaro is roughly advocating for that part at least as well, and I have recently floated the concept to a few people (like antonio) who seem to agree.

We're in the middle of enhancements freeze right now and will probably all have more bandwidth for this soon, possibly even at KubeCon?

@ash2k
Copy link
Member

ash2k commented Nov 4, 2024

Related to this issue from the user's perspective: ideally CustomResourceDefinition type needs to be moved out from the apiextensions-apiserver repo/module: kubernetes/apiextensions-apiserver#57. It's even worse than importing the built-in types and client-go as it brings in a lot of server-side code into consumers dependency tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/code-organization Issues or PRs related to kubernetes code organization sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

9 participants