add ability to disable nodePort Support #4727
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @jpiper! |
|
Hi @jpiper. 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-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
There was a problem hiding this comment.
Thanks, this PR looks good, is tested and documented 👍 .
As Albert Camus said:
To name things wrongly is to add to the misfortune of the world
If I understand correctly, when external-dns is namespaced, it does not make sense to set ignoreNodePorts to true.
So, 🤔 wdyt of renaming ignore-nodeports cli args into namespaced ?
And so, in the chart, when namespaced is enabled, it would add this --namespaced=true that would disable the informer ?
| }, | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
should we warn here in else instead of in Endpoints() ?
I guess it's expected to not flood all day the logs.
Same applies to service
There was a problem hiding this comment.
good point, I've dropped the log level in servive/pod to a debug level and then added warnings here.
It really depends, in an environment where you have access to the node resource, you could still run in the namespaced mode with the nodeinformer enabled, it really depends on your multi-tenancy model of your cluster. I have seen people give tenants access to the node resource, and likewise, multi-tenant clusters where you are not allowed to view the node resource. However, I agree, the naming could be improved. How do we feel about |
|
alternatives I can think of for the flag: |
This comment was marked as resolved.
This comment was marked as resolved.
ah yes, this is my fault, sorry. I am getting confused between how the chart works and how one could theoretically deploy external-dns using a namespace scope but still using cluster resources. The chart doesn't support this deployment mode though so you're right, I can disable the node informer with the helm |
| @@ -123,6 +125,11 @@ func legacyEndpointsFromDNSControllerService(svc *v1.Service, sc *serviceSource) | |||
| // It will use node role label to check if the node has the "node" role. This means control plane nodes and other | |||
| // roles will not be used as targets. | |||
| func legacyEndpointsFromDNSControllerNodePortService(svc *v1.Service, sc *serviceSource) ([]*endpoint.Endpoint, error) { | |||
| if sc.nodeInformer == nil { | |||
| log.Warnf("Unable to extract nodePort targets from service %s/%s as nodePort support is disabled", svc.Namespace, svc.Name) | |||
There was a problem hiding this comment.
Since the user explicitly disabled nodePort support then Info seems more appropriate than Warn.
| log.Warnf("Unable to extract nodePort targets from service %s/%s as nodePort support is disabled", svc.Namespace, svc.Name) | |
| log.Infof("Unable to extract nodePort targets from service %s/%s as nodePort support is disabled", svc.Namespace, svc.Name) |
I've suggested Info level here (and elsewhere Warn is used) but I see other places in the PR where Debug is used.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
PR needs rebase. 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-sigs/prow repository. |
k8s-ci-robot
added
the
needs-rebase
|
/assign @Raffo |
Description
When running external-dns in a multi-tenant environment (e.g. in
namespaced=truemode in the helm chart), users may not be in a position to give access to theNoderesource to the external-dns deployment. At the moment, external-dns will not function without access to theNoderesource as a Node informer is always created by thePodandServicesources.In this PR I add the flag
--ignore-nodeportsthat disables the Node informer and then ensures thePodandServicesources makes no attempt to try and retrieve any information from theNoderesource.