kubernetes / kubernetes/kubectl

Reduce useless duplicate reading of certificate data

Open
#1,880 11 comments 0 reactions 1 assignee Claimed by @harshit-seth View on GitHub
kind/feature priority/backlog sig/cli triage/accepted
Dominant language
Go
Stars
3.3k
Forks
1k
PR merge metrics
No merged PRs in 30d

Description

### What would you like to be added?

When using `kubectl __complete -n ''` I measure about 180 opened files:
```
/home/user/.kube/config
/home/user/.kube/cache/discovery/api.k8s.io2.osso.nl/v1/serverresources.json
...
```
However, if I specify my config using files `certificate-authority: path-to/ca.crt` instead of `client-certificate-data: LS0tLS...` then that number explodes to about 500+ opened files.

Turns out it does the above, and then also rereads all the certificate files (for each CRD?):
```
$ strace -f -eread kubectl __complete -n '' 2>&1 | grep 'read(.*BEGIN PRIVATE' | wc -l
59
```
^- 58 reads too many
```
$ strace -f -eopenat kubectl __complete -n '' 2>&1 | grep -F .key | wc -l
118
```
^- 117 openat's too many

I'd like to see that reduced.

### Why is this needed?

When reading a file from the disk is expensive, e.g. when you have on-access virus scanning, doing useless opens/reads makes the tab completion unusably slow.

For CLI applications, I would say it should just read the certificate data once and then use those instead of reopening them. In fact, the 117 openat's too many are done _after_ connecting, so I doubt they are even used at all.

I let Mr. Claude work on it some, and it produced three patches to resolve this. Is this something you're willing to accept after I review them myself?

[edit]

The Claude generated root cause docs:

---

## 1. Root cause

The opens are **not** TLS-handshake related. They come from two places, both of which run
**once per REST client**, and kubectl builds **one REST client per GroupVersion** it touches.

The repeating 6-open signature in `strace.log`
(`user.crt, user.key, ca.crt, ca.crt, user.crt, user.key`) decomposes exactly as:

| # | opener | what it does |
|---|--------|--------------|
| 3 | `clientcmd.validateAuthInfo` + `clientcmd.validateClusterInfo`, via `DirectClientConfig.ConfirmUsable()` | pure **readability probe** — `os.Open` + `Close`, contents never read |
| 3 | `transport.dataFromSliceOrFile` ← `transport.loadTLSFiles` ← `transport.tlsConfigKey` | reads all three files **just to compute the TLS transport-cache key**, even on a cache *hit* |

Call chain, per GroupVersion (verified with `strace -k` stack traces):

```
resource.(*Builder).visitBySelector
└─ resource.(*Builder).getClient(gv)
└─ resource.ClientConfigFunc.unstructuredClientForGroupVersion
├─ clientConfigFn() # == MatchVersionFlags.ToRESTConfig
│ └─ genericclioptions.(*ConfigFlags).ToRESTConfig
│ └─ clientcmd.(*DirectClientConfig).ClientConfig
│ └─ ConfirmUsable()
│ ├─ validateAuthInfo -> open(client-certificate), open(client-key)
│ └─ validateClusterInfo -> open(certificate-authority)
└─ rest.RESTClientFor -> rest.HTTPClientFor -> rest.TransportFor
└─ transport.New
└─ transport.(*tlsTransportCache).get
└─ transport.tlsConfigKey
└─ transport.loadTLSFiles
-> read(ca), read(cert), read(key) # even on cache HIT
```

Neither layer caches:

* `ConfigFlags` memoizes the `clientcmd.ClientConfig` *object* (`usePersistentConfig`), but
**not** the result of calling `ClientConfig()` on it, so `ConfirmUsable()` re-probes the
files on every call. `MatchVersionFlags.ToRESTConfig` doesn't memoize either.
* `tlsConfigKey` reads the files *before* the cache lookup, so a hit still costs 3 opens.

---

[edit]

The Claude generated patches:
- Fix A — `client-go/transport`: don't read files to build the cache key

`TLSConfigFor` already calls `loadTLSFiles` itself, so a cache **miss** still loads everything.

- Fix B-alt — `cli-runtime/genericclioptions`: resolve the REST config once

`ConfigFlags.ToRESTConfig()` memoizes the resolved `*rest.Config` when `usePersistentConfig` is set

- Fix C — `client-go/tools/clientcmd`: memoize `ConfirmUsable`

`ConfirmUsable` is reached from both `ClientConfig()` and `Namespace()`, so every kubectl command probes each credential file twice. Memoizing avoids the double lookup.

- Fix D — `client-go/transport`: seed the caching cert loader

Pass `c.TLS.CertData, c.TLS.KeyData` to the `cachingCertificateLoader`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.