kubeconfig: Append /clusterURL to the AuthInfo added by crc
#4020
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
gbraad
changed the title
/clusterURL to the AuthInfo added by crc
|
@anjannath #3940 (comment) check the commit from this comment because we don't want to modify the complete kubeconfig file instead just want to make sure our entry is according to |
checked the commit, and i think the changes here are similar to what you posted there, since we have two different functions for updating the kubeconfig one for microshift ( the other additional function/changes in this PR to the i did test with an existing kubeconfig file with entries from |
On microshift side we don't create kubeconfig (as part of crc codebase), we just merge what is created by microshift so should we fix it because we are merging or microshift should fix that kubeconfig?
|
Since microshift document using these files outside of the machine running the cluster ( https://access.redhat.com/documentation/en-us/red_hat_build_of_microshift/4.14/html/configuring/microshift-kubeconfig ), imo it would be nice if the users in there were suffixed, especially as they create multiple kubeconfig depending on the hostname the cluster is going to be accessed from. |
does this need an issue? According to the document they generate these files in:
I think this strategy makes sense too, as it prevents any conflict. the file is specific for the microshift usecase. why not write ours in: |
Is it necessary to have the port |
in the case of standalone kubeconfig file that is only containing one pair of cluster and user, there's no provision for having conflicts, the conflict may occur when we merge microshift generated kubeconfig to the user's kubeconfig, so i think its more important that we namespace it (add the cluster suffix) when the merging happens. and we can also open an issue in microshift repo to know if they think this is something they would consider doing. |
we need to merge the kubeconfig onto the default kubeconfig location, i.e |
The kubeconfig can be set by environment variable... something I also expect: I would also expect PD to pick this up. Something the extension should do? Perhaps even they should allow additional files to be loaded in PD. Especially as the microshift documentation specifies they will use |
currently
this only says that when the |
regardless, seems to be 'convention' as it is documented. Although, something for PD+extension to handle.
Shouldn't we do this? /cc: @praveenkumar @cfergeau and therefore not 'merge' our config in by default (or as default now, but allow this to be configured). |
If |
is this possible? if not, file an issue for something like a
I believe this is what happens... but in that case suggesting merging content to However, I still did not get an answer to: #4020 (comment) about the port in the suffix. Is this necessary? |
Nothing is necessary, even appending a random string would work instead of the cluster name. Having the port number makes it consistent with the example in #3940. |
|
but i think it'd still be good to append
|
Don't want to lock the user in so he can only use port
the identifier is more of a 'nice to have' for the user, but 'needs to be unique' due to how they store it in code when read/parsed. it does not add anything else than user-facing info. |
We are not hardcoding
Right, Also since we are merging microshift kubeconfig file to global one it also remove the override in case same user exist for different cluster.
most of the tools do it (like kind/minikube ..etc.) but they don't merge but add the context like we are doing for OCP by generating the required field. Technically on microshift side we are getting the required info from the generated kubeconfig by parsing it and then appending it to global kubeconfig (something same might oc/kubectl can do behind the scene when KUBECONFIG env variable set for multiple kubeconfigs) . With our current approach (merging/appending) user can use oc command on OCP/microshift cluster without everytime setting the KUBECONFIG env which is big plus since most of the time user don't even use |
I can read the code, but I am questioning if this is necessary. It might be better to just say:
and therefore substring on the port/semi-colon. |
So adding the
|
ahh, yes it'd cause issues since it'd be the same but we allow only one preset to run at a time, so kubeconfig will not have entries from both, as to move to okd preset from ocp we'd have to run |
there is no |
Do not understand this part. To be consistent with the other entries in |
this adds /api.crc.testing:6443 i.e the API server URL to the name key of the AuthInfos list this prevents the following error from `oc` when kubeconfig has two entries with same name in the AuthInfos list: ``` error: error loading config file ".kube/config": error converting *[]NamedAuthInfo into *map[string]*api.AuthInfo: duplicate name "user" in list: ```
anjannath
force-pushed
the
issue_3940
branch
from
e7ced55 to
a711b26
Compare
|
@anjannath: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
so whenever we do |
this appends
/api.crc.testing:6443i.e the API server URL to the name key of the AuthInfos listthis prevents the following error from
ocwhen kubeconfig has two entries with same name in the AuthInfos list:Fixes #3940