Remove requirement for certificate authority and certificate authority data in kubeconfig. #222
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
k8s-ci-robot
added
the
cncf-cla: no
k8s-ci-robot
added
the
size/M
|
I already signed an agreement and linked to GitHub. Maybe it needs time to propagate? |
|
@twitchax your commits have a different email address, I think, see: |
twitchax
force-pushed
the
config_authority_fix
branch
from
70d6638
to
c4caf22
Compare
k8s-ci-robot
added
cncf-cla: yes
|
@brendandburns, I see. Fixed and force pushed. Thanks! |
There was a problem hiding this comment.
I was a bit confused by the title - it looks like we're actually addressing the case where there is neither certificate-authority nor certificate-authority-data (previously it would work without certificate-authority-data but only if certificate-authority was provided, which it wasn't in the test config). Is that correct or have I misunderstood? If that is the intent, might be good to update the PR title/commit message. But "Ivan has misunderstood" is more likely I suspect...
| @@ -155,45 +155,32 @@ private void SetClusterDetails(K8SConfiguration k8SConfig, Context activeContext | |||
|
|
|||
| if (clusterDetails?.ClusterEndpoint == null) | |||
| { | |||
| throw new KubeConfigException($"Cluster not found for context {activeContext} in kubeconfig"); | |||
| throw new KubeConfigException($"Cluster not found for context `{activeContext}` in kubeconfig"); | |||
There was a problem hiding this comment.
Is this change for consistency with other error messages, or just personal preference? I feel backticks are a bit unusual in C# messages; if we do want to change, is there a reason to prefer them over single quotes?
There was a problem hiding this comment.
The backticks are personal preference, but I do think that specific names of things referenced in an error message should be delimited in one manner or another. Switch to single quotes or remove?
src/KubernetesClient/KubernetesClientConfiguration.ConfigFile.cs
Outdated
Show resolved
Hide resolved
twitchax
changed the title
|
@itowlson, correct on the confusing title. Fixed. |
Update casing of `URL` at @itowlson suggestion. Co-Authored-By: twitchax <twitchax@gmail.com>
|
@itowlson, ping. :) |
|
This looks good to me. @itowlson are you ok with the tick-marks? |
|
I don't feel strongly enough about the use of backticks to think it's worth holing this PR for. I'd like to have a convention for how we delimit names but it can wait. /lgtm |
k8s-ci-robot
added
the
lgtm
|
@itowlson, I do not mind updating the PR for single quotes or removing them. Just let me know. |
|
@twitchax Let's get this PR merged as is, and come back to the delimiter stuff as a separate issue across the codebase. And apologies for not getting back earlier on this. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, twitchax The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This PR addresses #220.
Basically, we remove the requirement upon validation (and the impacted test) for certificate authority data in the
kubeconfigfile. Thekubeconfigwill work just fine inkubectlwithout this data if the CA is well-known (e.g., LetsEncrypt). Therefore, this PR removes that validation step, thereby pushing any failures due to unknown CA issues to the time of negotiation.