Make TLS enabled by default for PowerDNS provider #3839
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @matusf. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/L
pkg/apis/externaldns/types.go
Outdated
| @@ -289,7 +289,7 @@ var defaultConfig = &Config{ | |||
| OVHApiRateLimit: 20, | |||
| PDNSServer: "http://localhost:8081", | |||
| PDNSAPIKey: "", | |||
| PDNSTLSEnabled: false, | |||
| PDNSTLSDisabled: false, | |||
There was a problem hiding this comment.
| PDNSTLSDisabled: false, | |
| PDNSSkipTLSVerify: false, |
There was a problem hiding this comment.
Changed there and also in other places by d0fabd0.
matusf
force-pushed
the
fix/pdns-tls-on
branch
from
6fe31bd
to
d0fabd0
Compare
provider/pdns/pdns.go
Outdated
| CAFilePath string | ||
| ClientCertFilePath string | ||
| ClientCertKeyFilePath string | ||
| } | ||
|
|
||
| func (tlsConfig *TLSConfig) setHTTPClient(pdnsClientConfig *pgo.Configuration) error { | ||
| if !tlsConfig.TLSEnabled { | ||
| if tlsConfig.SkipTLSVerify { | ||
| log.Debug("Skipping TLS for PDNS Provider.") |
There was a problem hiding this comment.
This doesn't seem right. It still has to configure TLS, it just needs to configure it to skip certificate verification.
There was a problem hiding this comment.
fixed by 721c4e2. now the SkipTLSVerify flag is being passed to the NewTLSConfig function
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/retest |
1 similar comment
|
/retest |
Use factory function which creates new PDNS provider. The only changing argument is TLSConfig, so we can default all of the rest.
Omited TLS config is the same as empty TLS config. It will default to the same value.
All of the providers have TLS enabled by default so this change will make PDNS provider behave as exected. Additionally, enabling TLS by default is a good practice and previous bahaviour was a bit misleading. It was possible to pass `--tls-ca` without `--pdns-tls-enabled` and the PDNS provider would ignore the tls and instantiate client with disabled tls. This change adds a flag to disable the tls: `--pdns-skip-tls-verify`. Similar flag is used by pihole and bluecat providers. Additionaly this change makes providing custom TLS CA optional. It if is not provided. A system certificates will be used. This makes PDNS behave the same as other providers.
matusf
force-pushed
the
fix/pdns-tls-on
branch
from
d0fabd0
to
721c4e2
Compare
|
/cc @mloiseleur |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mloiseleur, Raffo 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 |
k8s-ci-robot
added
the
approved
Description
All of the providers have TLS enabled by default so this change will
make PDNS provider behave as exected. Additionally, enabling TLS
by default is a good practice and previous bahaviour was a bit
misleading. It was possible to pass
--tls-cawithout--pdns-tls-enabledand the PDNS provider would ignore the tls andinstantiate client with disabled tls. This change adds a flag to disable
the tls:
--pdns-skip-tls-verify. Similar flag is used by pihole andbluecat providers.
Additionaly this change makes providing custom TLS CA optional. If it is
not provided, a system certificates will be used. This makes PDNS behave
the same as other providers.