fix: kubectl expose fails for apps with same-port, different-protocol #114909

aimuz · 2023-01-09T03:39:40Z

Fixed: #114402
Signed-off-by: aimuz [email protected]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #114402

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Kubectl expose supports the creation of different protocol service on the same port

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

aimuz · 2023-01-09T03:40:25Z

/sig cli

aimuz · 2023-01-09T03:41:48Z

/assign anggao

Can you review the code for me

k8s-ci-robot · 2023-01-09T03:41:49Z

@aimuz: GitHub didn't allow me to assign the following users: anggao.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign anggao

Can you review the code for me

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.

aimuz · 2023-01-09T03:42:45Z

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

+ serviceName := o.Name
+ if len(serviceName) == 0 {
+ serviceName = o.DefaultName
+ if len(serviceName) == 0 {


This is to avoid the misunderstanding of duplicate names. In the loop, name is also used. To avoid misunderstanding, I renamed it

aimuz · 2023-01-09T03:44:07Z

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

+ name := servicePortName
+ if len(portStringSlice) > 1 {
+ name = fmt.Sprintf("%s-%d", strings.ToLower(protocol), port)
+ }


Compared with the naming of prot-i, this method may be easier to understand, which is also to distinguish between different protocols on the same port

aimuz · 2023-01-09T03:45:09Z

/assign @anggao

k8s-ci-robot · 2023-01-09T03:45:10Z

@aimuz: GitHub didn't allow me to assign the following users: anggao.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @anggao

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.

aimuz · 2023-01-09T06:41:57Z

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

@@ -1722,8 +1756,8 @@ func TestGenerateService(t *testing.T) {
 service, err := exposeServiceOptions.createService()
 if test.expectErr == "" {
 require.NoError(t, err)
- if !apiequality.Semantic.DeepEqual(service, test.expected) {
- t.Errorf("\nexpected:\n%#v\ngot:\n%#v", test.expected, service)
+ if diff := cmp.Diff(test.expected, service); diff != "" {


This will output more intuitive differences

aimuz · 2023-01-09T07:58:26Z

/test pull-kubernetes-integration

ardaguclu

Thanks for the fix and the effort you invested.

Maybe it is better to do less changes to fix that issue without touching generated files.

to prevent accidental merging;
/hold

ardaguclu · 2023-01-09T08:17:18Z

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

 }
- selector, err := parseLabels(o.Selector)
+ obj, err := versioned.GenerateService(opts)


This looks unusual. What is the reason of this change?

Repetitive code implementation, which I extract as a public method

See my comment above. We don't want to use the generators.

ardaguclu · 2023-01-09T08:17:56Z

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

- portProtocolMap[portProtocol[0]] = portProtocol[1]
- }
- return portProtocolMap, nil
+ return obj, nil


What is the reason of these changes with respect to the referenced issue?

Repetitive code implementation, which I extract as a public method

Is this something required for fixing the bug or is this refactoring?.

Refactoring was done while fixing the bug

Can you start with just fixing the bug, with minimal or no refactoring at first? Then maybe do the refactor later if needed.

It's more complicated to review a bug fix and refactor together.

ardaguclu · 2023-01-09T08:18:43Z

staging/src/k8s.io/kubectl/pkg/polymorphichelpers/interface.go

@@ -67,7 +67,7 @@ var MapBasedSelectorForObjectFn MapBasedSelectorForObjectFunc = mapBasedSelector

 // ProtocolsForObjectFunc will call the provided function on the protocols for the object,
 // return nil-map if no protocols for the object, or return an error.
-type ProtocolsForObjectFunc func(object runtime.Object) (map[string]string, error)
+type ProtocolsForObjectFunc func(object runtime.Object) (map[string][]string, error)


Is this backward compatible?

No, but I don't think this change will have any impact. Only kubectl will use this

It is public and it looks like people are using it (via github search, but possibly more elsewhere)...
https://github.com/search?q=ProtocolsForObjectFunc&type=code

Can we leave the existing function as-is and create a new one with the new behavior?

Perhaps mark the old one as deprecated, with an explanation that it doesn't support multiple protocols per port, and that code should use the new function instead?

ardaguclu · 2023-01-09T08:22:55Z

As far as I can see, integration test failure is real.

aimuz · 2023-01-09T08:57:01Z

Maybe it is better to do less changes to fix that issue without touching generated files.

Yes, the best way is to complete the bug repair with the least code

For example, I can roll back the changes to the portname, which will reduce the number of changes, but this will result in inconsistent portname naming.

For example, for public method extraction, I can roll back this part and only modify the required part, but it will be difficult to maintain. For the same logic code, modify and maintain multiple copies.

@ardaguclu If you need to do so, please let me know at any time, and it will be rolled back

k8s-triage-robot · 2023-04-26T03:34:45Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

aimuz · 2023-04-26T06:00:10Z

/remove-lifecycle stale

aimuz · 2023-06-12T05:25:46Z

/pin @brianpursley

aimuz · 2023-06-14T13:18:08Z

/ping @brianpursley @ardaguclu

ardaguclu · 2023-06-14T17:26:23Z

staging/src/k8s.io/kubectl/pkg/polymorphichelpers/multiprotocolsforobject_test.go

+ expected: map[string][]string{"101": {"TCP"}},
+ },
+ {
+ name: "unsupported object",


Does portsforobject also return expectErr true?

yes

This is the code for portsforobject

func portsForObject(object runtime.Object) ([]string, error) { switch t := object.(type) { case *corev1.ReplicationController: return getPorts(t.Spec.Template.Spec), nil case *corev1.Pod: return getPorts(t.Spec), nil case *corev1.Service: return getServicePorts(t.Spec), nil case *extensionsv1beta1.Deployment: return getPorts(t.Spec.Template.Spec), nil case *appsv1.Deployment: return getPorts(t.Spec.Template.Spec), nil case *appsv1beta2.Deployment: return getPorts(t.Spec.Template.Spec), nil case *appsv1beta1.Deployment: return getPorts(t.Spec.Template.Spec), nil case *extensionsv1beta1.ReplicaSet: return getPorts(t.Spec.Template.Spec), nil case *appsv1.ReplicaSet: return getPorts(t.Spec.Template.Spec), nil case *appsv1beta2.ReplicaSet: return getPorts(t.Spec.Template.Spec), nil default: return nil, fmt.Errorf("cannot extract ports from %T", object) } }

ardaguclu · 2023-06-14T17:32:52Z

staging/src/k8s.io/kubectl/pkg/polymorphichelpers/multiprotocolsforobject.go

+ for _, port := range container.Ports {
+ // Empty protocol must be defaulted (TCP)
+ if len(port.Protocol) == 0 {
+ port.Protocol = corev1.ProtocolTCP


Although port.Protocol is value type(it looks safe), I think, we should avoid such assignments to objects inside corev1.PodSpec.
Instead, I'd rather prefer;

protocol := corev1.ProtocolTCP if len(port.Protocol) > 0 { protocol = port.Protocol }

@ardaguclu done

ardaguclu · 2023-06-14T17:33:07Z

staging/src/k8s.io/kubectl/pkg/polymorphichelpers/multiprotocolsforobject.go

+ for _, servicePort := range spec.Ports {
+ // Empty protocol must be defaulted (TCP)
+ if len(servicePort.Protocol) == 0 {
+ servicePort.Protocol = corev1.ProtocolTCP


Fixed: kubernetes#114402 Signed-off-by: aimuz <[email protected]>

ardaguclu · 2023-07-03T06:18:16Z

Thanks @aimuz

/lgtm
/triage accepted
/priority backlog

I'm deferring to @brianpursley for last approval, if he wants to check.

k8s-ci-robot · 2023-07-03T06:18:23Z

LGTM label has been added.

Git tree hash: c399785bd6ad0c6770a0bab6a2abd7a85a4f843f

aimuz · 2023-07-03T10:10:41Z

/pin @brianpursley

Do you have time for a code review, thank you

aojea · 2023-07-06T09:08:50Z

great 👏

ardaguclu · 2023-07-06T09:25:16Z

/hold cancel
/approve

k8s-ci-robot · 2023-07-06T09:25:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aimuz, ardaguclu

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~~ [ardaguclu]

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

aimuz · 2023-07-06T10:33:19Z

Thank you all for your hard work!

aimuz · 2023-07-07T05:51:46Z

Does this need to be Cherry-picked to another branch?

@ardaguclu @aojea

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. area/kubectl and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 9, 2023

k8s-ci-robot requested review from ardaguclu and brianpursley January 9, 2023 03:40

aimuz commented Jan 9, 2023

View reviewed changes

aimuz force-pushed the fix-114402 branch from 9d0e0a9 to 3730698 Compare January 9, 2023 06:28

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 9, 2023

aimuz force-pushed the fix-114402 branch 2 times, most recently from f6b61cc to 4cabc1e Compare January 9, 2023 06:33

aimuz commented Jan 9, 2023

View reviewed changes

ardaguclu reviewed Jan 9, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2023

aimuz force-pushed the fix-114402 branch from 4cabc1e to d7a4375 Compare January 9, 2023 09:23

aimuz requested a review from ardaguclu January 26, 2023 03:29

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

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

ardaguclu reviewed Jun 14, 2023

View reviewed changes

fix: kubectl expose fails for apps with same-port, different-protocol

e5116a3

Fixed: kubernetes#114402 Signed-off-by: aimuz <[email protected]>

aimuz force-pushed the fix-114402 branch from 564d988 to e5116a3 Compare June 19, 2023 01:59

aimuz requested review from ardaguclu and brianpursley June 29, 2023 07:33

k8s-ci-robot assigned ardaguclu Jul 3, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2023

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2023

k8s-ci-robot merged commit 2075b20 into kubernetes:master Jul 6, 2023
12 checks passed

k8s-ci-robot added this to the v1.28 milestone Jul 6, 2023

fix: kubectl expose fails for apps with same-port, different-protocol #114909

fix: kubectl expose fails for apps with same-port, different-protocol #114909

Conversation

aimuz commented Jan 9, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

aimuz commented Jan 9, 2023

aimuz commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aimuz commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

Choose a reason for hiding this comment

aimuz commented Jan 9, 2023

ardaguclu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardaguclu commented Jan 9, 2023

aimuz commented Jan 9, 2023 • edited

k8s-triage-robot commented Apr 26, 2023

aimuz commented Apr 26, 2023

aimuz commented Jun 12, 2023

aimuz commented Jun 14, 2023

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

ardaguclu commented Jul 3, 2023

k8s-ci-robot commented Jul 3, 2023

aimuz commented Jul 3, 2023

aojea commented Jul 6, 2023

ardaguclu commented Jul 6, 2023

k8s-ci-robot commented Jul 6, 2023

aimuz commented Jul 6, 2023

aimuz commented Jul 7, 2023

aimuz commented Jan 9, 2023 •

edited