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

Added service annotation to set Azure DNS label for public IP #47849

Merged
merged 1 commit into from Nov 13, 2017

Conversation

@tomerf
Contributor

tomerf commented Jun 21, 2017

What this PR does / why we need it: Added a feature to set the DNS label for public IPs in the Azure cloud.
For example:

apiVersion: v1
kind: Service
metadata:
  annotations:
    service.alpha.kubernetes.io/label-name: myservice
...

Will resolve myservice.westus.cloudapp.azure.com to the service's IP.

Which issue this PR fixes: fixes #44775

Special notes for your reviewer: Note that this is defining a new annotation, so feel free to point out if there is a preferred convention or anything else that needs to be done.

Release note:

New service annotation "service.beta.kubernetes.io/azure-dns-label-name" to set Azure DNS label name for public IP
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jun 21, 2017

Hi @tomerf. 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.

@tomerf

This comment has been minimized.

Contributor

tomerf commented Jun 21, 2017

/assign @smarterclayton

@tomerf

This comment has been minimized.

Contributor

tomerf commented Jun 28, 2017

Any comments on the new feature?

@quinton-hoole

This comment has been minimized.

Member

quinton-hoole commented Jul 17, 2017

@tomerf Is there any reason why this should apply only to Azure?
Also, is there a design document for this? It seems like the provision of DNS records for Ingress could/should be generalized, and not done one-off for a single provider.
I'd like @kubernetes/sig-network-feature-requests to take a look at this, and approve a design before merging the relevant PR.

@tomerf

This comment has been minimized.

Contributor

tomerf commented Jul 17, 2017

I'm working with Azure and could really use this functionality. I am not familiar with how other clouds implement this. I can research for other vendors if the network team agrees. I would also welcome suggestions/contributions from someone with the relevant experience.

Using annotations seems to me as the most reasonable approach at this level, and could use another review if generalized.

@quinton-hoole

This comment has been minimized.

Member

quinton-hoole commented Aug 3, 2017

@tomerf Yes, I imagine that if you proposed a design for exposing DNS for ingress in general, that would be well received. I do recall that there were some challenges with doing that (which is why it was not done initially). I don't remember the details though. @kubernetes/sig-network-feature-requests might be able to help. Or try their slack channel.
https://github.com/kubernetes/community/wiki/SIG-Network

@tomerf

This comment has been minimized.

Contributor

tomerf commented Aug 21, 2017

Ok, I have written a basic proposal. I listed a few use cases and some more ideas (like a static IP).
Can you provide feedback and let's continue from there?
Design proposal

@cmluciano

This comment has been minimized.

Member

cmluciano commented Aug 21, 2017

@tomerf Can you please submit this proposal as a PR against https://github.com/kubernetes/community/blob/master/contributors/design-proposals/ . You can then add the /sig network label so that SIG-Network maintainers can review it.

@@ -110,4 +110,7 @@ const (
// BetaAnnotationExternalTraffic An annotation that denotes if this Service desires to route
// external traffic to local endpoints only. This preserves Source IP and avoids a second hop.
BetaAnnotationExternalTraffic = "service.beta.kubernetes.io/external-traffic"
// AlphaAnnotationLabelName Annotation speficying the DNS label name in Azure for the service.
AlphaAnnotationLabelName = "service.alpha.kubernetes.io/label-name"

This comment has been minimized.

@quinton-hoole

quinton-hoole Aug 21, 2017

Member

I think we need to add a DNS qualifier here, to make it clear that this refers to a DNS label, not any other type of label in the system (of which there are very many).

So for example AlphaAnnotationServiceDNSLabelName and .../dns-label-name

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 30, 2017

Adding do-not-merge/release-note-label-needed because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-action-required", "release-note-experimental" or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

@tomerf

This comment has been minimized.

Contributor

tomerf commented Sep 2, 2017

@quinton-hoole I submitted a design proposal (kubernetes/community#983) but the sig-network team doesn't think this feature is generic enough, so I think we should pursue this idea for Azure only.
Maybe in the future someone will come with a more comprehensive idea (support for third party DNS services?).

@tomerf

This comment has been minimized.

Contributor

tomerf commented Sep 13, 2017

/retest
I don't see why the last test fails, ideas?

@tomerf

This comment has been minimized.

Contributor

tomerf commented Sep 25, 2017

All the tests pass, shall we continue?
@thockin @quinton-hoole @smarterclayton

@quinton-hoole

This comment has been minimized.

Member

quinton-hoole commented Sep 25, 2017

Thanks @tomerf . If this is going to be provider-specific I'd ideally prefer it to be implemented outside of the federation core (e.g. via some provider-specific controller that could watch for the annotations on ingress objects, and create the associated DNS records).

Have you considered that approach?

@tomerf

This comment has been minimized.

Contributor

tomerf commented Sep 25, 2017

@quinton-hoole What do you mean?
Everything is done in the azure provider-specific code. This is handled once only on the Azure Frontend IP resource creation so no need for a controller to watch for the annotation.
I discussed the design proposal with @thockin and he prefers it is done that way.

@quinton-hoole

This comment has been minimized.

Member

quinton-hoole commented Sep 25, 2017

@tomerf OK, thanks - let me take a closer look and think about it some more. Will try to do this week.

@tomerf

This comment has been minimized.

Contributor

tomerf commented Oct 8, 2017

@quinton-hoole How's it going?

@quinton-hoole

This comment has been minimized.

Member

quinton-hoole commented Oct 10, 2017

Sorry, I've been a bit swamped. I haven't forgotten about you.

@quinton-hoole

This comment has been minimized.

Member

quinton-hoole commented Oct 12, 2017

My apologies, I initially misread this as being related to federated services. But it's for normal Kubernetes services. I will defer to @kubernetes/sig-network-pr-reviews to decide on this one. They should review, approve etc.

@cmluciano

This comment has been minimized.

Member

cmluciano commented Oct 16, 2017

@kubernetes/sig-azure-misc

@tomerf

This comment has been minimized.

Contributor

tomerf commented Oct 26, 2017

@thockin ETA?

@feiskyer

The change LGTM. ping @thockin for approval.

@thockin

This comment has been minimized.

Member

thockin commented Nov 9, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 9, 2017

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Nov 9, 2017

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 44775

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

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

@tomerf

This comment has been minimized.

Contributor

tomerf commented Nov 13, 2017

/retest
Is there anything else needed to merge this?

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Nov 13, 2017

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Nov 13, 2017

@tomerf: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce 60ed8ef link /test pull-kubernetes-federation-e2e-gce
pull-kubernetes-e2e-gce 188db6f link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Nov 13, 2017

Automatic merge from submit-queue (batch tested with PRs 55594, 47849, 54692, 55478, 54133). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit e686b63 into kubernetes:master Nov 13, 2017

12 of 15 checks passed

pull-kubernetes-e2e-gce Job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-kubeadm-gce Parent Job Status Changed: Job triggered.
cla/linuxfoundation tomerf authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-e2e-gce-bazel Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-etcd3 Jenkins job succeeded.
Details
pull-kubernetes-e2e-gce-gpu Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment