-
Notifications
You must be signed in to change notification settings - Fork 39.9k
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
Add Service type "ExternalName" which results in CNAME DNS #30599
Conversation
I also changed the comments a bit to make the two types.go files use the same wording for the same fields. |
cc @kubernetes/kube-api |
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. |
@@ -100,6 +100,14 @@ func ValidateDNS1123Label(value string, fldPath *field.Path) field.ErrorList { | |||
return allErrs | |||
} | |||
|
|||
func ValidateDNS1123Subdomain(value string, fldPath *field.Path) field.ErrorList { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Godoc on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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. |
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):
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):
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 |
yeah, I am nervous about how to extend an enum like this. Can we do i Review status: 0 of 15 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. Comments from Reviewable |
We should maybe discuss the annotation thing on the api machinery call today. |
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. Comments from Reviewable |
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. |
We don't have anything gating alpha features today consistently. Partially On Wed, Aug 17, 2016 at 12:02 PM, Tim Hockin [email protected]
|
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. :( |
It seems I am wrong, and we DO have a feature bug, but maybe have not been updating it. |
I mentioned the feature at the top. I can't update it, though.Oh, Github. :-) I'm trying to finish the DNS change today. |
So the open issues are:
Reviewed 1 of 1 files at r4, 10 of 10 files at r5. pkg/api/types.go, line 1742 [r3] (raw file):
|
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 |
Review status: 1 of 15 files reviewed at latest revision, 5 unresolved discussions. pkg/api/types.go, line 1742 [r3] (raw file):
|
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. pkg/api/validation/validation.go, line 2232 [r6] (raw file):
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
Review status: 4 of 15 files reviewed at latest revision, 4 unresolved discussions. pkg/api/validation/validation.go, line 2232 [r6] (raw file):
|
GCE e2e build/test passed for commit 88fdb96. |
LGTM +lgtm Reviewed 1 of 1 files at r7. Comments from Reviewable |
@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-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 88fdb96. |
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 ```
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.
This change is