Validate if service has duplicate port #47336

xiangpengzhao · 2017-06-12T11:41:05Z

What this PR does / why we need it:
Validate if a service has duplicate Spec.Ports.Port (same number with same protocol)

xref #47221
fixes this part:

It is possible to express a Service with multiple ports blocks with the same number. This is not very useful and may cause trouble for implementations of Services.

Special notes for your reviewer:
/cc @thockin @liggitt @mengqiy
@kubernetes/sig-network-pr-reviews

Release note:

NONE

k8s-ci-robot · 2017-06-12T11:41:14Z

@xiangpengzhao: Reiterating the mentions to trigger a notification:
@kubernetes/sig-network-pr-reviews.

In response to this:

What this PR does / why we need it:
Validate if a service has duplicate Spec.Ports.Port (same number with same protocol)

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): partially fixes #47221
fixes this part:

It is possible to express a Service with multiple ports blocks with the same number. This is not very useful and may cause trouble for implementations of Services.

Special notes for your reviewer:
/cc @thockin @liggitt @mengqiy
@kubernetes/sig-network-pr-reviews

Release note:
NONE

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 · 2017-06-12T11:41:22Z

Hi @xiangpengzhao. 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 @k8s-bot 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.

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. I understand the commands that are listed here.

pmichali

Looks good overall. Just wondering if additional coverage should be added.

pmichali · 2017-06-30T14:58:43Z

pkg/api/validation/validation_test.go

+			},
+			numErrs: 0,
+		},
+		{


Should you have a test case with port==0 to cover that case?

will remove the port==0 case as per @mengqiy comment.

mengqiy · 2017-06-30T16:59:46Z

pkg/api/validation/validation.go

+		key := api.ServicePort{Protocol: port.Protocol, Port: port.Port}
+		_, found := ports[key]
+		if found {
+			allErrs = append(allErrs, field.Duplicate(portPath.Child("port"), port.Port))


This error message may be misleading, since it only talks about port. We should include protocol as well.

Good point! Will fix.

I'm not sure two errors make sense either... a single error that included both the protocol and the port number is what I would expect - field.Duplicate(portPath, key)

mengqiy · 2017-06-30T17:00:16Z

pkg/api/validation/validation.go

+	portsPath = specPath.Child("ports")
+	ports := make(map[api.ServicePort]bool)
+	for i, port := range service.Spec.Ports {
+		if port.Port == 0 {


why we treat port 0 differently?

I thought port can be unspecified. That's incorrect. Thanks for pointing this out!

xiangpengzhao · 2017-07-04T01:49:39Z

inline comments addressed. PTAL.

mengqiy · 2017-07-05T22:38:03Z

/ok-to-test

xiangpengzhao · 2017-07-25T02:59:40Z

@ping @liggitt and @derekwaynecarr for reviewing
and
/assign @thockin
for approval

thanks!

xiangpengzhao · 2017-08-01T05:49:35Z

Thanks @liggitt ! Inline comment addressed. PTAL.

thockin · 2017-08-01T15:42:58Z

/lgtm
/approve

k8s-github-robot · 2017-08-01T15:43:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, xiangpengzhao

Associated issue: 47221

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/api/OWNERS~~ [thockin]

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

xiangpengzhao · 2017-08-01T15:52:44Z

/retest

k8s-github-robot · 2017-08-01T21:31:02Z

Automatic merge from submit-queue

k8s-ci-robot requested review from liggitt, mengqiy and thockin June 12, 2017 11:41

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/network Categorizes an issue or PR as relevant to SIG Network. labels Jun 12, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 12, 2017

k8s-github-robot assigned liggitt and derekwaynecarr Jun 12, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 12, 2017

xiangpengzhao force-pushed the fix-dup-port branch 2 times, most recently from 7b009e5 to 519ac02 Compare June 13, 2017 01:06

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2017

pmichali suggested changes Jun 30, 2017

View reviewed changes

mengqiy reviewed Jun 30, 2017

View reviewed changes

xiangpengzhao force-pushed the fix-dup-port branch from 519ac02 to 36bd862 Compare July 3, 2017 03:07

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2017

xiangpengzhao force-pushed the fix-dup-port branch from 36bd862 to feb68d3 Compare July 4, 2017 01:48

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 5, 2017

xiangpengzhao force-pushed the fix-dup-port branch from feb68d3 to f56bf5a Compare July 6, 2017 01:14

k8s-ci-robot assigned thockin Jul 25, 2017

xiangpengzhao force-pushed the fix-dup-port branch from f56bf5a to 354394c Compare August 1, 2017 05:47

Validate if service has duplicate port

3f28074

xiangpengzhao force-pushed the fix-dup-port branch from 354394c to 3f28074 Compare August 1, 2017 07:03

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2017

k8s-github-robot merged commit 7be37ef into kubernetes:master Aug 1, 2017

xiangpengzhao deleted the fix-dup-port branch August 1, 2017 23:27

xiangpengzhao mentioned this pull request Aug 28, 2017

Services allow duplicate port #47221

Closed

tmszdmsk mentioned this pull request Jun 2, 2020

kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477

Open

Validate if service has duplicate port #47336

Validate if service has duplicate port #47336

Uh oh!

Conversation

xiangpengzhao commented Jun 12, 2017 • edited by thockin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 12, 2017

Uh oh!

k8s-ci-robot commented Jun 12, 2017

Uh oh!

pmichali left a comment

Choose a reason for hiding this comment

Uh oh!

pmichali Jun 30, 2017

Choose a reason for hiding this comment

Uh oh!

xiangpengzhao Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

mengqiy Jun 30, 2017

Choose a reason for hiding this comment

Uh oh!

xiangpengzhao Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

liggitt Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

mengqiy Jun 30, 2017

Choose a reason for hiding this comment

Uh oh!

xiangpengzhao Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

xiangpengzhao commented Jul 4, 2017

Uh oh!

mengqiy commented Jul 5, 2017

Uh oh!

xiangpengzhao commented Jul 25, 2017

Uh oh!

xiangpengzhao commented Aug 1, 2017

Uh oh!

thockin commented Aug 1, 2017

Uh oh!

k8s-github-robot commented Aug 1, 2017

Uh oh!

xiangpengzhao commented Aug 1, 2017

Uh oh!

k8s-github-robot commented Aug 1, 2017

Uh oh!

Uh oh!

xiangpengzhao commented Jun 12, 2017 •

edited by thockin

Loading