New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow headless svc without ports to have endpoints #67622

Merged
merged 1 commit into from Aug 21, 2018

Conversation

@thockin
Member

thockin commented Aug 20, 2018

As cited in
kubernetes/dns#174 - this is documented to
work, and I don't see why it shouldn't work. We allowed the definition
of headless services without ports, but apparently nobody tested it very
well.

Manually tested clusterIP services with no ports - validation error.

Manually tested services with negative ports - validation error.

New tests failed, output inspected and verified. Now pass.

xref kubernetes/dns#174

Release note:

Headless Services with no ports defined will now create Endpoints correctly, and appear in DNS.
@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Aug 20, 2018

Member

Holding until CLI changes are made.

Member

thockin commented Aug 20, 2018

Holding until CLI changes are made.

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Aug 20, 2018

Member

CLI changes are in

Member

thockin commented Aug 20, 2018

CLI changes are in

if !more {
hostPort := net.JoinHostPort(addr.IP, strconv.Itoa(int(port.Port)))
list = append(list, hostPort)
if len(ss.Ports) == 0 {

This comment has been minimized.

@rramkumar1

rramkumar1 Aug 21, 2018

Member

Small comment might be helpful here to describe the reason for the if/else or what you are doing differently.

@rramkumar1

rramkumar1 Aug 21, 2018

Member

Small comment might be helpful here to describe the reason for the if/else or what you are doing differently.

This comment has been minimized.

@thockin

thockin Aug 21, 2018

Member

roger

@thockin

thockin Aug 21, 2018

Member

roger

Show outdated Hide outdated pkg/controller/endpoint/endpoints_controller_test.go
}
}
subsets = endpoints.RepackSubsets(subsets)

This comment has been minimized.

@rramkumar1

rramkumar1 Aug 21, 2018

Member

Previously, RepackSubsets() was being called for each pod. Did you move it after the loop to ensure the repacking is only done once?

@rramkumar1

rramkumar1 Aug 21, 2018

Member

Previously, RepackSubsets() was being called for each pod. Did you move it after the loop to ensure the repacking is only done once?

This comment has been minimized.

@thockin

thockin Aug 21, 2018

Member

Yeah, it really only needs to be done once. That was a bug :)

@thockin

thockin Aug 21, 2018

Member

Yeah, it really only needs to be done once. That was a bug :)

Allow headless svc without ports to have endpoints
As cited in
kubernetes/dns#174 - this is documented to
work, and I don't see why it shouldn't work.  We allowed the definition
of headless services without ports, but apparently nobody tested it very
well.

Manually tested clusterIP services with no ports - validation error.

Manually tested services with negative ports - validation error.

New tests failed, output inspected and verified.  Now pass.
@thockin

re-pushed with fixes

}
}
subsets = endpoints.RepackSubsets(subsets)

This comment has been minimized.

@thockin

thockin Aug 21, 2018

Member

Yeah, it really only needs to be done once. That was a bug :)

@thockin

thockin Aug 21, 2018

Member

Yeah, it really only needs to be done once. That was a bug :)

Show outdated Hide outdated pkg/controller/endpoint/endpoints_controller_test.go
if !more {
hostPort := net.JoinHostPort(addr.IP, strconv.Itoa(int(port.Port)))
list = append(list, hostPort)
if len(ss.Ports) == 0 {

This comment has been minimized.

@thockin

thockin Aug 21, 2018

Member

roger

@thockin

thockin Aug 21, 2018

Member

roger

@rramkumar1

This comment has been minimized.

Show comment
Hide comment
@rramkumar1

rramkumar1 Aug 21, 2018

Member

/lgtm

Member

rramkumar1 commented Aug 21, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 21, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 21, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

Contributor

k8s-ci-robot commented Aug 21, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 21, 2018

Contributor

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

Contributor

k8s-merge-robot commented Aug 21, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit 1c01372 into kubernetes:master Aug 21, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation thockin authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Aug 24, 2018

Member

/sig api-machinery
for the release note tool.

Member

neolit123 commented Aug 24, 2018

/sig api-machinery
for the release note tool.

mapAddressByPort(&subsets[i].NotReadyAddresses[k], port, false, allAddrs, portToAddrReadyMap)
if len(subsets[i].Ports) == 0 {
// Don't discard endpoints with no ports defined, use a sentinel.
mapAddressesByPort(&subsets[i], api.EndpointPort{Port: -1}, allAddrs, portToAddrReadyMap)

This comment has been minimized.

@krmayankk

krmayankk Aug 29, 2018

Contributor

@thockin since ports for headless service dont make sense, are we at some point also going to throw validation errors ? Its not backward compatible i guess but if there is a way to deprecate and eventually not allow that, it will be helpful

@krmayankk

krmayankk Aug 29, 2018

Contributor

@thockin since ports for headless service dont make sense, are we at some point also going to throw validation errors ? Its not backward compatible i guess but if there is a way to deprecate and eventually not allow that, it will be helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment