Skip to content

Support KUBECONFIG environment variable #3383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

datho7561
Copy link
Contributor

  • Watch for changes in file(s) indicated by KUBECONFIG when it's set
  • Warn when multiple config files are specified in KUBECONFIG, since the Kubernetes client library we are using doesn't support this use case

Closes #3382

Signed-off-by: David Thompson [email protected]

@datho7561 datho7561 requested a review from vrubezhny October 2, 2023 16:11
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (2bc56d9) 35.64% compared to head (fee0cf2) 35.68%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3383      +/-   ##
==========================================
+ Coverage   35.64%   35.68%   +0.03%     
==========================================
  Files          79       79              
  Lines        5549     5571      +22     
  Branches     1092     1096       +4     
==========================================
+ Hits         1978     1988      +10     
- Misses       3571     3583      +12     
Files Coverage Δ
src/extension.ts 68.36% <100.00%> (+0.65%) ⬆️
src/openshift/openshiftItem.ts 42.30% <ø> (-2.14%) ⬇️
src/openshift/project.ts 40.42% <0.00%> (ø)
src/openshift/cluster.ts 8.48% <50.00%> (+0.21%) ⬆️
src/explorer.ts 32.07% <27.77%> (+0.94%) ⬆️
src/util/kubeUtils.ts 50.00% <40.90%> (-4.24%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@datho7561 datho7561 force-pushed the 3382-handle-kubeconfig-env-var branch from 0e2f5fe to 95a46fe Compare October 6, 2023 18:23
@vrubezhny
Copy link
Contributor

vrubezhny commented Oct 18, 2023

@datho7561 If I set KUBECONFIG env. variable to point to some Kube config other that the default one - should the Kube config from that env. variable to be used and shown in Application Explorer?

image

In case of multiple Kube configs set in the env. variable I get Application Explorer "broken" not allowing to login to cluster...

image

Shouldn't we use a Kube config defined in KUBECONFIG env. variable (or pick up the first one if multiple set) to access a cluster?

UPD: The multiple Kube configs problem probably is not a relevant one - it happened because I actually copied the same config file several times. But OS Tools had never told me about the problems happened during the configuration files loading and data merging...
It looks like the real problem specified in error was hid when throwing an exception and never shown to a user in the following lines, while probably we should show an error notification pointing to the real problem described in error, so the user knows what actually has happened and what configuration problem is to be fixed:

} catch (error) {
throw new Error('Kubernetes configuration cannot be loaded. Please check configuration files for errors and fix them to continue.');
}

@datho7561
Copy link
Contributor Author

I'll fix the tree item that displays the current kubeconfig file so that it respects the environment variable.

The library we are using to read the Kubeconfig doesn't handle merging Kubeconfigs when there are multiple ones specified. What path forward makes the most sense to you:

  • Keep the current implementation: as long as there are no overlapping definitions (eg. same cluster defined in both KUBECONFIG) it works as expected
  • Only use the first KUBECONFIG that the user specifies

Either way, it should warn you if you use the "Multiple KUBECONFIG" feature. I'll double check this usecase.

@vrubezhny
Copy link
Contributor

vrubezhny commented Oct 18, 2023

@datho7561 I think we should follow the way the library does it.

If we can point which Kube config to be used by the library - let's load the only that config - we don't need the others if they're not going to be used. (And in this case we probably can add a selector preference to allow users to change to a different config from the list of the ones defined in KUBECONFIG env. variable)

If the library gets a config not directly from a Kube config file but, for example, as a json-like input, then probably we should merge all the configs specified in KUBECONFIG env. variable (notifying users on all possible problems) and provide the result to the library.

Honestly I don't know yet how is that library used and initialized with data so, it's hard to make a choice for me here.

Either way, it should warn you if you use the "Multiple KUBECONFIG" feature. I'll double check this usecase.

This warning appeared for me with no problem - I was warned on using multiple configs when more than one config file was set up in KUBECONFIG env. variable.

@datho7561 datho7561 force-pushed the 3382-handle-kubeconfig-env-var branch from 95a46fe to 47ebefc Compare October 19, 2023 14:06
@datho7561
Copy link
Contributor Author

I pushed a new commit that forces the user to pick one of the KUBECONFIG files if there are multiple when the extension starts. I'm not sure how to feel about this, since (unless we do a large refactor) we need to block the rest of the extension from starting until the user selects the file.

vrubezhny
vrubezhny previously approved these changes Oct 20, 2023
Copy link
Contributor

@vrubezhny vrubezhny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

The contexts are loaded only from a user selected config file in case of multiple configs are provided through the KUBECONFIG environment variable.

- Watch for changes in file(s) indicated by `KUBECONFIG` when it's set
- Force users to pick one file when multiple config files are specified in `KUBECONFIG`,
  since the Kubernetes client library we are using doesn't support using multiple files

Closes redhat-developer#3382

Signed-off-by: David Thompson <[email protected]>
@datho7561 datho7561 force-pushed the 3382-handle-kubeconfig-env-var branch from 42f0290 to fee0cf2 Compare October 20, 2023 13:13
@datho7561 datho7561 merged commit c304f01 into redhat-developer:main Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application explorer is broken if KUBECONFIG env var is set
2 participants