yuqi1129 commented on PR #13724:
URL: https://github.com/apache/gravitino/pull/13724#issuecomment-6096393243
Reviewed the complete diff at `cb52e205f16e93984454dc8d27b58baefb9d28cd`
against `7ff63ddbfda3bfb0f35bd71568f0fdf1ecee73be`.
**Verdict: please address the remaining kubeconfig validation gap before
merge.** The earlier fixes cover the reported cases, but the same wrong-cluster
fallback is still reachable through another malformed configuration.
**[Important] A named cluster without a cluster object passes validation and
keeps the default API server.**
In `K8sClientUtils.createClientConfig` (lines 143–150), `getClusterNames`
(lines 186–192) checks only whether a name appears in the YAML. For example:
```yaml
apiVersion: v1
kind: Config
current-context: jobs
clusters:
- name: jobs
contexts:
- name: jobs
context: {cluster: jobs}
```
Using the compiled PR classes and fabric8 7.2.0, I reproduced
`createClientConfig` accepting this file and returning
`https://kubernetes.default.svc/`. Explicit `cluster: null` has the same
result. Fabric8's `mergeClusters` skips entries whose cluster object is null,
while the name-only guard accepts them. An empty `cluster: {}` instead produces
a bare `NullPointerException` from the missing server.
Please validate that the selected cluster has a non-null cluster object and
a usable server, with tests for missing/null/empty cluster content and a blank
server. Prefer validating and constructing from the same parsed snapshot,
avoiding the current second read/parse of the file. Do not simply reject the
default server URL: a valid kubeconfig could explicitly select it.
This part is not wired into the running executor yet, so existing
deployments are unaffected. Once it is wired in, the accepted configuration can
contact the wrong cluster; successful submission there would depend on that
cluster's authentication and permissions. I did not submit jobs to a real
cluster.
**[Nit, non-blocking] Repeated token-file I/O failures log a full WARN stack
trace on every token lookup.**
In `FileTokenProvider.getToken` (lines 82–88), after a token has been
loaded, a deleted or persistently unreadable file retains the old token but
logs the same failure on every call. Keeping the old token during replacement
is reasonable; consider throttling duplicate warnings while continuing to check
for recovery. This was assessed from the code and the existing deleted-file
test, not a logging performance benchmark.
Verification: all 20 module tests passed, as did `shadowJar`, Javadoc and
spotless checks. An additional probe loaded the shaded jar without external
fabric8 jars, created and closed the client, selected `JdkHttpClientImpl`, and
preserved the configured namespace. Temporarily bypassing the usable-context
guard made the existing context regression test fail; after restoring the exact
head, all 20 tests passed again. No real-cluster/HTTP integration test was run.
The earlier empty-token, null-metadata, error-message, namespace and
duration-key comments are addressed. Distribution wiring is explicitly covered
by #13616 and is a reasonable scope exclusion here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
