Add Service type "ExternalName" which results in CNAME DNS #30599

therc · 2016-08-14T23:08:33Z

ExternalName allows kubedns to return CNAME records for external
services. No proxying is involved.

First step for kubernetes/enhancements#33

See original issue at
#13748

No release note yet, that will come with the kubedns change.

NONE

This change is

therc · 2016-08-14T23:09:35Z

I also changed the comments a bit to make the two types.go files use the same wording for the same fields.

therc · 2016-08-14T23:12:50Z

cc @kubernetes/kube-api

therc · 2016-08-15T18:25:12Z

I assume that it'd be a good idea to also have defaulting and validation in. :-) Feel free to reassign to @smarterclayton, @thockin or both, since they are the feature shepherds.

smarterclayton · 2016-08-16T22:18:48Z

pkg/api/validation/validation.go

@@ -100,6 +100,14 @@ func ValidateDNS1123Label(value string, fldPath *field.Path) field.ErrorList {
 	return allErrs
 }

+func ValidateDNS1123Subdomain(value string, fldPath *field.Path) field.ErrorList {


Godoc on this.

smarterclayton · 2016-08-16T22:23:11Z

Given that this is alpha, we generally implement this as an annotation. In this case the type is the new thing, so we should probably call it "AlphaExternalName". The only concern there would be that because this is an enum that we'd potentially have services stored in the db without an enum, which could cause breaks.

thockin · 2016-08-17T01:58:09Z

Review status: 0 of 15 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

pkg/api/types.go, line 1742 [r3] (raw file):

// ServiceSpec describes the attributes that a user creates on a service
type ServiceSpec struct {
  // Type determines how to expose the service. Defaults to ClusterIP and only

This makes it sound like "type" does not apply if clusterIP == "none", but type=ExternalName doesn't need clusterIP ?

pkg/api/v1/defaults.go, line 117 [r3] (raw file):

      obj.Type = ServiceTypeClusterIP
  } else if obj.Type == ServiceTypeExternalName {
      obj.ClusterIP = ClusterIPNone

I don't think you should default this here. It should instead be a validation error if type is externalName but clusterIP != ""

Uggh, but that is in the REST path, isn't it? By the time you get here, an IP has been allocated. If you override this, you will leak. Or did I get the sequence wrong?

Comments from Reviewable

thockin · 2016-08-17T01:59:50Z

yeah, I am nervous about how to extend an enum like this. Can we do i
t as an annotation and say that they annotation is only considered if type=ClusterIP and clusterIP==None ?

Review status: 0 of 15 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

Comments from Reviewable

smarterclayton · 2016-08-17T15:41:00Z

We should maybe discuss the annotation thing on the api machinery call today.

thockin · 2016-08-17T16:02:16Z

I don't know if there's a general answer to extending arbitrary enums with alpha values, especially when some platforms don't want to enable alpha features. I think it has to be case-by-case. In this case, define the requirement as the most innocuous setting + an alpha override.

Also, alpha features have to be gateable. Do we need a --enable-alpha-annotations flag for apiserver that everyone can check?

Reviewed 1 of 5 files at r2, 14 of 14 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

Comments from Reviewable

therc · 2016-08-17T16:05:46Z

I might be able to join in the second half of the API meeting, but I'm at IDF16, so I'm not sure if I'll be able to find a corner quiet enough.

smarterclayton · 2016-08-17T16:07:39Z

We don't have anything gating alpha features today consistently. Partially
that's because external components don't have shared global config. I'd
probably argue that any alpha feature represented in the API needs to be
controlled by the API server, which potentially means filtering annotations
on both input and output. The security issues with annotations are also
problematic - since annotations aren't discoverable (you can't discover
which alpha features exist) lots of implementers don't realize they need to
support / defend against annotations, and since annotations are one big
bucket there's no way to limit which annotations can get set by which users
short of admission control.

On Wed, Aug 17, 2016 at 12:02 PM, Tim Hockin notifications@github.com
wrote:

I don't know if there's a general answer to extending arbitrary enums with
alpha values, especially when some platforms don't want to enable alpha
features. I think it has to be case-by-case. In this case, define the
requirement as the most innocuous setting + an alpha override.

Also, alpha features have to be gateable. Do we need a

--enable-alpha-annotations flag for apiserver that everyone can check?

Reviewed 1 of 5 files at r2, 14 of 14 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved

discussions, some commit checks failed.

Comments from Reviewable
https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/30599#-:-KPODGKPpnN9zTDv13NA:b6hsbzf

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30599 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p7cq6guBLlA2K8MqUUx-4TqEXAJ_ks5qgzCYgaJpZM4JkAx2
.

thockin · 2016-08-17T18:44:38Z

Something we DIDN'T do for this is open a feature bug. @therc should do that. Given your unavailability this week, and the lack of a corresponding DNS change, I have very little confidence it will make the 1.4 window. :(

thockin · 2016-08-17T18:50:59Z

It seems I am wrong, and we DO have a feature bug, but maybe have not been updating it.

therc · 2016-08-17T19:04:55Z

I mentioned the feature at the top. I can't update it, though.Oh, Github. :-)

I'm trying to finish the DNS change today.

thockin · 2016-08-18T21:24:00Z

So the open issues are:

Was this all in vain because it needs to be an annotation
Where's the corresponding kube-dns change
e2e
docs

Reviewed 1 of 1 files at r4, 10 of 10 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

pkg/api/types.go, line 1742 [r3] (raw file):

Previously, therc (Rudi C) wrote…

I reworded it, but it's still a bit clumsy.

How about:

// Type determines how the Service is exposed. Defaults to ClusterIP. Valid options are ExternalName, ClusterIP, NodePort, and LoadBalancer.
// "ExternalName" maps to the specified externalName.
// "ClusterIP" allocates a cluster-internal IP address for load-balancing to endpoints. Endpoints are determined by the selector or if that is not specified, by manual construction of an Endpoints object. If clusterIP is "None", no virtual IP is allocated and the endpoints are published as a set of endpoints rather than a stable IP.
// "NodePort" builds on ClusterIP and allocates a port on every node which routes to the clusterIP.
// "LoadBalancer" builds on NodePort and creates an external load-balancer (if supported in the current cloud) which routes to the clusterIP

pkg/api/v1/defaults.go, line 117 [r3] (raw file):

Previously, therc (Rudi C) wrote…

I thought that defaults happened before validation, but I think they happen before CREATION instead. pkg/api/rest/create.go:BeforeCreate calls Validate. And pkg/registry/service/rest:Create calls BeforeCreate, then allocates a VIP. The VIP is allocated only if clusterIP == "". I changed IsServiceIPRequested to short-circuit to False for ExternalName services so thatthey don't trigger an allocation, either, then I changed the check to make sure that clusterIP == "".

I have also removed the default.

Ahh yes, gooooood

Comments from Reviewable

therc · 2016-08-18T21:51:59Z

I was up until almost 1 last night, I must have forgotten to press Submit. :-( #30931 implements the kube-dns change, along with simple unit tests and e2e.

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

Comments from Reviewable

therc · 2016-08-19T16:20:48Z

Review status: 1 of 15 files reviewed at latest revision, 5 unresolved discussions.

pkg/api/types.go, line 1742 [r3] (raw file):

Previously, thockin (Tim Hockin) wrote…

How about:

// Type determines how the Service is exposed. Defaults to ClusterIP. Valid options are ExternalName, ClusterIP, NodePort, and LoadBalancer.
// "ExternalName" maps to the specified externalName.
// "ClusterIP" allocates a cluster-internal IP address for load-balancing to endpoints. Endpoints are determined by the selector or if that is not specified, by manual construction of an Endpoints object. If clusterIP is "None", no virtual IP is allocated and the endpoints are published as a set of endpoints rather than a stable IP.
// "NodePort" builds on ClusterIP and allocates a port on every node which routes to the clusterIP.
// "LoadBalancer" builds on NodePort and creates an external load-balancer (if supported in the current cloud) which routes to the clusterIP

Done.

Comments from Reviewable

thockin · 2016-08-19T18:12:50Z

Code LGTM.

I discussed this with a few folks and I think we can move this through without the usual beta cycle. Rationale: it's already opt-in (no change unless users explicitly activate it), the implementation is simple, test coverage should be sufficient, and the crater is relatively small.

I picked a couple nits, so I can't quite LGTM this - fix those and ping me. The subsequent DNS change also LGTM. Needs docs and final steps from the feature checklist. I'll also re-title it for relnote

+lgtm

Reviewed 5 of 15 files at r6.
Review status: 5 of 15 files reviewed at latest revision, 4 unresolved discussions.

pkg/api/validation/validation.go, line 2232 [r6] (raw file):

          allErrs = append(allErrs, ValidateDNS1123Subdomain(service.Spec.ExternalName, specPath.Child("externalName"))...)
      } else {
          allErrs = append(allErrs, field.Invalid(specPath.Child("externalName"), service.Spec.ExternalName, "externalName must not be empty"))

error string should not repeat field name

This should be handled as field.Required() - see elsewhere

Comments from Reviewable

ExternalName allows kubedns to return CNAME records for external services. No proxying is involved. See original issue at kubernetes#13748 Feature tracking at kubernetes/enhancements#33

therc · 2016-08-19T18:33:01Z

Review status: 4 of 15 files reviewed at latest revision, 4 unresolved discussions.

pkg/api/validation/validation.go, line 2232 [r6] (raw file):

Previously, thockin (Tim Hockin) wrote…

error string should not repeat field name

This should be handled as field.Required() - see elsewhere

Done.

Comments from Reviewable

k8s-bot · 2016-08-19T19:40:38Z

GCE e2e build/test passed for commit 88fdb96.

thockin · 2016-08-19T21:07:45Z

LGTM +lgtm

Reviewed 1 of 1 files at r7.
Review status: 5 of 15 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

therc · 2016-08-19T23:54:37Z

@thockin could you add the milestone so it doesn't end up at the very bottom of the submit queue? I also don't know if the fact that it's an API change makes it any higher than a P3.

k8s-github-robot · 2016-08-20T02:37:08Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-08-20T03:20:42Z

GCE e2e build/test passed for commit 88fdb96.

k8s-github-robot · 2016-08-20T03:20:50Z

Automatic merge from submit-queue

Automatic merge from submit-queue Add ExternalName kube-dns e2e test ExternalName allows kubedns to return CNAME records for external services. No proxying is involved. Built on top of and includes #30599 See original issue at #13748 Feature tracking at kubernetes/enhancements#33 The e2e test is at least as comprehensive as the one for headless services (namely, only to some degree) ```release-note Add ExternalName services as CNAME references to external ones ```

googlebot added the cla: yes label Aug 14, 2016

k8s-github-robot assigned erictune Aug 14, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Aug 14, 2016

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

smarterclayton reviewed Aug 16, 2016
View reviewed changes

therc force-pushed the ext-svc-ref branch from d9482ff to f2fa470 Compare August 16, 2016 22:57

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 16, 2016

erictune assigned thockin and unassigned erictune Aug 17, 2016

therc force-pushed the ext-svc-ref branch from f2fa470 to e2c4160 Compare August 17, 2016 16:13

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

thockin mentioned this pull request Aug 18, 2016

Idea: make headless services first-class #30918

Closed

therc mentioned this pull request Aug 18, 2016

Add ExternalName kube-dns e2e test #30931

Merged

therc force-pushed the ext-svc-ref branch from e4c904b to 5ea4ad4 Compare August 19, 2016 16:16

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 19, 2016

thockin changed the title ~~Add ExternalName to ServiceSpec~~ Add Service type "ExternalName" which results in CNAME DNS Aug 19, 2016

Add ExternalName to ServiceSpec

88fdb96

ExternalName allows kubedns to return CNAME records for external services. No proxying is involved. See original issue at kubernetes#13748 Feature tracking at kubernetes/enhancements#33

therc force-pushed the ext-svc-ref branch from 5ea4ad4 to 88fdb96 Compare August 19, 2016 18:31

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2016

thockin added this to the v1.4 milestone Aug 20, 2016

thockin added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Aug 20, 2016

k8s-github-robot merged commit 1aecaf2 into kubernetes:master Aug 20, 2016

chengzhang1016 mentioned this pull request Oct 9, 2016

ExternalName type service cannot be created if without spec.ports. #34394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Service type "ExternalName" which results in CNAME DNS #30599

Add Service type "ExternalName" which results in CNAME DNS #30599

therc commented Aug 14, 2016 •

edited

Loading

therc commented Aug 14, 2016

therc commented Aug 14, 2016

therc commented Aug 15, 2016

smarterclayton Aug 16, 2016

therc Aug 16, 2016

smarterclayton commented Aug 16, 2016

thockin commented Aug 17, 2016

thockin commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

thockin commented Aug 17, 2016

therc commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

--enable-alpha-annotations flag for apiserver that everyone can check?

discussions, some commit checks failed.

thockin commented Aug 17, 2016

thockin commented Aug 17, 2016

therc commented Aug 17, 2016

thockin commented Aug 18, 2016

therc commented Aug 18, 2016

therc commented Aug 19, 2016

thockin commented Aug 19, 2016

therc commented Aug 19, 2016

k8s-bot commented Aug 19, 2016

thockin commented Aug 19, 2016

therc commented Aug 19, 2016

k8s-github-robot commented Aug 20, 2016

k8s-bot commented Aug 20, 2016

k8s-github-robot commented Aug 20, 2016

Add Service type "ExternalName" which results in CNAME DNS #30599

Add Service type "ExternalName" which results in CNAME DNS #30599

Conversation

therc commented Aug 14, 2016 • edited Loading

therc commented Aug 14, 2016

therc commented Aug 14, 2016

therc commented Aug 15, 2016

smarterclayton Aug 16, 2016

Choose a reason for hiding this comment

therc Aug 16, 2016

Choose a reason for hiding this comment

smarterclayton commented Aug 16, 2016

thockin commented Aug 17, 2016

thockin commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

thockin commented Aug 17, 2016

therc commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

--enable-alpha-annotations flag for apiserver that everyone can check?

discussions, some commit checks failed.

thockin commented Aug 17, 2016

thockin commented Aug 17, 2016

therc commented Aug 17, 2016

thockin commented Aug 18, 2016

therc commented Aug 18, 2016

therc commented Aug 19, 2016

thockin commented Aug 19, 2016

therc commented Aug 19, 2016

k8s-bot commented Aug 19, 2016

thockin commented Aug 19, 2016

therc commented Aug 19, 2016

k8s-github-robot commented Aug 20, 2016

k8s-bot commented Aug 20, 2016

k8s-github-robot commented Aug 20, 2016

therc commented Aug 14, 2016 •

edited

Loading