Namespace has no effect when exposing deployment with --dry-run=client #97492

masap · 2020-12-24T01:40:26Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This is how to reproduce.

$ kubectl create ns testns
$ kubectl -n testns create deployment test-deploy --image=nginx
$ kubectl -n testns expose deployment.apps/test-deploy --port=80 --dry-run=client -o yaml
apiVersion: v1
kind: Service
metadata:
  creationTimestamp: null
  labels:
    app: test-deploy
  name: test-deploy
spec:
  ports:
  - port: 80
    protocol: TCP
    targetPort: 80
  selector:
    app: test-deploy
status:
  loadBalancer: {}

If --dry-run=client is not specified, namespace element is contained in the yaml.

Which issue(s) this PR fixes:
NONE

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixed no effect namespace when exposing deployment with --dry-run=client.

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

NONE

This is how to reproduce. $ kubectl create ns testns $ kubectl -n testns create deployment test-deploy --image=nginx $ kubectl -n testns expose deployment.apps/test-deploy --port=80 --dry-run=client -o yaml apiVersion: v1 kind: Service metadata: creationTimestamp: null labels: app: test-deploy name: test-deploy spec: ports: - port: 80 protocol: TCP targetPort: 80 selector: app: test-deploy status: loadBalancer: {} If --dry-run=client is not specified, namespace element is contained in the yaml.

k8s-ci-robot · 2020-12-24T01:40:34Z

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

knight42 · 2020-12-24T13:22:51Z

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose_test.go

+					Type:     corev1.ServiceTypeLoadBalancer,
+				},
+			},
+			expected: "namespace: testns",


why is this the expected output?

Though exact result is as follows, this code checks only namespace because we are only interested in it.

apiVersion: v1 kind: Service metadata: creationTimestamp: null labels: svc: test name: foo namespace: testns spec: ports: - port: 14 protocol: UDP targetPort: 14 selector: func: stream type: LoadBalancer status: loadBalancer: {}

@knight42 Is there any comment ?

knight42

Nice catch!

/ok-to-test

knight42 · 2020-12-24T13:24:27Z

/triage accepted

cblecker · 2021-01-05T20:30:34Z

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

+			if meta, err := meta.Accessor(object); err == nil && o.EnforceNamespace {
+				meta.SetNamespace(o.Namespace)
+			}


@soltysh @deads2k Is it preferable to take this route, and manipulate the generated object before printing.. or would it be better to, in the generator, add the namespace to the generated object?

That code seems to be used in the run command as well.

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/run/run.go

Lines 677 to 679 in 8b24d1f

if meta, err := meta.Accessor(actualObj); err == nil && o.EnforceNamespace {

meta.SetNamespace(o.Namespace)

}

This is ok temporary solution, we're slowly getting rid of generators which will clear this situation.

knight42 · 2021-01-20T03:41:44Z

/lgtm
Need approval from @kubernetes/sig-cli-maintainers.

masap · 2021-01-20T04:13:20Z

@knight42 Thanks!

masap · 2021-01-20T04:13:25Z

/assign @deads2k

soltysh

/lgtm
/approve

soltysh · 2021-01-20T16:57:11Z

staging/src/k8s.io/kubectl/pkg/cmd/expose/expose.go

+			if meta, err := meta.Accessor(object); err == nil && o.EnforceNamespace {
+				meta.SetNamespace(o.Namespace)
+			}


This is ok temporary solution, we're slowly getting rid of generators which will clear this situation.

k8s-ci-robot · 2021-01-20T16:57:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: masap, soltysh

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:

~~staging/src/k8s.io/kubectl/OWNERS~~ [soltysh]

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

k8s-ci-robot requested review from derekwaynecarr and soltysh December 24, 2020 01:41

knight42 reviewed Dec 24, 2020

View reviewed changes

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 Dec 24, 2020

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 24, 2020

cblecker reviewed Jan 5, 2021

View reviewed changes

k8s-ci-robot assigned knight42 Jan 20, 2021

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

k8s-ci-robot assigned deads2k Jan 20, 2021

soltysh approved these changes Jan 20, 2021

View reviewed changes

k8s-ci-robot assigned soltysh Jan 20, 2021

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

k8s-ci-robot merged commit f02435d into kubernetes:master Jan 20, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 20, 2021

masap deleted the expose1 branch January 20, 2021 23:55

github-actions bot mentioned this pull request Jan 26, 2021

Week Ending January 24, 2021 dev-obs/actus#329

Open

masap mentioned this pull request Feb 11, 2021

REQUEST: New membership for masap kubernetes/org#2498

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Namespace has no effect when exposing deployment with --dry-run=client #97492

Namespace has no effect when exposing deployment with --dry-run=client #97492

masap commented Dec 24, 2020

k8s-ci-robot commented Dec 24, 2020

knight42 Dec 24, 2020

masap Dec 24, 2020

masap Jan 18, 2021

knight42 left a comment

knight42 commented Dec 24, 2020

cblecker Jan 5, 2021

masap Jan 5, 2021

soltysh Jan 20, 2021

knight42 commented Jan 20, 2021

masap commented Jan 20, 2021

masap commented Jan 20, 2021

soltysh left a comment

soltysh Jan 20, 2021

k8s-ci-robot commented Jan 20, 2021

	if meta, err := meta.Accessor(actualObj); err == nil && o.EnforceNamespace {
	meta.SetNamespace(o.Namespace)
	}

Namespace has no effect when exposing deployment with --dry-run=client #97492

Namespace has no effect when exposing deployment with --dry-run=client #97492

Conversation

masap commented Dec 24, 2020

k8s-ci-robot commented Dec 24, 2020

knight42 Dec 24, 2020

Choose a reason for hiding this comment

masap Dec 24, 2020

Choose a reason for hiding this comment

masap Jan 18, 2021

Choose a reason for hiding this comment

knight42 left a comment

Choose a reason for hiding this comment

knight42 commented Dec 24, 2020

cblecker Jan 5, 2021

Choose a reason for hiding this comment

masap Jan 5, 2021

Choose a reason for hiding this comment

soltysh Jan 20, 2021

Choose a reason for hiding this comment

knight42 commented Jan 20, 2021

masap commented Jan 20, 2021

masap commented Jan 20, 2021

soltysh left a comment

Choose a reason for hiding this comment

soltysh Jan 20, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 20, 2021