Skip to content
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 in-cluster pod and services dns names to no_proxy #1661

Merged
merged 4 commits into from Jun 17, 2020

Conversation

amwat
Copy link
Contributor

@amwat amwat commented Jun 10, 2020

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 10, 2020
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 10, 2020
@@ -294,6 +294,9 @@ func getProxyEnv(cfg *config.Cluster, networkName string, nodeNames []string) (m

noProxyList := append(subnets, envs[common.NOProxy])
noProxyList = append(noProxyList, nodeNames...)
// Add .svc explicitly to no_proxy to allow in cluster
// pod and service dns resolution
noProxyList = append(noProxyList, ".svc")
Copy link
Contributor

@aojea aojea Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we have to no_proxy the cluster domain , if we look at the standard coredns config

apiVersion: v1
data:
  Corefile: |
    .:53 {
        errors
        health {
           lameduck 5s
        }
        ready
        kubernetes cluster.local in-addr.arpa ip6.arpa {
           pods insecure
           fallthrough in-addr.arpa ip6.arpa
           ttl 30
        }
        prometheus :9153
        forward . /etc/resolv.conf
        cache 30
        loop
        reload
        loadbalance
    }

we should add .cluster.local

If you see the DNS spec for k8s, the svc is a subdomain https://github.com/kubernetes/dns/blob/master/docs/specification.md

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, thanks for linking to the spec 👍
I also found a couple of issues related to this: kubernetes/kubeadm#2114

I think we'll need to add all of those since k8s also resolves partially qualified names
.svc,.svc.cluster,.svc.cluster.local especially for things like admission webhook which only use .svc by default https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#service-reference, and this still won't solve the problem where services should be resolvable through http://<service> and http://<service>.<ns>

Also, I wasn't sure if cluster.local is static or something that we should be picking up from the coredns config, but looks like it is the default.

Copy link
Contributor

@aojea aojea Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll need to add all of those since k8s also resolves partially qualified names
.svc,.svc.cluster,.svc.cluster.local especially for things like admission webhook which only use .svc by default https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#service-reference, and this still won't solve the problem where services should be resolvable through http://<service> and http://<service>.<ns>

TIL, thanks for the info

Also, I wasn't sure if cluster.local is static or something that we should be picking up from the coredns config, but looks like it is the default.

it is the kubeadm default I guess, but per example the kubernetes CI that changes it
I think that for KIND environments we should be safe, maybe just add a comment mentioning this, don't know, an environment with proxies and changing the default kind domain seems a very specific case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's configurable unfortunately

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 10, 2020
@aojea
Copy link
Contributor

aojea commented Jun 10, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2020
@BenTheElder
Copy link
Member

sorry for the wait
/lgtm
/approve
thanks for this!

@BenTheElder BenTheElder merged commit 6455b92 into kubernetes-sigs:master Jun 17, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amwat, BenTheElder

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2020
@amwat amwat deleted the svc branch June 17, 2020 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants