ExternalDNS Resource Creation Refactor #308
Conversation
|
/ok-to-test sha=abc88e7 |
|
/ok-to-test sha=305acc1 |
pkg/manifests/external_dns.go
Outdated
| for _, dnsConfig := range externalDnsConfigs { | ||
| // Can safely assume the namespace exists if using kube-system | ||
| if dnsConfig.Namespace != "" && dnsConfig.Namespace != "kube-system" { |
There was a problem hiding this comment.
where do we default to a namespace in dnsConfig?
we the default should be conf.NS (that's the old behavior)
There was a problem hiding this comment.
we should really control the constructor for the externalDns configs within this package to prevent misuse
There was a problem hiding this comment.
I agree with the use of the constructor - i'll make the type private and add a public constructor, so that there's no need to default on a namespace
|
/ok-to-test sha=faa277e |
| } | ||
| func publicConfigForIngress(conf *config.Config) *manifests.ExternalDnsConfig { | ||
| return manifests.NewExternalDNSConfig( | ||
| conf, |
There was a problem hiding this comment.
we are passing in so many arguments here. should use a struct as input instead, it's too long.
There was a problem hiding this comment.
resolving per offline discussion re: correct usage of params
There was a problem hiding this comment.
I still think taking in a struct param is better. this is a lot of args
There was a problem hiding this comment.
so we'd have one externaldnsconfig that's visible to the controllers and one internally? that just seems like a confusing pattern.
Either we declare the type in the controller, which would create a cyclical dependency, or we expose the fields in the public type - but that violates the initial premise of forming the constructor (ie to prevent incorrect usage/assignment of the fields/to abstract them away from the controllers).
We could also create a third type within manifests but that seems excessive
|
/ok-to-test sha=c920ec5 |
|
/ok-to-test sha=7f7ff6d |
Description
Refactoring how we create ExternalDNS resources to parameterize serviceaccount, namespace, and identity. It will be necessary to not couple these to the cluster conf fed in at application start time to allow for externaldns resource creation via crds/to allow for workload identity support.
Fixes # (issue)
Feature # (details)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tests have been adjusted.
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration. Is it a breaking change which will impact consuming tool(s)?
Checklist: