kubectl create: enforce namespace during dry run #84343

busser · 2019-10-25T09:02:35Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

When creating a role with kubectl create role, any specified namespace is ignored if the command is a dry run. This is unexpected behavior.

For example, the following command yields the following output:

kubectl create role foo --verb=get --resource=pod --namespace=bar --output=yaml --dry-run

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  creationTimestamp: null
  name: foo
rules:
- apiGroups:
  - ""
  resources:
  - pods
  verbs:
  - get

This pull-request fixes this issue. The command above now provides the expected output:

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  creationTimestamp: null
  name: foo
  namespace: bar
rules:
- apiGroups:
  - ""
  resources:
  - pods
  verbs:
  - get

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#705

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The output of a dry run of creating a resource with kubectl now always takes into account the namespace specified in the command.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2019-10-25T09:02:36Z

Welcome @busser!

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-10-25T09:02:43Z

Hi @busser. 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.

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.

soltysh · 2019-10-25T13:23:47Z

/ok-to-test

soltysh

Although kubernetes/kubectl#705 uses create role as an example this problem applies to majority (if not all) of create commands. Can you please check all of them? Also don't forget to add tests ensuring that when run with --dry-run the namespace is properly set.

staging/src/k8s.io/kubectl/pkg/cmd/create/create_role.go

busser · 2019-10-25T14:03:36Z

/hold

Putting this PR on hold while I fix the other kubectl create commands and add tests.

soltysh · 2019-11-07T09:44:25Z

/priority backlog

k8s-ci-robot · 2019-11-11T13:05:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

When the TestConfigFlags.WithNamespace method is called, the ClientConfig is wrapped inside a namespacedClientConfig. Previously, calling the namespaceClientConfig.Namespace method would return that the namespace should not be enforced. This made testing namespace enforcement complicated. From now on, the method will return that the namespace should be enforced.

A change in the cmdtesting.TestFactory broke the TestSetLocalNamespace unit test. Calling the TestFactory.WithNamespace method now enforces the "test" namespace, which conflicts with how this test works. This unit tests works by loading a resource from a file with its namespace set to "existing-ns", which was overwritten by "test". This fix corrects this issue.

The create.RunCreateRole function now enforces namespace during dry runs. The TestCreateRole unit test has been updated to reflect that.

TestCreateClusterRole now runs as multiple subtests and uses the diff package to print differences between the expected and actual cluster roles.

If the namespace is to be enforced, make sure to always enforce it, even during dry runs.

TestCreateCronJob now uses the diff package to print differences between the expected and actual cron jobs.

The tests were renamed because there could not be a TestComplete function for both roles and cron jobs, for example. Same for TestValidate.

If the namespace is to be enforced, make sure to always enforce it, even during dry runs.

The TestCreateRoleValidation test was replaced by TestValidateCreateRole to match how other `kubectl create` commands are tested.

busser · 2019-11-14T17:08:07Z

/test pull-kubernetes-node-e2e-containerd

busser · 2019-11-15T15:06:49Z

/test pull-kubernetes-node-e2e-containerd

busser · 2019-11-15T15:34:50Z

The pull-kubernetes-node-e2e-containerd test keeps failing. I am getting the same error messages as in issue #85040.

busser · 2019-11-15T15:42:45Z

/unhold

All kubectl create sub-commands have been checked and, when needed, fixed.

busser · 2019-11-18T12:49:11Z

/test pull-kubernetes-node-e2e-containerd

soltysh

Also please squash your changes into a single commit.

soltysh · 2019-11-27T10:22:07Z

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

@@ -96,7 +96,7 @@ type namespacedClientConfig struct {
 }

 func (c *namespacedClientConfig) Namespace() (string, bool, error) {
-	return c.namespace, false, nil
+	return c.namespace, true, nil


I thought we agreed this to be:

return c.namespace, len(c.namespace) > 0, nil

didn't we?

You're right.

soltysh · 2019-11-27T10:29:22Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_cronjob.go

 	return nil
 }

 func (o *CreateCronJobOptions) Run() error {
 	var cronjob *batchv1beta1.CronJob
 	cronjob = o.createCronJob()
+	if o.EnforceNamespace {
+		cronjob.Namespace = o.Namespace
+		cronjob.Spec.JobTemplate.Namespace = o.Namespace


Not needed, namespace for the created job will be always inherited from the cronjob itself.

soltysh · 2019-11-27T10:29:47Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_cronjob.go

@@ -149,18 +150,28 @@ func (o *CreateCronJobOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, a
 }

 func (o *CreateCronJobOptions) Validate() error {
+	if o.Name == "" {


len(o.Name) == 0 is preferred, see below.

soltysh · 2019-11-27T10:36:09Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_cronjob_test.go

+	}
+}
+
+func TestRunCreateCronJob(t *testing.T) {


This is mostly duplicating TestCreateCronJob, if you look at what Run in create_cronjob.go does it's only creating the CJ itself and I don't think we need that much of testing of the printing and all the rest of the machinery. This test has more setup code than actual code, and one of the reasons we've went with *Options struct was to get rid of those additional setup steps. The high level testing should happen in test-cmd, instead.

soltysh · 2019-11-27T10:37:11Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_cronjob_test.go

+	}
+}
+
+func TestCompleteCreateCronJob(t *testing.T) {


The same argument as below, I don't think we need to test every single line of create commands, Complete method is one of those which can be easily tested in test-cmd.

soltysh · 2019-11-27T10:38:42Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_clusterrole_test.go

+			if test.resourceNames != "" {
+				cmd.Flags().Set("resource-name", test.resourceNames)
+			}
+			cmd.Run(cmd, []string{clusterRoleName})


I'd prefer we move with the testing towards the pattern from create cronjob, where you directly inject values into *Options struct and focus testing on the "raw meat", which in here is RunCreateRole.

The less of the test factory magic, the easier this testing is for newcomers to understand it.

soltysh · 2019-11-27T10:45:42Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_role_test.go

@@ -22,7 +22,7 @@ import (

 	rbac "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
-	"k8s.io/apimachinery/pkg/apis/meta/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"


metav1 is the proffered alias for this, since you've touched this.

fejta-bot · 2020-04-07T17:23:25Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-ci-robot · 2020-04-07T17:23:32Z

@busser: PR needs rebase.

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.

zhouya0 · 2020-04-26T05:56:55Z

@busser Seems no updates from this PR? Could I take this bug and send my PR?

busser · 2020-05-23T10:24:01Z

@busser Seems no updates from this PR? Could I take this bug and send my PR?

Sorry, I haven't taken the time to finish this. Feel free to submit your solution :)

zhouya0 · 2020-05-24T03:36:09Z

@busser Seems no updates from this PR? Could I take this bug and send my PR?

Sorry, I haven't taken the time to finish this. Feel free to submit your solution :)

Yes, it's already been fixed and we can close this :).
/close

k8s-ci-robot · 2020-05-24T03:36:22Z

@zhouya0: Closed this PR.

In response to this:

@busser Seems no updates from this PR? Could I take this bug and send my PR?

Sorry, I haven't taken the time to finish this. Feel free to submit your solution :)

Yes, it's already been fixed and we can close this :).
/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 dixudx and smarterclayton October 25, 2019 09:04

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 25, 2019

soltysh requested changes Oct 25, 2019

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/create/create_role.go Show resolved Hide resolved

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 25, 2019

busser changed the title ~~kubectl create role: add namespace during dry run~~ kubectl create: enforce namespace during dry run Oct 30, 2019

busser requested a review from soltysh October 30, 2019 19:32

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 7, 2019

busser added 14 commits November 14, 2019 12:20

kubectl/cmd/create: add TestRunCreateRole unit tests

c19c0c5

kubectl create role: add namespace to test

4e24530

The create.RunCreateRole function now enforces namespace during dry runs. The TestCreateRole unit test has been updated to reflect that.

kubectl create clusterrole: refactor unit test

b0a456f

TestCreateClusterRole now runs as multiple subtests and uses the diff package to print differences between the expected and actual cluster roles.

kubectl create cronjob: add namespace during dry run

71decc0

If the namespace is to be enforced, make sure to always enforce it, even during dry runs.

kubectl create cronjob: refactor unit test

c9610e1

TestCreateCronJob now uses the diff package to print differences between the expected and actual cron jobs.

kubectl create role: rename unit tests

62c011c

The tests were renamed because there could not be a TestComplete function for both roles and cron jobs, for example. Same for TestValidate.

kubectl create cronjob: add unit tests

f4baba2

kubectl create job: add namespace during dry run

2c55008

If the namespace is to be enforced, make sure to always enforce it, even during dry runs.

kubectl create cronjob: remove unused TestFactory

e5879a0

kubectl create cronjob: fix unit test error message

1d3ceaa

kubectl create job: add unit test

66aab43

kubectl create job: remove unit test

b1f5b23

The TestCreateRoleValidation test was replaced by TestValidateCreateRole to match how other `kubectl create` commands are tested.

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2019

soltysh requested changes Nov 27, 2019

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 7, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2020

k8s-ci-robot closed this May 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubectl create: enforce namespace during dry run #84343

kubectl create: enforce namespace during dry run #84343

busser commented Oct 25, 2019 •

edited

k8s-ci-robot commented Oct 25, 2019

k8s-ci-robot commented Oct 25, 2019

soltysh commented Oct 25, 2019

soltysh left a comment

busser commented Oct 25, 2019

soltysh commented Nov 7, 2019

k8s-ci-robot commented Nov 11, 2019

busser commented Nov 14, 2019

busser commented Nov 15, 2019

busser commented Nov 15, 2019

busser commented Nov 15, 2019

busser commented Nov 18, 2019

soltysh left a comment

soltysh Nov 27, 2019

busser Jan 8, 2020

soltysh Nov 27, 2019

soltysh Nov 27, 2019

soltysh Nov 27, 2019

soltysh Nov 27, 2019

soltysh Nov 27, 2019

soltysh Nov 27, 2019

soltysh Nov 27, 2019

fejta-bot commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

zhouya0 commented Apr 26, 2020

busser commented May 23, 2020

zhouya0 commented May 24, 2020

k8s-ci-robot commented May 24, 2020

kubectl create: enforce namespace during dry run #84343

kubectl create: enforce namespace during dry run #84343

Conversation

busser commented Oct 25, 2019 • edited

k8s-ci-robot commented Oct 25, 2019

k8s-ci-robot commented Oct 25, 2019

soltysh commented Oct 25, 2019

soltysh left a comment

Choose a reason for hiding this comment

busser commented Oct 25, 2019

soltysh commented Nov 7, 2019

k8s-ci-robot commented Nov 11, 2019

busser commented Nov 14, 2019

busser commented Nov 15, 2019

busser commented Nov 15, 2019

busser commented Nov 15, 2019

busser commented Nov 18, 2019

soltysh left a 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

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

fejta-bot commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

zhouya0 commented Apr 26, 2020

busser commented May 23, 2020

zhouya0 commented May 24, 2020

k8s-ci-robot commented May 24, 2020

busser commented Oct 25, 2019 •

edited