-
Notifications
You must be signed in to change notification settings - Fork 101
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
Conversation
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.
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?
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.
I think we also need to return ClusterRole and ClusterRoleBinding https://github.com/AbsaOSS/k8gb/pull/292/files#diff-9519c8095c65b9bc934732e5b724b7ebfa3c792a96ac9e6a8037adeaa9ad26ecR3
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 <[email protected]>
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.
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
@k0da i've amended PR |
@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? |
Do we need ingresses, pods and services for external-dns? IIUC we're only insterested in DNSEndpoint object |
@k0da good point, current conf was just took from the upstream. We can strict it down to DNSEndpoints |
also verbs should be limited to get, read only. |
Signed-off-by: Yury Tsarev <[email protected]>
@ytsarev, definitely should not list external-dns providers as there going to be more. |
@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. |
@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 |
@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. |
@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 |
@ytsarev makes sense. |
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.
lgtm
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 [email protected]