cleanup: Inject a single k8s.Client instead of reading the kubeconfig ad hoc - #4026
cleanup: Inject a single k8s.Client instead of reading the kubeconfig ad hoc#4026gauron99 wants to merge 3 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauron99 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
10766d1 to
76e9b1d
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors cluster access across the codebase to use a single injected *k8s.Client instance, eliminating ad-hoc kubeconfig reads and package-level OpenShift detection/global helpers. It introduces a unified client abstraction that can be constructed from either kubeconfig resolution (CLI-style) or a raw rest.Config (library-style), and wires that client through deployers/describers/removers/pipelines/operator code and related tests.
Changes:
- Introduce
pkg/k8s.Clientwith kubeconfig-backed and rest-config-backed constructors, plus per-client OpenShift detection. - Inject
*k8s.Clientthrough cluster-facing helpers and components (Knative/K8s/KEDA/Tekton/PAC/operator/http transport), removing former global helpers/caches. - Update unit/integration/e2e tests to build and pass the new client and to validate OpenShift detection via a fake API server.
Reviewed changes
Copilot reviewed 85 out of 85 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/testing/testing.go | Adds FakeCluster helper producing a kubeconfig for an httptest-backed API server (OpenShift probe). |
| pkg/testing/k8s/testing.go | Switches integration namespace helper to use k8s.Clientset() from the new client. |
| pkg/pipelines/tekton/templates.go | Injects *k8s.Client into Tekton template creation/apply helpers and manifestival client creation. |
| pkg/pipelines/tekton/templates_test.go | Updates unit tests for new *k8s.Client parameters and manifestival client signature. |
| pkg/pipelines/tekton/templates_int_test.go | Updates integration tests to pass a kubeconfig-backed *k8s.Client. |
| pkg/pipelines/tekton/resources.go | Injects *k8s.Client into Tekton resource deletion helpers and client creation. |
| pkg/pipelines/tekton/resources_pac.go | Injects *k8s.Client into PAC secret/repository helpers and client/namespace resolution. |
| pkg/pipelines/tekton/pipelines_provider.go | Adds provider-held *k8s.Client and threads it through remote-build operations and describers/removers. |
| pkg/pipelines/tekton/pipelines_provider_test.go | Updates mocks and call sites for client-injected PVC creation helpers. |
| pkg/pipelines/tekton/pipelines_pac_provider.go | Threads provider client through PAC detection/info/route and secret access. |
| pkg/pipelines/tekton/pipelines_pac_provider_test.go | Updates PAC provider tests to construct provider with a *k8s.Client. |
| pkg/pipelines/tekton/pipelines_int_test.go | Updates remote pipeline integration tests to build and pass a single *k8s.Client. |
| pkg/pipelines/tekton/pac/pac.go | Makes PAC detection/info/route functions client-injected (typed/dynamic clients from kc). |
| pkg/pipelines/tekton/pac/client.go | Makes Tekton PAC client creation use injected client for namespace + rest config. |
| pkg/pipelines/tekton/gitlab_int_test.go | Updates GitLab integration tests to use new client-injected constructors. |
| pkg/pipelines/tekton/client.go | Makes Tekton client(s) creation use injected *k8s.Client for rest config. |
| pkg/operator/syncer.go | Makes operator Syncer hold and use an injected *k8s.Client. |
| pkg/operator/sync.go | Updates SyncFunctionCR flow to accept *k8s.Client and thread it to secret helpers. |
| pkg/operator/sync_test.go | Updates operator sync tests/mocks for client-injected paths. |
| pkg/knative/remover.go | Makes Knative remover client-injected and adds nil-guard before cluster use. |
| pkg/knative/remover_int_test.go | Updates Knative remover integration tests to pass a *k8s.Client. |
| pkg/knative/logs.go | Makes Knative logs gathering accept injected *k8s.Client for pod log selector. |
| pkg/knative/lister.go | Updates lister to use kc.RestConfig() (new API) for client creation. |
| pkg/knative/lister_int_test.go | Updates Knative lister integration tests for new constructors. |
| pkg/knative/describer.go | Makes describer client-injected, adds nil-guard, and uses kc.Clientset(). |
| pkg/knative/describer_int_test.go | Updates Knative describer integration tests for new constructors. |
| pkg/knative/deployer.go | Makes deployer client-injected, removes ad-hoc client construction, and threads client into http transport/logs/resource checks. |
| pkg/knative/deployer_int_test.go | Updates Knative deployer integration tests to construct/pass *k8s.Client. |
| pkg/knative/client.go | Makes serving/eventing client constructors accept injected *k8s.Client and adds nil-guards. |
| pkg/keda/remover.go | Makes KEDA remover client-injected and adds nil-guard before cluster use. |
| pkg/keda/remover_int_test.go | Updates KEDA remover integration tests for new constructors. |
| pkg/keda/lister.go | Updates lister to use kc.RestConfig() (new API). |
| pkg/keda/lister_int_test.go | Updates KEDA lister integration tests for new constructors. |
| pkg/keda/describer.go | Makes KEDA describer client-injected, adds nil-guard, and injects client into HTTPScaledObject clientset creation. |
| pkg/keda/describer_int_test.go | Updates KEDA describer integration tests for new constructors. |
| pkg/keda/deployer.go | Makes KEDA deployer client-injected, threads client into underlying k8s deployer and extra KEDA resources. |
| pkg/keda/deployer_int_test.go | Updates KEDA deployer integration tests for new constructors. |
| pkg/keda/client.go | Makes HTTPScaledObject clientset creation accept injected client and adds nil-guard. |
| pkg/k8s/serviceaccount.go | Updates serviceaccount helper to use injected *k8s.Client for clientset creation. |
| pkg/k8s/security_context.go | Makes default pod security context depend on explicit OpenShift flag instead of global detection. |
| pkg/k8s/security_context_test.go | Updates security context tests to pass OpenShift flag directly (removes global override). |
| pkg/k8s/secrets.go | Injects *k8s.Client across secret helpers (get/list/delete/ensure). |
| pkg/k8s/secrets_test.go | Updates secret tests to call list helpers with an injected client. |
| pkg/k8s/remover.go | Makes k8s remover client-injected and adds nil-guard before cluster use. |
| pkg/k8s/remover_int_test.go | Updates k8s remover integration tests for new constructors. |
| pkg/k8s/persistent_volumes.go | Injects client across PVC helpers and volume upload flow; threads OpenShift flag into security context. |
| pkg/k8s/persistent_volumes_int_test.go | Updates PVC integration tests to pass client through PVC/log helpers. |
| pkg/k8s/openshift.go | Moves OpenShift-related helpers onto Client (service CA, registry default, docker loaders) and removes global cache/override. |
| pkg/k8s/manifestival.go | Makes manifestival client creation accept injected *k8s.Client. |
| pkg/k8s/logs.go | Injects *k8s.Client into pod log helpers and factors out a core-interface podLogs helper. |
| pkg/k8s/logs_int_test.go | Updates logs integration tests for client-injected pod log helpers. |
| pkg/k8s/lister_int_test.go | Updates k8s lister integration tests for new constructors. |
| pkg/k8s/dialer.go | Makes in-cluster dialer use injected *k8s.Client (rest config + namespace + OpenShift flag). |
| pkg/k8s/dialer_int_test.go | Updates dialer integration tests for new client-injected APIs. |
| pkg/k8s/describer.go | Makes k8s describer client-injected and adds nil-guard. |
| pkg/k8s/describer_int_test.go | Updates k8s describer integration tests for new constructors. |
| pkg/k8s/deployer.go | Makes k8s deployer client-injected; threads client through resource validation; uses rest config only where needed. |
| pkg/k8s/deployer_int_test.go | Updates k8s deployer integration tests for new constructors. |
| pkg/k8s/configmaps.go | Injects *k8s.Client across configmap helpers (get/list). |
| pkg/k8s/configmaps_test.go | Updates configmap tests to call list helpers with an injected client. |
| pkg/k8s/client.go | Introduces new Client abstraction (kubeconfig/rest-config backed), clientset/dynamic/namespace helpers, per-client OpenShift detection. |
| pkg/k8s/client_test.go | Reworks tests (external package) and adds coverage for detection/namespace/registry/credential loaders using FakeCluster. |
| pkg/http/transport.go | Updates round tripper to accept injected *k8s.Client (for lazy in-cluster dialer default). |
| pkg/http/transport_test.go | Updates transport tests for new signature (passing nil kc with explicit dialer). |
| pkg/http/openshift.go | Updates OpenShift CA option to use injected client method. |
| pkg/http/openshift_int_test.go | Updates OpenShift transport integration test to use per-client detection and injected CA option. |
| pkg/functions/client_int_test.go | Updates function client integration tests for new client-injected constructors. |
| pkg/deployer/testing/integration_test_helper.go | Updates deployer integration helpers to use client-injected k8s/knative constructors and dialer. |
| pkg/config/config.go | Changes RegistryDefault to accept injected client and use per-client OpenShift detection. |
| pkg/builders/builders_int_test.go | Updates builder integration test code paths to use client-injected clientset. |
| e2e/e2e_test.go | Updates e2e helpers to use k8s.Client.RestConfig() instead of global client config. |
| e2e/e2e_core_test.go | Updates e2e core test to construct a kubeconfig-backed *k8s.Client. |
| cmd/run.go | Constructs a single kubeconfig-backed client for the command and passes it into client config/options. |
| cmd/root.go | Updates registry/namespace helpers to use new client API (still constructs clients on demand). |
| cmd/logs.go | Wraps logs gatherer and client construction (still needs aligning to single-client DI—see comments). |
| cmd/func-util/main.go | Updates func-util deploy flow to construct and pass a single *k8s.Client and use per-client OpenShift detection. |
| cmd/environment.go | Updates environment command to use kc.RestConfig() for cluster host retrieval. |
| cmd/deploy.go | Builds one *k8s.Client per deploy command, threads it through transports/credentials/deployers/messages. |
| cmd/deploy_test.go | Updates deploy tests to use FakeCluster instead of global OpenShift override and updated namespace call. |
| cmd/config.go | Threads kube client through config prompts (volumes/envs). |
| cmd/config_volumes.go | Injects *k8s.Client into volume prompt flow and list helpers. |
| cmd/config_envs.go | Injects *k8s.Client into env prompt flow and list helpers. |
| cmd/completion_util.go | Updates completion to use a kubeconfig-backed *k8s.Client. |
| cmd/client.go | Adds ClientConfig.K8sClient and updates transport/credentials/deployers/pipelines/syncer wiring to use a single injected client. |
| cmd/build.go | Builds one *k8s.Client per build command and threads it through build client options and transports. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func (c *Client) RestConfig() (*rest.Config, error) { | ||
| if c.cfg != nil { | ||
| return rest.CopyConfig(c.cfg), nil | ||
| } | ||
| cfg, err := c.cc.ClientConfig() |
| if o.inClusterDialer == nil { | ||
| o.inClusterDialer = k8s.NewLazyInitInClusterDialer(kc) | ||
| } |
| selectCA := func(ctx context.Context, serverName string) (*x509.Certificate, error) { | ||
| if strings.HasPrefix(serverName, openShiftRegistryHost) { | ||
| o.Do(func() { | ||
| ca, err = k8s.GetOpenShiftServiceCA(ctx) | ||
| ca, err = c.OpenShiftServiceCA(ctx) | ||
| if err != nil { |
| // Client for the given namespace | ||
| client, err := NewTektonClient(namespace) | ||
| client, err := NewTektonClient(pp.kc) | ||
| if err != nil { |
| func NewLogsCmd(newClient ClientFactory) *cobra.Command { | ||
| return newLogsCmd(newClient, knative.GetKServiceLogs) | ||
| return newLogsCmd(newClient, func(ctx context.Context, opts knative.LogsOptions, out io.Writer) error { | ||
| return knative.GetKServiceLogs(ctx, k8s.NewClientFromKubeconfig(), opts, out) | ||
| }) | ||
| } |
Add k8s.Client, a single cluster client built once at the top of the call chain and passed down. It wraps clientcmd.ClientConfig and owns OpenShift detection, cached per instance, with WithOpenShift for tests. The CLI resolves the client in the command (build, deploy, run) or in cmd.NewClient via the new ClientConfig.K8sClient field, and hands it to the transport, the credentials provider, and the deploy decorator. The OpenShift helpers GetOpenShiftServiceCA and GetOpenShiftDockerCredentialLoaders become methods and their global forms are removed. Package-level functions still used by code not yet migrated (pkg/k8s internals, pkg/config, knative deployer) stay as they are and are marked for removal; no new callers were added. Tests use a fake API server (testing.FakeCluster) instead of the package-level SetOpenShiftForTest override.
Every cluster-facing helper in pkg/k8s now takes the *Client as its first argument after ctx: secrets, configmaps, persistent volumes, pod logs, service accounts, manifestival, CheckResourcesArePresent. The in-cluster dialer and fnhttp.NewRoundTripper are built from the Client instead of a clientcmd.ClientConfig, and the pod security contexts take the OpenShift flag from the Client rather than a package global. Callers pass the Client they already hold (deployers, tekton PipelinesProvider) or resolve it at the command (config envs/volumes, logs). The operator syncer and tekton clients still resolve from the kubeconfig themselves; they are marked TODO for the next step. SetOpenShiftForTest is gone: the security context tests pass the flag directly. The shared rest.Config is copied before the dialer and volume upload mutate it.
767b14d to
16c1dda
Compare
Finish the client injection. Every cluster-facing component now receives its *k8s.Client from its constructor or caller; nothing below the command layer reads the kubeconfig on its own. - tekton: NewTektonClient and NewTektonClients take the client; the PipelinesProvider passes its own to pipeline, pipeline-run and PAC repository deletion. - pac: NewTektonPacClientAndResolvedNamespace, DetectPACInstallation, DetectPACOpenShiftRoute and GetPACInfo take the client. - operator: NewSyncer takes the client and passes it to SyncFunctionCR. - config: Global.RegistryDefault takes the client for OpenShift detection. - cmd: the remaining direct readers (defaultNamespace, registry, environment, completion, deploy messages) resolve the client explicitly or receive the command's client. With no callers left, the package-level GetClientConfig, NewKubernetesClientset, NewDynamicClient, NewClientAndResolvedNamespace, GetDefaultNamespace, GetDefaultOpenShiftRegistry and IsOpenShift are removed together with the process-wide OpenShift detection cache. The kubeconfig loading rules live in one unexported function behind NewClientFromKubeconfig. Integration and e2e tests construct their client with NewClientFromKubeconfig at the top of the test.
16c1dda to
218f79a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 91 out of 91 changed files in this pull request and generated 7 comments.
Suppressed comments (1)
pkg/knative/client.go:40
- This has the same injected-client breakage as
NewServingClient: an unrelated missingKUBECONFIGoverrides a validkcbeforekc.RestConfig()runs. That preventsNewClientFromConfigusers from constructing the eventing client; validation must be scoped to loader-backed clients rather than performed globally here.
| ) | ||
|
|
||
| func NewServingClient(namespace string) (clientservingv1.KnServingClient, error) { | ||
| func NewServingClient(kc *k8s.Client, namespace string) (clientservingv1.KnServingClient, error) { |
| // OpenShiftServiceCA fetches the OpenShift service CA certificate by creating | ||
| // a temporary ConfigMap annotated for CA bundle injection. | ||
| func (c *Client) OpenShiftServiceCA(ctx context.Context) (*x509.Certificate, error) { | ||
| client, ns, err := c.ClientAndNamespace("") |
| c.coreV1 = client.CoreV1() | ||
|
|
||
| c.namespace, _, err = c.clientConfig.Namespace() | ||
| c.namespace, err = c.client.DefaultNamespace() |
|
|
||
| // figure out pac installation namespace | ||
| installed, _, err := pac.DetectPACInstallation(ctx) | ||
| installed, _, err := pac.DetectPACInstallation(ctx, pp.kc) |
| } | ||
| cfg, _ := config.NewDefault() | ||
| return cfg.RegistryDefault() | ||
| return cfg.RegistryDefault(k8s.NewClientFromKubeconfig()) |
| // Gets the cluster host | ||
| var host string | ||
| cc, err := k8s.GetClientConfig().ClientConfig() | ||
| cc, err := k8s.NewClientFromKubeconfig().RestConfig() |
| // OpenShiftDockerCredentialLoaders returns a credential loader for the | ||
| // internal OpenShift registry, authenticating with the active user's token. | ||
| func (c *Client) OpenShiftDockerCredentialLoaders() []creds.CredentialsCallback { | ||
| rawConf, err := c.RawConfig() |
Changes
🎁 Add
k8s.Client, one cluster client built once in CLI and passed down. It wraps the kubeconfig loader (NewClient, NewClientFromKubeconfig) or a bare rest.Config (NewClientFromConfig, for library users with a host and token and no kubeconfig). It owns OpenShift detection, cached per instance, with WithOpenShift for tests.🧹 Every cluster-facing constructor and helper takes the client: knative/k8s/keda deployers, describers, removers and all pkg/k8s helpers (secrets, configmaps, PVCs...
🗑️ Remove the package-level k8s-derived helpers like
GetClientConfig,NewKubernetesClientset,IsOpenShiftand the global OpenShift detection cache.🧹 Tests:
testing.FakeClusterruns a fake API server answering the OpenShift discovery probe, so the deploy tests exercise real detection instead of a global override; security context tests pass the OpenShift flag directly.There now should exist 2 modes of this k8s client construction:
KUBECONFIG->~/.kube/config-> in-clusterfuncmost likely - no kubeconfig)NOTE: RawConfig() and DefaultNamespace() return err since they do require kubeconfig.
either way:
This is the DI half of #3841 , split out so it can be reviewed (hopefully) easier. There should be 0 user-visible behavior change.
reviewing
The three commits are ordered for review (introduce the client and wire the CLI; inject into pkg/k8s; inject into tekton/operator/config and delete the globals). Each builds and passes tests on its own (AI tested);
Each intermediate commit might hold some TODOs or comments thats removed by the followup commit -> I intend to squash after reviews
/kind cleanup
Relates to #3841
Release Note