CoreDNS/SkyDNS provider #253
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
istalker2
force-pushed
the
coredns
branch
2 times, most recently
from
dcf8dd7
to
862679f
Compare
This mostly hides the circular dependency, by putting them in the same package. It's still there on a logical level. :) As the provider doesn't depend on the registry (but the other way around) it should be possible to test it without the registry. |
|
TL;DR; the comment from @mikkeloscar has been addressed, "interfaces" package has gone. Thank you! The reason why I created interfaces package is to use TXT registry in coredns_test.go. Currently this is impossible due to circular imports. This doesn't discard the fact that registries are tight to providers on logical level, but decouples interface from implementations and breaks circular imports. However, this makes it to be not a true unit test as it touches several units at once. Also reorganizing code in the same PR with a new feature is not good even though I believe such reorganization is good. So I removed the interfaces package and changed the test |
|
@istalker2 could you please adjust etcd client to work with tls certificates, as in kubernetes/kubernetes#47049 ? |
|
@mikkeloscar could you please take yet another look there? What needs to be addressed to merge that (besides obvious conflict in main.go)? Should it wait for #228, because it seems that development there stalled a bit. |
|
@dshulyak done. Please see 3rd commit message for details |
|
+1 for this |
|
What is missing to get this merged? |
| api etcd.KeysAPI | ||
| } | ||
|
|
||
| var _ skyDNSClient = etcdClient{} |
| } | ||
|
|
||
| // Service represents SkyDNS/CoreDNS etcd record | ||
| type Service struct { |
There was a problem hiding this comment.
I would prefer it to be SkyDNSService
| @@ -0,0 +1,392 @@ | |||
| /* | |||
There was a problem hiding this comment.
testing coverage needs to be increased, currently it is only ~40% for this file
istalker2
force-pushed
the
coredns
branch
2 times, most recently
from
a928724
to
ea5e710
Compare
linki
added
docs missing
kind/feature
k8s-ci-robot
added
the
size/XXL
istalker2
force-pushed
the
coredns
branch
2 times, most recently
from
9078b57
to
da74c04
Compare
|
this still alive? |
This commit adds support for CoreDNS through its etcd middleware. Because the middleware is backward compatible with SkyDNS this commit adds support for SkyDNS as well. In fact, new provider is available under two names in CLI (coredns and skydns). All interactions with middleware happen through etcd cluster, whose location (URIs) is specified via --etcd CLI parameter by default http://localhost:2379). The provider translates CoreDNS/DkyDNS SRV records to A/CNAME + optional TXT endpoints, when reading from etcd and combines A/CNAME with TXT endpoints back into single SRV record when writing it back. Also adds github.com/coreos/etcd package to glide.yaml and vendor folder because it is used by the provider
|
This commit adds ability to use TLS transport for etcd. New logic is applied when the etcd URL has https:// scheme. TLS parameters are passed in the environment variables: ETCD_CA_FILE - path to CA certificate. If not specified, then system-provided certificates are used. ETCD_CERT_FILE - client certificate ETCD_KEY_FILE - client key file - either both of none of this two must be specified ETCD_TLS_SERVER_NAME - expected CN of the certificate. Useful when URL points to a different domain from that in server certificate ETCD_TLS_INSECURE - if set to "1" (or "true" or "yes") makes client bypass server certificate validation. Also for unification with other providers and rest of connection settings, etcd URL is no longer specified in the command line, but rather in ETCD_URLS environment variable (defaults to http://localhost:2379). More than one comma-separated URL can be specified. All of the URLs must start with either http:// or https:// Also, now it possible to communicate with etcd through proxy specified in standard environment variables
|
Is this finished? |
|
@istalker2 Could you add a tutorial of coredns in external-dns/docs/tutorials/ teach us how to use it ? |
|
@gogeof You use it exactly the same way as other externanal-dns providers:
|
|
@istalker2 thanks a lot here is my Dockerfile to build external-dnshere is my external-dns.yaml |
|
@Gegeof these warnings have nothing to do with the provider code. Actually, it shows, that provider works as expected. The warnings you see are printed by external-dns sources which collect data that is going to be fed to the provider then. external-dns scans various supported k8s objects marked with special annotation and tries to generate a DNS record out of their metadata. There can be several different approaches for generating such record out of an object. But if none of them succeed you get such warning. You're going to get it with any provider |
|
@istalker2 yes, you're right, I have found the annotations in code: source/service.go and find the annotation here: This is my fault, maybe I should learn more how to use external-dns, I will continue. Thanks a lot. |
|
@istalker2 thanks a lot, I use your repo to build external-dns, and after test, it works for me. After your code merge, I will add doc. |
|
Hello, Thank you for this pull request. I am testing external-dns with coredns provider, it works really fine. EDIT: I read somewhere in the ExternalDNS that mutliple IP are not supported, which make my message as no sense. Context:On our on-prem Kubernetes cluster, the Nginx ingress controller is deployed on two nodes ( When external-dns starts, it creates the 2 DNS records properly: You can notice:
The issueBut on the next updates, after
You can notice:
Across multiple updates from external-dns, DNS server returns the first, the second, or both IPs. ExpectedThe DNS server should always return all the IP addresses. Thank you for your help. Complete logs for information: |
|
Merged-in latest master branch |
|
hi all ! what's current status on this ? any ETA / blockers re: getting this merged ? Best regards! |
|
Any updates? |
k8s-ci-robot
added
size/XL
|
I merged in master and fixed the conflicts:
|
|
Thanks again for your PR, will be released this week. |
So far, the last column was labelled with `STATUS` and had three options (installed, available, unavailable). This was hard to read because installed and available have the same length, and `unavailable` was not specific. Now, the column is labelled `INSTALLED` with values yes/no/unavailable on <PLATFORM>
This PR adds support for CoreDNS through its etcd middleware.
Because the middleware is backward compatible with SkyDNS this
commit adds support for SkyDNS as well. In fact, new provider
is available under two names in CLI (coredns and skydns).
All interactions with middleware happen through etcd cluster,
whose location (URIs) is specified via --etcd CLI parameter
by default http://localhost:2379).
The provider translates CoreDNS/DkyDNS SRV records to
A/CNAME + optional TXT endpoints, when reading from etcd and
combines A/CNAME with TXT endpoints back into single SRV record
when writing it back.
Also:
because it is used by the provider
declaration there. This is required in order to use registry in
provider unit tests. Otherwise there going to be circular imports
because provider UTests would import registry which imports providers
back.