forbid unnamed context #56769

dixudx · 2017-12-04T06:42:12Z

What this PR does / why we need it:
forbid unnamed contexts with validation

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #56747

Special notes for your reviewer:
/assign @sttts @fabianofranz

Release note:

Forbid unnamed contexts in kubeconfigs.

sttts · 2017-12-12T08:56:56Z

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

@@ -167,6 +168,9 @@ func addConversionFuncs(scheme *runtime.Scheme) error {
 				if err := s.Convert(&curr.Context, newContext, 0); err != nil {
 					return err
 				}
+				if len(curr.Name) == 0 {
+					return fmt.Errorf("you must specify a non-empty context name for %#v", curr)


non-empty context names are not allowed. "you" sounds strange here.

Make sense. Updated. PTAL.

sttts · 2017-12-12T08:57:09Z

/assign liggitt

liggitt · 2017-12-12T14:35:07Z

pkg/kubectl/cmd/config/use_context.go

@@ -88,7 +88,7 @@ func (o *useContextOptions) complete(cmd *cobra.Command) error {

 func (o useContextOptions) validate(config *clientcmdapi.Config) error {
 	if len(o.contextName) == 0 {
-		return errors.New("you must specify a current-context")
+		return errors.New("non-empty context names are not allowed")


this message text is very confusing

liggitt · 2017-12-12T14:37:05Z

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

@@ -167,6 +168,9 @@ func addConversionFuncs(scheme *runtime.Scheme) error {
 				if err := s.Convert(&curr.Context, newContext, 0); err != nil {
 					return err
 				}
+				if len(curr.Name) == 0 {


conversion isn't the right place to do validation like this... validate at point of use

For such conversionFuncs registered/initialized at init, it is quite hard to do the validate at point of use. Will looks quite ugly if added IMO.

Errors in conversion do not surface well. This data should not prevent conversion.

Validate here: https://github.com/dixudx/kubernetes/blob/20da5a0b5f46f10054ea4d3eacaaf760a890cc1b/staging/src/k8s.io/client-go/tools/clientcmd/validation.go

liggitt · 2017-12-12T14:37:21Z

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

@@ -167,6 +168,9 @@ func addConversionFuncs(scheme *runtime.Scheme) error {
 				if err := s.Convert(&curr.Context, newContext, 0); err != nil {
 					return err
 				}
+				if len(curr.Name) == 0 {
+					return fmt.Errorf("non-empty context name for %#v is not allowed", curr)


I think this should be "empty context name is not allowed"

fabianofranz · 2017-12-21T16:57:40Z

/unassign

dixudx · 2018-01-07T03:44:55Z

ping @liggitt for another look. Thanks.

liggitt · 2018-01-07T17:45:25Z

/lgtm

k8s-ci-robot · 2018-01-07T17:45:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, liggitt

Associated issue: #56747

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubectl/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2018-01-07T19:02:36Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-01-07T19:19:44Z

Automatic merge from submit-queue (batch tested with PRs 57521, 56769). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot · 2018-01-07T19:44:33Z

@dixudx: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce	`792a229`	link	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

sttts · 2018-01-08T09:05:06Z

Updated the release note. Ptal.

k8s-ci-robot assigned fabianofranz and sttts Dec 4, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 4, 2017

dixudx force-pushed the forbid_unnamed_context branch from 0f6e4b7 to 4513f43 Compare December 5, 2017 02:09

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 5, 2017

sttts reviewed Dec 12, 2017

View reviewed changes

k8s-ci-robot assigned liggitt Dec 12, 2017

dixudx force-pushed the forbid_unnamed_context branch from 4513f43 to 62e9f34 Compare December 12, 2017 09:02

kubernetes deleted a comment from k8s-github-robot Dec 12, 2017

liggitt reviewed Dec 12, 2017

View reviewed changes

dixudx force-pushed the forbid_unnamed_context branch 2 times, most recently from ea287ac to 20da5a0 Compare December 13, 2017 02:50

k8s-ci-robot unassigned fabianofranz Dec 21, 2017

cblecker mentioned this pull request Dec 22, 2017

kubectl config view --minify=true fails for kubeconfig without current context #56747

Closed

liggitt mentioned this pull request Jan 6, 2018

current context set to "" (no character) #38053

Closed

dixudx force-pushed the forbid_unnamed_context branch 2 times, most recently from 5cd0f90 to af00893 Compare January 7, 2018 02:36

forbid unnamed context

792a229

dixudx force-pushed the forbid_unnamed_context branch from af00893 to 792a229 Compare January 7, 2018 02:52

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 7, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2018

k8s-github-robot merged commit a2bce0d into kubernetes:master Jan 7, 2018

dixudx deleted the forbid_unnamed_context branch January 8, 2018 01:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

forbid unnamed context #56769

forbid unnamed context #56769

dixudx commented Dec 4, 2017 •

edited by sttts

Loading

sttts Dec 12, 2017

dixudx Dec 12, 2017

sttts commented Dec 12, 2017

liggitt Dec 12, 2017

liggitt Dec 12, 2017

dixudx Dec 13, 2017

liggitt Dec 13, 2017

liggitt Jan 6, 2018

liggitt Dec 12, 2017

dixudx Dec 13, 2017

fabianofranz commented Dec 21, 2017

dixudx commented Jan 7, 2018

liggitt commented Jan 7, 2018

k8s-ci-robot commented Jan 7, 2018

k8s-github-robot commented Jan 7, 2018

k8s-github-robot commented Jan 7, 2018

k8s-ci-robot commented Jan 7, 2018 •

edited

Loading

sttts commented Jan 8, 2018

forbid unnamed context #56769

forbid unnamed context #56769

Conversation

dixudx commented Dec 4, 2017 • edited by sttts Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianofranz commented Dec 21, 2017

dixudx commented Jan 7, 2018

liggitt commented Jan 7, 2018

k8s-ci-robot commented Jan 7, 2018

k8s-github-robot commented Jan 7, 2018

k8s-github-robot commented Jan 7, 2018

k8s-ci-robot commented Jan 7, 2018 • edited Loading

sttts commented Jan 8, 2018

dixudx commented Dec 4, 2017 •

edited by sttts

Loading

k8s-ci-robot commented Jan 7, 2018 •

edited

Loading