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

Bring back external-dns service account #329

Merged
merged 2 commits into from
Mar 1, 2021
Merged

Bring back external-dns service account #329

merged 2 commits into from
Mar 1, 2021

Conversation

k0da
Copy link
Collaborator

@k0da k0da commented Feb 28, 2021

During CoreDNS refactor https://github.com/AbsaOSS/k8gb/pull/292
local external-dns instance was removed, while Route53 and NS1
external-dns instances relies on service account created by local
instance. This commit brings back external-dns service account.

Fixes: https://github.com/AbsaOSS/k8gb/issues/328

Signed-off-by: Dinar Valeev dinar.valeev@absa.africa

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

Do we still need this ServiceAccount for cases when absaoss/k8s_crd CoreDNS plugin is used instead of local external-dns?
If not, maybe SA template should be made optional for such cases?

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

During CoreDNS refactor https://github.com/AbsaOSS/k8gb/pull/292
local external-dns instance was removed, while Route53 and NS1
external-dns instances relies on service account created by local
instance. This commit brings back external-dns service account.

Fixes: https://github.com/AbsaOSS/k8gb/issues/328

Signed-off-by: Dinar Valeev <dinar.valeev@absa.africa>
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

time="2021-03-01T08:47:03Z" level=debug msg="Considering zone: /hostedzone/Z1026666KUI3QVMMU5ZI (domain: k8gb.io.)"
time="2021-03-01T08:47:03Z" level=error msg="dnsendpoints.externaldns.k8s.io is forbidden: User \"system:serviceaccount:k8gb:external-dns\" cannot list resource \"dnsendpoints\" in API group \"externaldns.k8s.io\" at the cluster scope"

We definitely need to return RBAC part

@ytsarev
Copy link
Member

ytsarev commented Mar 1, 2021

@k0da i've amended PR

@ytsarev
Copy link
Member

ytsarev commented Mar 1, 2021

@somaritane thinking of condition for route53 and ns1? Makes sense, though I don't like the idea of listing all external-dns based edgeDNS providers explicitly. Make it inverse for Infoblox condition?

@k0da
Copy link
Collaborator Author

k0da commented Mar 1, 2021

Do we need ingresses, pods and services for external-dns? IIUC we're only insterested in DNSEndpoint object

@ytsarev
Copy link
Member

ytsarev commented Mar 1, 2021

@k0da good point, current conf was just took from the upstream. We can strict it down to DNSEndpoints

@k0da
Copy link
Collaborator Author

k0da commented Mar 1, 2021

also verbs should be limited to get, read only.

Signed-off-by: Yury Tsarev <yury.tsarev@absa.africa>
@somaritane
Copy link
Contributor

@somaritane thinking of condition for route53 and ns1? Makes sense, though I don't like the idea of listing all external-dns based edgeDNS providers explicitly. Make it inverse for Infoblox condition?

@ytsarev, definitely should not list external-dns providers as there going to be more.
Thinking about Infoblox in condition, is it going to be the only case where we use k8s_crd CoreDNS plugin instead of local external-dns?

@ytsarev
Copy link
Member

ytsarev commented Mar 1, 2021

@somaritane slight misunderstanding here it seems. We are using coredns crd plugin in every case. It replaces the previous setup of local external-dns+etcd. In the case of infoblox we just don't use external-dns+infoblox-provider given its limitations and interacting with Infoblox api directly with k8gb. Will it be the only case of direct API integration is an open question as the external-dns providers heavily deviate from a quality standpoint.

So the external-dns instances in the context of this PR are purely externally edgeDNS facing.

@ytsarev
Copy link
Member

ytsarev commented Mar 1, 2021

@somaritane so the ClusterRole reasonably became pretty much read-only for DNSEndpoints. In this case we can neglect additional conditions IMHO and simply install it every time

@somaritane
Copy link
Contributor

@ytsarev I see now. In this case let's make rbac template created only in case if Infoblox provider is not enabled, so we don't create entities we're not going to use in cluster.

@ytsarev
Copy link
Member

ytsarev commented Mar 1, 2021

@somaritane but it's vulnerable to the same scenario. If we add second non external-dns based edgeDNS we will have to list it. It's more burden than having 3 unused security fine rbac entities in the cluster

@somaritane
Copy link
Contributor

@ytsarev makes sense.

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

lgtm

@k0da k0da merged commit 0f97708 into master Mar 1, 2021
@somaritane somaritane deleted the extdns_svc branch March 9, 2021 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

edgeDNS external-dns pods are failing to start since v0.7.5
3 participants