Support TLS Server Name overrides in kubeconfig file by surki · Pull Request #82056 · kubernetes/kubernetes

surki · 2019-08-28T05:31:14Z

Signed-off-by: Suresh Kumar Ponnusamy [email protected]

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #80596

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Support TLS Server Name overrides in kubeconfig file

k8s-ci-robot · 2019-08-28T05:31:16Z

Welcome @surki!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-08-28T05:31:23Z

Hi @surki. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

roycaihw · 2019-08-29T20:07:43Z

/cc @lavalamp @liggitt
/sig auth

staging/src/k8s.io/client-go/tools/clientcmd/overrides.go

staging/src/k8s.io/client-go/tools/clientcmd/api/types.go

surki · 2019-09-04T13:50:05Z

@liggitt @lavalamp Can you please take a look at this, your review comments have been addressed? Thanks

surki · 2019-09-11T08:34:34Z

/assign @smarterclayton

staging/src/k8s.io/kubectl/pkg/cmd/config/create_cluster.go

staging/src/k8s.io/client-go/tools/clientcmd/client_config.go

liggitt · 2019-09-25T16:16:18Z

/ok-to-test

liggitt · 2019-09-25T16:19:24Z

/priority important-longterm

Signed-off-by: Suresh Kumar Ponnusamy <[email protected]>

And update RecommendedClusterOverrideFlags/BindClusterFlags

surki · 2019-10-08T13:26:58Z

/retest

surki · 2019-10-09T04:59:35Z

@liggitt I rebased the changes to latest master, all tests are passing.
Please let me know if you want me to squash the changes.

staging/src/k8s.io/kubectl/pkg/cmd/config/create_cluster.go

liggitt · 2019-10-09T15:20:34Z

Thought of a couple more issues. You can override the server name on the CLI with --server. For parity, this should also be overrideable.

kubernetes/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go

Lines 43 to 44 in 8577711

    
           flagAPIServer        = "server" 
        
           flagInsecure         = "insecure-skip-tls-verify"

kubernetes/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go

Lines 159 to 168 in 8577711

    
           // bind cluster flags 
        
           if f.APIServer != nil { 
        
           	overrides.ClusterInfo.Server = *f.APIServer 
        
           } 
        
           if f.CAFile != nil { 
        
           	overrides.ClusterInfo.CertificateAuthority = *f.CAFile 
        
           } 
        
           if f.Insecure != nil { 
        
           	overrides.ClusterInfo.InsecureSkipTLSVerify = *f.Insecure 
        
           }

kubernetes/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go

Lines 294 to 299 in 8577711

    
           if f.APIServer != nil { 
        
           	flags.StringVarP(f.APIServer, flagAPIServer, "s", *f.APIServer, "The address and port of the Kubernetes API server") 
        
           } 
        
           if f.Insecure != nil { 
        
           	flags.BoolVar(f.Insecure, flagInsecure, *f.Insecure, "If true, the server's certificate will not be checked for validity. This will make your HTTPS connections insecure") 
        
           }

When overriding the server on the CLI, I would expect the server verify name to be overridden as well. This is similar to how overriding the insecure or CA settings on the CLI overrides all the trust settings (insecure/ca-file/ca-data) together:

kubernetes/staging/src/k8s.io/client-go/tools/clientcmd/client_config.go

Lines 452 to 461 in 8577711

    
           // * An override of --insecure-skip-tls-verify=true and no accompanying CA/CA data should clear already-set CA/CA data 
        
           // otherwise, a kubeconfig containing a CA reference would return an error that "CA and insecure-skip-tls-verify couldn't both be set". 
        
           // * An override of --certificate-authority should also override TLS skip settings and CA data, otherwise existing CA data will take precedence. 
        
           caLen := len(config.overrides.ClusterInfo.CertificateAuthority) 
        
           caDataLen := len(config.overrides.ClusterInfo.CertificateAuthorityData) 
        
           if config.overrides.ClusterInfo.InsecureSkipTLSVerify || caLen > 0 || caDataLen > 0 { 
        
           	mergedClusterInfo.InsecureSkipTLSVerify = config.overrides.ClusterInfo.InsecureSkipTLSVerify 
        
           	mergedClusterInfo.CertificateAuthority = config.overrides.ClusterInfo.CertificateAuthority 
        
           	mergedClusterInfo.CertificateAuthorityData = config.overrides.ClusterInfo.CertificateAuthorityData 
        
           }

k8s-ci-robot · 2019-10-29T08:15:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: surki
To complete the pull request process, please assign smarterclayton
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

surki · 2019-10-29T09:43:05Z

/test pull-kubernetes-integration

surki · 2019-10-29T10:17:38Z

Thought of a couple more issues. You can override the server name on the CLI with --server. For parity, this should also be overrideable.

@liggitt Done

surki · 2019-11-11T14:25:25Z

@liggitt Can you PTAL?

liggitt · 2019-11-13T21:10:04Z

@liggitt Can you PTAL?

this will slip to 1.18. I can take a look after next week.

surki · 2019-12-17T15:57:07Z

@liggitt can you PTAL?

liggitt · 2019-12-17T16:44:03Z

staging/src/k8s.io/client-go/tools/clientcmd/client_config.go

 		mergedClusterInfo.CertificateAuthorityData = config.overrides.ClusterInfo.CertificateAuthorityData
 	}

+	if config.overrides.ClusterInfo.TLSServerName != "" {


Suggested change

if config.overrides.ClusterInfo.TLSServerName != "" {

if config.overrides.ClusterInfo.TLSServerName != "" || config.overrides.ClusterInfo.Server != "" {

since tls server name defaults to server, I would expect setting Server and TLSServerName in the overrides to be paired. for example, if I have a kubeconfig that has this:

tlsServerName: https://example.com server: https://1.2.3.4

and I override --server https://foo.com, I would expect foo.com to be used to verify the server unless I also override --tls-server-name https://example.com

liggitt · 2019-12-17T16:46:21Z

staging/src/k8s.io/kubectl/pkg/cmd/config/create_cluster.go

@@ -121,6 +125,9 @@ func (o *createClusterOptions) modifyCluster(existingCluster clientcmdapi.Cluste
 	if o.server.Provided() {
 		modifiedCluster.Server = o.server.Value()


should specifying a server name clear TLSServerName, similar to how insecureSkipTLSVerify and CA interact?

smourapina · 2020-02-06T18:33:26Z

Hello @liggitt, @surki!
Bug Triage team here for the 1.18 release. This is a friendly reminder that code freeze is scheduled for 5 March. Is this issue still intended for milestone 1.18? Thanks in advance!

smourapina · 2020-02-25T18:00:16Z

Hello again @liggitt!
Just leaving another friendly ping to remind you that code freeze is coming in about 1 week (5 March). Should this be moved to the next milestone or will it still be finished by then?

smourapina · 2020-03-02T05:46:20Z

@liggitt, @surki: Bug Triage for 1.18 checking in. We are a few days away from code freeze for release 1.18 (which happens next Thursday, 5 March). As this PR has not been updated recently, should the milestone be adjusted? Thanks!

liggitt · 2020-03-04T04:05:08Z

closing in favor of #88769 which resolved the outstanding comments in a second commit

/close

k8s-ci-robot · 2020-03-04T04:05:11Z

@liggitt: Closed this PR.

Details

In response to this:

closing in favor of #88769 which resolved the outstanding comments in a second commit

/close

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.

k8s-ci-robot requested review from dshulyak and yliaog August 28, 2019 05:32

k8s-ci-robot added area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 28, 2019

surki mentioned this pull request Aug 28, 2019

Support TLS Server Name overrides in kubeconfig file #80596

Closed

k8s-ci-robot requested review from lavalamp and liggitt August 29, 2019 20:07

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Aug 29, 2019

liggitt reviewed Aug 29, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/clientcmd/overrides.go Outdated Show resolved Hide resolved

staging/src/k8s.io/client-go/tools/clientcmd/api/types.go Outdated Show resolved Hide resolved

lavalamp reviewed Aug 29, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/clientcmd/api/types.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned smarterclayton Sep 11, 2019

lavalamp reviewed Sep 11, 2019

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/config/create_cluster.go Outdated Show resolved Hide resolved

lavalamp reviewed Sep 11, 2019

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/config/create_cluster.go Outdated Show resolved Hide resolved

lavalamp reviewed Sep 11, 2019

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/config/create_cluster.go Outdated Show resolved Hide resolved

lavalamp reviewed Sep 11, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/clientcmd/client_config.go Outdated Show resolved Hide resolved

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 25, 2019

surki added 3 commits October 8, 2019 17:23

Support TLS Server Name overrides in kubeconfig file

fc2f5ce

Signed-off-by: Suresh Kumar Ponnusamy <[email protected]>

Rename option to "tls-server-name"

887b99f

And update RecommendedClusterOverrideFlags/BindClusterFlags

Fix help text

00e5e81

surki force-pushed the server-name-override-kubeconfig branch from 467069a to 00e5e81 Compare October 8, 2019 11:54

Fix usage information

fc8a1bb

liggitt reviewed Oct 9, 2019

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/config/create_cluster.go Outdated Show resolved Hide resolved

Add kubectl options for tls servername override

1570dba

Fix typo

5332752

liggitt added this to the v1.18 milestone Nov 13, 2019

liggitt reviewed Dec 17, 2019

View reviewed changes

deads2k mentioned this pull request Mar 3, 2020

Support TLS Server Name overrides in kubeconfig file #88769

Merged

k8s-ci-robot closed this Mar 4, 2020

	if config.overrides.ClusterInfo.TLSServerName != "" {
	if config.overrides.ClusterInfo.TLSServerName != "" \|\| config.overrides.ClusterInfo.Server != "" {

		@@ -121,6 +125,9 @@ func (o *createClusterOptions) modifyCluster(existingCluster clientcmdapi.Cluste
		if o.server.Provided() {
		modifiedCluster.Server = o.server.Value()

Conversation

surki commented Aug 28, 2019

Uh oh!

k8s-ci-robot commented Aug 28, 2019

Uh oh!

k8s-ci-robot commented Aug 28, 2019

Uh oh!

roycaihw commented Aug 29, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

surki commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

surki commented Sep 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liggitt commented Sep 25, 2019

Uh oh!

liggitt commented Sep 25, 2019

Uh oh!

surki commented Oct 8, 2019

Uh oh!

surki commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

liggitt commented Oct 9, 2019

Uh oh!

k8s-ci-robot commented Oct 29, 2019

Uh oh!

surki commented Oct 29, 2019

Uh oh!

surki commented Oct 29, 2019

Uh oh!

surki commented Nov 11, 2019

Uh oh!

liggitt commented Nov 13, 2019

Uh oh!

surki commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

smourapina commented Feb 6, 2020

Uh oh!

smourapina commented Feb 25, 2020

Uh oh!

smourapina commented Mar 2, 2020

Uh oh!

liggitt commented Mar 4, 2020

Uh oh!

k8s-ci-robot commented Mar 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

surki commented Sep 4, 2019 •

edited

Loading

surki commented Oct 9, 2019 •

edited

Loading

surki commented Dec 17, 2019 •

edited

Loading

liggitt Dec 17, 2019 •

edited

Loading