fix: removed duplicate code in KubeConfigUpdate. Added tests#310
fix: removed duplicate code in KubeConfigUpdate. Added tests#310adietish wants to merge 1 commit into
Conversation
Signed-off-by: Andre Dietisheim <adietish@redhat.com>
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR refactors the kubeconfig update pipeline to centralize persistence logic in the base class via an injected ChangesKubeconfig persistence and context refactoring
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt (1)
74-75: ⚡ Quick winAvoid positional
saveConfigoverloads with identical collection types.These overloads take
users,clusters, andcontextsas separateArrayList<Any?>parameters, and the private delegate reorders them. A future call-site swap will still compile and can write a malformed kubeconfig. PassingContextEntriesdirectly would make this helper much safer.Refactor direction
- protected fun saveConfig( - config: KubeConfig, - users: ArrayList<Any?>, - clusters: ArrayList<Any?>, - contexts: ArrayList<Any?>, - currentContext: String - ) { - saveConfig(contexts, clusters, users, config.preferences, currentContext, config.path) - } + protected fun saveConfig(config: KubeConfig, entries: ContextEntries) { + saveConfig( + entries.contexts, + entries.clusters, + entries.users, + config.preferences, + entries.currentContextName, + config.path + ) + }Then update the two callers to pass
entries/contextEntriesdirectly.Also applies to: 85-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt` around lines 74 - 75, The saveConfig overloads currently accept separate ArrayList<Any?> for users, clusters, and contexts which is error-prone; change the private helper signature (the delegated saveConfig) to accept the strongly-typed ContextEntries (or the appropriate ContextEntries wrapper type) instead of positional ArrayList<Any?> parameters, update the two callers that currently pass entries / contextEntries to pass the ContextEntries object directly, and remove any manual reordering logic inside saveConfig so the original entries are preserved by type; reference the saveConfig overloads and the callers that pass entries/contextEntries to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt`:
- Around line 209-214: When setting a token in setTokenFor (and in the
UpdateToken flow), also clear any existing client cert fields to avoid
presenting both credentials: after Utils.setValue(this, token,
arrayOf("user","token")) remove or null out "user.client-certificate-data" and
"user.client-key-data" on the same user object (use the existing Utils helpers —
e.g., Utils.setValue(this, null, arrayOf("user","client-certificate-data")) /
Utils.setValue(this, null, arrayOf("user","client-key-data")) or the
Utils.removeValue equivalent) so UpdateToken mirrors UpdateClientCert's behavior
of removing the opposite auth field.
In
`@src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt`:
- Around line 29-32: The tests currently use a single mock (mockPersister)
returned by testPersisterFactory so they only assert multiple save() calls
rather than per-file routing; change testPersisterFactory to create and return a
distinct mock ConfigPersister for each File (store them in a map keyed by
File/path) and update assertions to verify each file's dedicated mock was
invoked exactly once (e.g., verify(exactly = 1) {
persistersByFile[userFile].save(...) } and verify for the other file including
the context name matcher) so the test validates that each config file is
persisted by its own factory-produced persister.
---
Nitpick comments:
In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt`:
- Around line 74-75: The saveConfig overloads currently accept separate
ArrayList<Any?> for users, clusters, and contexts which is error-prone; change
the private helper signature (the delegated saveConfig) to accept the
strongly-typed ContextEntries (or the appropriate ContextEntries wrapper type)
instead of positional ArrayList<Any?> parameters, update the two callers that
currently pass entries / contextEntries to pass the ContextEntries object
directly, and remove any manual reordering logic inside saveConfig so the
original entries are preserved by type; reference the saveConfig overloads and
the callers that pass entries/contextEntries to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d0d178b7-c346-4934-b30f-3011c0683346
📒 Files selected for processing (3)
src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigEntries.ktsrc/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.ktsrc/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt
| private fun setTokenFor(username: String, config: KubeConfig) { | ||
| config.users?.find { user -> | ||
| username == Utils.getValue(user, arrayOf("name")) | ||
| }?.apply { | ||
| Utils.setValue(this, token, arrayOf("user", "token")) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt"
echo "== File existence =="
ls -l "$FILE"
echo
echo "== setTokenFor block (around lines 190-240) =="
nl -ba "$FILE" | sed -n '180,240p'
echo
echo "== UpdateToken / UpdateClientCert (best-effort ranges) =="
rg -n "class UpdateToken|class UpdateClientCert|fun setTokenFor|setClientCert" "$FILE" -n || true
# Print larger region around each match line
for ln in $(rg -n "class UpdateToken|class UpdateClientCert|fun setTokenFor|setClientCert" "$FILE" | cut -d: -f1 | sort -n | uniq); do
echo
echo "== Context around $ln =="
nl -ba "$FILE" | sed -n "$((ln-30)),$((ln+40))p"
doneRepository: redhat-developer/devspaces-gateway-plugin
Length of output: 341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt"
echo "== setTokenFor block (approx 180-240) =="
awk 'NR>=180 && NR<=240 {printf "%6d %s\n", NR, $0}' "$FILE"
echo
echo "== Auth-mode updater classes/logic near matches =="
rg -n "class UpdateToken|class UpdateClientCert|fun setTokenFor|setClientCert|removeValue|client-certificate-data|client-key-data|arrayOf\\(\"user\", \"token\"\\)" "$FILE" || trueRepository: redhat-developer/devspaces-gateway-plugin
Length of output: 3576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt"
echo "== UpdateClientCert.setClientCert block =="
awk 'NR>=232 && NR<=285 {printf "%6d %s\n", NR, $0}' "$FILE"
echo
echo "== Utils.removeValue usage in this file =="
rg -n "Utils\\.removeValue" "$FILE" || true
echo
echo "== Find Utils.removeValue definition (search repo) =="
rg -n "fun removeValue\\(" -S . || true
echo
echo "== Search for token vs cert precedence in repo comments/usages =="
rg -n "client-certificate-data|client-key-data|auth.*token|precedence" -S src/main/kotlin || trueRepository: redhat-developer/devspaces-gateway-plugin
Length of output: 11712
🌐 Web query:
Kubernetes client-go kubeconfig precedence when both token and client-certificate-data are set
💡 Result:
In client-go, when both a bearer token (token or token-file) and a client certificate (client-certificate-data and client-key-data) are present in the kubeconfig, client-go treats this as a mutual exclusivity error: it will not construct a request with both token auth and certificate auth. Specifically, transport/round_trippers.go checks for “basic auth or token auth” and errors if both username/password or bearer token are set together (it refuses combining certain auth types) [1]. In the rest.Config structures, the client TLS material has its own precedence rules (CertData overrides CertFile, KeyData overrides KeyFile) [2] [3], and the bearer token file has precedence over bearer token string (the last successfully read value from the token file takes precedence over the bearer token string) [2] [3]. But precedence between token auth vs client-certificate-data is effectively: “no precedence because it’s invalid to have both”; client-go will fail configuration/transport setup rather than choose one auth method. Practical implication: ensure your kubeconfig “user” section includes either: - token (or tokenFile / bearerTokenFile) and not client-certificate-data/client-key-data, or - client-certificate-data/client-key-data (and not a bearer token). (Separate from client-go’s behavior, the Kubernetes API server supports X.509 client certificate authentication when presented and validated against its client trust CA) [4].
Citations:
- 1: https://github.com/kubernetes/client-go/blob/da6e2946e51b907f353947a24a91ae91393df734/transport/round_trippers.go
- 2: https://github.com/kubernetes/client-go/blob/master/rest/config.go
- 3: https://github.com/kubernetes/client-go/blob/master/transport/config.go
- 4: https://kubernetes.io/docs/reference/access-authn-authz/authentication/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt"
UTILS="src/main/kotlin/com/redhat/devtools/gateway/openshift/Utils.kt"
echo "== All occurrences of client-certificate-data/client-key-data in KubeConfigUpdate.kt =="
rg -n "client-certificate-data|client-key-data|removeValue\\(this, arrayOf\\(\"user\", \"token\"\\)" "$FILE" || true
echo
echo "== UpdateToken setTokenFor body (exact) =="
awk 'NR>=200 && NR<=220 {printf "%6d %s\n", NR, $0}' "$FILE"
echo
echo "== UpdateClientCert setClientCert body (exact) =="
awk 'NR>=256 && NR<=275 {printf "%6d %s\n", NR, $0}' "$FILE"
echo
echo "== Utils.removeValue implementation (exact) =="
awk 'NR>=1 && NR<=120 {printf "%6d %s\n", NR, $0}' "$UTILS"Repository: redhat-developer/devspaces-gateway-plugin
Length of output: 5117
🌐 Web query:
In Kubernetes/client-go, what happens if a kubeconfig user contains both bearer token (user.token) and client-certificate-data/client-key-data? Is it rejected, ignored, or does it authenticate via both?
💡 Result:
In Kubernetes client-go, if a kubeconfig user definition contains both a bearer token (user.token) and client-certificate-data/client-key-data, the resulting rest.Config object will contain both sets of credentials [1][2]. When client-go builds the REST client configuration from the kubeconfig file, it populates the rest.Config struct with the values present in the user section [1]. Specifically: - The Token is stored in the BearerToken field [1][3]. - The certificate and key data are stored in the TLSClientConfig.CertData and TLSClientConfig.KeyData fields [1][3][2]. Because these fields are independent within the rest.Config object, the client library does not reject or ignore one in favor of the other during the loading process [1][3]. When a request is eventually made, the underlying transport used by client-go will generally prioritize the authentication methods it supports. In practice, when both are provided, the Kubernetes API server or the client transport logic may behave differently based on the specific implementation, but client-go itself does not "choose" one over the other at the configuration loading stage [1][2]. If you are using these credentials, it is important to note that the API server's authentication layer will evaluate them according to its own configured authentication strategies [4]. If the server receives a request with both a client certificate and an Authorization header (bearer token), the API server's authentication chain will typically attempt to authenticate the user using the provided methods sequentially [4]. Most Kubernetes API server configurations will successfully authenticate the user if either method is valid, though this can vary depending on the specific enabled authentication modules [4].
Citations:
- 1: https://github.com/kubernetes/client-go/blob/master/tools/clientcmd/client_config.go
- 2: https://github.com/kubernetes/client-go/blob/master/transport/config.go
- 3: https://github.com/kubernetes/client-go/blob/master/rest/config.go
- 4: https://kubernetes.io/docs/reference/access-authn-authz/authentication/
Clear client-cert fields when switching back to token auth.
UpdateClientCert removes user.token, but UpdateToken only sets user.token and leaves user.client-certificate-data / user.client-key-data in the same kubeconfig user. Since client-go can load token + client cert together into the same config, the resulting requests may present both credentials, letting the API server authenticate via either method and causing inconsistent auth behavior.
Suggested fix
private fun setTokenFor(username: String, config: KubeConfig) {
config.users?.find { user ->
username == Utils.getValue(user, arrayOf("name"))
}?.apply {
Utils.setValue(this, token, arrayOf("user", "token"))
+ Utils.removeValue(this, arrayOf("user", "client-certificate-data"))
+ Utils.removeValue(this, arrayOf("user", "client-key-data"))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun setTokenFor(username: String, config: KubeConfig) { | |
| config.users?.find { user -> | |
| username == Utils.getValue(user, arrayOf("name")) | |
| }?.apply { | |
| Utils.setValue(this, token, arrayOf("user", "token")) | |
| } | |
| private fun setTokenFor(username: String, config: KubeConfig) { | |
| config.users?.find { user -> | |
| username == Utils.getValue(user, arrayOf("name")) | |
| }?.apply { | |
| Utils.setValue(this, token, arrayOf("user", "token")) | |
| Utils.removeValue(this, arrayOf("user", "client-certificate-data")) | |
| Utils.removeValue(this, arrayOf("user", "client-key-data")) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt`
around lines 209 - 214, When setting a token in setTokenFor (and in the
UpdateToken flow), also clear any existing client cert fields to avoid
presenting both credentials: after Utils.setValue(this, token,
arrayOf("user","token")) remove or null out "user.client-certificate-data" and
"user.client-key-data" on the same user object (use the existing Utils helpers —
e.g., Utils.setValue(this, null, arrayOf("user","client-certificate-data")) /
Utils.setValue(this, null, arrayOf("user","client-key-data")) or the
Utils.removeValue equivalent) so UpdateToken mirrors UpdateClientCert's behavior
of removing the opposite auth field.
| private val kubeConfigPath = Path.of("/test/kubeconfig-primary") | ||
| private val mockPersister = mockk<ConfigPersister>(relaxed = true) | ||
| private val testPersisterFactory: (File) -> ConfigPersister = { mockPersister } | ||
|
|
There was a problem hiding this comment.
Per-file persistence routing is not actually validated by these tests.
testPersisterFactory always returns the same mockPersister, so “different files” tests only prove multiple save() calls happened, not that each config file was persisted through its own factory-produced persister. A file-routing regression can still pass.
Suggested test hardening
- private val mockPersister = mockk<ConfigPersister>(relaxed = true)
- private val testPersisterFactory: (File) -> ConfigPersister = { mockPersister }
+ private val persistersByFile = mutableMapOf<File, ConfigPersister>()
+ private val testPersisterFactory: (File) -> ConfigPersister = { file ->
+ persistersByFile.getOrPut(file) { mockk(relaxed = true) }
+ }// in split-file tests
val userPersister = persistersByFile[userConfigFile.toFile()]!!
val currentContextPersister = persistersByFile[currentContextConfigFile.toFile()]!!
verify(exactly = 1) { userPersister.save(any(), any(), any(), any(), any()) }
verify(exactly = 1) { currentContextPersister.save(any(), any(), any(), any(), match { it == data.contextName }) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 249-320, 365-450
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt`
around lines 29 - 32, The tests currently use a single mock (mockPersister)
returned by testPersisterFactory so they only assert multiple save() calls
rather than per-file routing; change testPersisterFactory to create and return a
distinct mock ConfigPersister for each File (store them in a map keyed by
File/path) and update assertions to verify each file's dedicated mock was
invoked exactly once (e.g., verify(exactly = 1) {
persistersByFile[userFile].save(...) } and verify for the other file including
the context name matcher) so the test validates that each config file is
persisted by its own factory-produced persister.
1 participant
No description provided.