kubernetes / kubernetes/kubectl
Reduce useless duplicate reading of certificate data
- 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
Assessment
This issue has not been assessed yet.