Handle unexpected names in the kubeconfig#1087
Conversation
|
@twz123 I pushed another commit, is that what you were suggesting? It's currently a separate commit, I'll squash them once we are happy with that change. Also: This still defensively nil-checks the lookups into the loaded config. If it's fine to assume that the config is reasonable, then I'd rather remove the nil checks and instead put a comment there about expectations. |
Nearly! As mentioned in #1087 (comment), I think it's better not to construct a new config (like I suggested initially), but rather modify the parsed one.
I think the explicit error handling makes a lot of sense. I think panics are good if invariants in the same program are broken. But in this case, the kubeconfig is external input, so we should treat it as such. |
pschichtel
force-pushed
the
bugfix/work-with-differing-names
branch
2 times, most recently
from
90006fe to
4d27c2a
Compare
pschichtel
force-pushed
the
bugfix/work-with-differing-names
branch
from
4d27c2a to
c2abb09
Compare
pschichtel
force-pushed
the
bugfix/work-with-differing-names
branch
from
c2abb09 to
c1250df
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens kubeconfig normalization to avoid nil dereferences when kubeconfig object names differ from previously hardcoded expectations, by deriving the source objects from the current context and rewriting the config to the expected names.
Changes:
- Update
kubeConfigto resolve context/cluster/auth-info viacurrent-contextand return clear errors when references are missing. - Normalize the kubeconfig by rewriting the
Clusters,Contexts, andAuthInfosmaps to the expected names and updating the API server address. - Add unit tests covering extension preservation, dropping non-current objects, and missing-reference error cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
phase/get_kubeconfig.go |
Reworks kubeconfig normalization to use the current context and adds nil-guard errors to prevent crashes. |
phase/get_kubeconfig_test.go |
Adds tests for extension preservation and error cases around missing referenced objects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
twz123
left a comment
twz123
left a comment
There was a problem hiding this comment.
Word Salad Machine has some valid nits in the tests. Other than that, looks good. Thank you!
kke
force-pushed
the
bugfix/work-with-differing-names
branch
from
c1250df to
bc0bc8b
Compare
kke
force-pushed
the
bugfix/work-with-differing-names
branch
from
bc0bc8b to
fdc1342
Compare
| require.NoError(t, err) | ||
| require.Equal(t, expectedOutput, actualOutput) |
K0s 1.36+ changed these names breaking the old logic. Also add some tests to ensure that unrelated data from the source config is retained and the validation properly triggers. Signed-off-by: Phillip Schichtel <phillip@schich.tel> Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
kke
force-pushed
the
bugfix/work-with-differing-names
branch
from
3959a24 to
b9583d9
Compare
Previously the names have been hardcoded, but it seems that my experimental 1.36 cluster has different names, which lead to segfaults. I changed to the logic to take the first and only available key and normalizes it to the expected names.