add ability to disable nodePort Support #4727

jpiper · 2024-09-05T13:55:01Z

Description

When running external-dns in a multi-tenant environment (e.g. in namespaced=true mode in the helm chart), users may not be in a position to give access to the Node resource to the external-dns deployment. At the moment, external-dns will not function without access to the Node resource as a Node informer is always created by the Pod and Service sources.

In this PR I add the flag --ignore-nodeports that disables the Node informer and then ensures the Pod and Service sources makes no attempt to try and retrieve any information from the Node resource.

k8s-ci-robot · 2024-09-05T13:55:09Z

Welcome @jpiper!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-09-05T13:55:10Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

munnerz · 2024-09-05T13:59:27Z

/ok-to-test

mloiseleur

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 ?

szuecs · 2024-09-18T15:06:05Z

source/pod.go

-		},
-	)
+		)
+	}


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

good point, I've dropped the log level in servive/pod to a debug level and then added warnings here.

jpiper · 2024-10-04T08:57:47Z

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 ?

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 disableNodeSupport?, which then really implies that node hostnames/IPs will not be able to be synced?

jpiper · 2024-10-04T09:00:14Z

alternatives I can think of for the flag:

--(disable|ignore)-node-(info|data|resources)
--disable-node-informer (leaks implementation details)

jpiper · 2024-10-04T09:49:02Z

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,

I am sorry: I do not understand this. When one is namespaced, AFAIK there is no Cluster RBAC. so there is no access possible to node, since it's a Cluster-wide resources.

Did I miss something ?

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 namespaced helm value is set to true

timbunce · 2024-10-04T10:41:13Z

source/compatibility.go

@@ -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)


Since the user explicitly disabled nodePort support then Info seems more appropriate than Warn.

Suggested change

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.

I do not thinkg it's an ifo level, but not a warning as well. What this should warn us about?

log.Warn:
- Purpose: Indicates a potential issue or a situation that might lead to problems in the future.
When to Use:
- Unexpected Behavior: When something unexpected happens but the application can still function.
- Resource Limitations: When resources are running low (e.g., disk space, memory).
- Configuration Issues: When encountering unexpected or invalid configuration values.

log.Debug:

- Purpose: Used for detailed information that is primarily helpful for debugging. When to Use: - Development and Testing: Useful for tracking the flow of execution, inspecting variable values, and understanding the internal workings of your application. - Troubleshooting: Helps pinpoint the root cause of issues during development and debugging.

Do you think there is a value in having this log as a warning instead of a debug?

The message contains as nodePort support is disabled

Is it for cases when nodePort explicitly disabled or accidently and RBAC needs modification?

source/pod.go

source/service.go

mloiseleur · 2024-10-27T13:31:12Z

/lgtm

mloiseleur · 2024-12-22T17:52:53Z

/assign @Raffo

k8s-ci-robot · 2025-01-27T11:58:17Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2025-01-27T11:58:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from raffo. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jpiper · 2025-01-27T12:07:53Z

I have rebased the PR

mloiseleur · 2025-01-28T08:17:38Z

cc @ivankatliarchuk

ivankatliarchuk

How this was tested?

Could you also provide a set of manual test steps using manifests and kubectl commands? This will help in understanding and verifying the implementation.

ivankatliarchuk · 2025-01-28T08:59:43Z

charts/external-dns/CHANGELOG.md

@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 - Added ability to configure `imagePullSecrets` via helm `global` value. ([#4667](https://github.com/kubernetes-sigs/external-dns/pull/4667)) _@jkroepke_
 - Added options to configure `labelFilter` and `managedRecordTypes` via dedicated helm values. ([#4849](https://github.com/kubernetes-sigs/external-dns/pull/4849)) _@abaguas_
+- Added support for `ignore-nodeports` argument ([#4727](https://github.com/kubernetes-sigs/external-dns/pull/4727)) _@jpiper_


Could we move chart modifications to a separate pull request? I do not think this change is even necessary, as it's just adds a single flag, which is supported with args:

Relevant comment from another pull request #4946 (comment)

ivankatliarchuk · 2025-01-28T09:05:29Z

charts/external-dns/values.yaml

@@ -235,6 +235,9 @@ labelFilter:
 # -- Record types to manage (default: A, AAAA, CNAME)
 managedRecordTypes: []

+# -- Disable nodePort support (for environments where access to Node resources is forbidden)
+ignoreNodePorts: false


Does this mean that

gnoreNodePorts: true

This should modify clusterroles and recources["nodes"]? here

external-dns/charts/external-dns/templates/clusterrole.yaml

Line 9 in b41f3fd

{{- if and (not .Values.namespaced) (or (has "node" .Values.sources) (has "pod" .Values.sources) (has "service" .Values.sources) (has "contour-httpproxy" .Values.sources) (has "gloo-proxy" .Values.sources) (has "openshift-route" .Values.sources) (has "skipper-routegroup" .Values.sources)) }}

ivankatliarchuk · 2025-01-28T09:17:23Z

source/compatibility.go

@@ -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)


I do not thinkg it's an ifo level, but not a warning as well. What this should warn us about?

log.Warn:
- Purpose: Indicates a potential issue or a situation that might lead to problems in the future.
When to Use:
- Unexpected Behavior: When something unexpected happens but the application can still function.
- Resource Limitations: When resources are running low (e.g., disk space, memory).
- Configuration Issues: When encountering unexpected or invalid configuration values.

log.Debug:

- Purpose: Used for detailed information that is primarily helpful for debugging. When to Use: - Development and Testing: Useful for tracking the flow of execution, inspecting variable values, and understanding the internal workings of your application. - Troubleshooting: Helps pinpoint the root cause of issues during development and debugging.

Do you think there is a value in having this log as a warning instead of a debug?

ivankatliarchuk · 2025-01-28T09:18:52Z

source/compatibility.go

@@ -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)


The message contains as nodePort support is disabled

Is it for cases when nodePort explicitly disabled or accidently and RBAC needs modification?

ivankatliarchuk · 2025-01-28T09:21:40Z

source/pod.go

-		cache.ResourceEventHandlerFuncs{
-			AddFunc: func(obj interface{}) {
+	if disableNodeInformer {
+		log.Infoln("host information (host IP/hostname) for pods is disabled as the node informer is disabled")


Same. Is this a debug level? As we explicitly disable it with the flag disableNodeInformer so the user is aware of what is he doing?

ivankatliarchuk · 2025-01-28T09:24:16Z

source/pod.go

+		nodeInformer = informerFactory.Core().V1().Nodes()
+		nodeInformer.Informer().AddEventHandler(
+			cache.ResourceEventHandlerFuncs{
+				AddFunc: func(obj interface{}) {


is this function with empty body required for which purpose?

ivankatliarchuk · 2025-01-28T09:28:09Z

source/pod.go

+				} else {
+					domainList := splitHostnameAnnotation(domainAnnotation)
+					for _, domain := range domainList {
+						node, _ := ps.nodeInformer.Lister().Get(pod.Spec.NodeName)


What if the node not exist by now? Shell we handle error here? Could be a potential panic or CrashLoop for no reason

k8s-ci-robot · 2025-01-29T01:57:47Z

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.

mloiseleur · 2025-02-15T08:41:28Z

@jpiper Do you think you can rebase and address @ivankatliarchuk review comments ?

jpiper · 2025-02-15T09:41:55Z

Yes I can 1 but I am on holiday for the next week. I can get to it on the 24th Feb!Sent from my iPhoneOn 15 Feb 2025, at 08:42, Michel Loiseleur ***@***.***> wrote: @jpiper Do you think you can rebase and address @ivankatliarchuk review comments ?—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***> mloiseleur left a comment (kubernetes-sigs/external-dns#4727) @jpiper Do you think you can rebase and address @ivankatliarchuk review comments ? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

k8s-ci-robot requested a review from mloiseleur September 5, 2024 13:55

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 5, 2024

k8s-ci-robot requested a review from szuecs September 5, 2024 13:55

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 5, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 5, 2024

mloiseleur reviewed Sep 7, 2024

View reviewed changes

szuecs reviewed Sep 18, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

timbunce reviewed Oct 4, 2024

View reviewed changes

k8s-ci-robot assigned mloiseleur Oct 27, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 27, 2024

k8s-ci-robot assigned Raffo Dec 22, 2024

mloiseleur removed their assignment Dec 22, 2024

jpiper force-pushed the disable_nodeport branch from 7ec6439 to dee7e8b Compare January 27, 2025 11:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 27, 2025

jpiper added 2 commits January 27, 2025 12:00

add ability to disable node informer

fefe366

fix existing tests

b626234

jpiper added 7 commits January 27, 2025 12:00

add test for pod source

3a12829

add helm docs

a4b037c

update chart changes

e650871

make logging less chatty

4816413

chart: automatically disable node informer when namespaced set

7d1c0a4

fix helm templating

0f93cb6

change log level

7d096b5

jpiper force-pushed the disable_nodeport branch from dee7e8b to 7d096b5 Compare January 27, 2025 12:04

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2025

fix rebase

9ce2a7a

jpiper requested a review from mloiseleur January 27, 2025 12:07

run flag generator

d748e9a

ivankatliarchuk suggested changes Jan 28, 2025

View reviewed changes

k8s-ci-robot assigned ivankatliarchuk Jan 28, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ability to disable nodePort Support #4727

add ability to disable nodePort Support #4727

jpiper commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

munnerz commented Sep 5, 2024

mloiseleur left a comment •

edited

Loading

szuecs Sep 18, 2024 •

edited

Loading

jpiper Oct 4, 2024

jpiper commented Oct 4, 2024

jpiper commented Oct 4, 2024

This comment was marked as resolved.

jpiper commented Oct 4, 2024

timbunce Oct 4, 2024

ivankatliarchuk Jan 28, 2025

ivankatliarchuk Jan 28, 2025

mloiseleur commented Oct 27, 2024

mloiseleur commented Dec 22, 2024

k8s-ci-robot commented Jan 27, 2025

k8s-ci-robot commented Jan 27, 2025

jpiper commented Jan 27, 2025

mloiseleur commented Jan 28, 2025

ivankatliarchuk left a comment

ivankatliarchuk Jan 28, 2025

ivankatliarchuk Jan 28, 2025

ivankatliarchuk Jan 28, 2025

ivankatliarchuk Jan 28, 2025

ivankatliarchuk Jan 28, 2025

ivankatliarchuk Jan 28, 2025

ivankatliarchuk Jan 28, 2025

k8s-ci-robot commented Jan 29, 2025

mloiseleur commented Feb 15, 2025

jpiper commented Feb 15, 2025 via email

	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)

add ability to disable nodePort Support #4727

Are you sure you want to change the base?

add ability to disable nodePort Support #4727

Conversation

jpiper commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

munnerz commented Sep 5, 2024

mloiseleur left a comment • edited Loading

Choose a reason for hiding this comment

szuecs Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpiper commented Oct 4, 2024

jpiper commented Oct 4, 2024

This comment was marked as resolved.

jpiper commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloiseleur commented Oct 27, 2024

mloiseleur commented Dec 22, 2024

k8s-ci-robot commented Jan 27, 2025

k8s-ci-robot commented Jan 27, 2025

jpiper commented Jan 27, 2025

mloiseleur commented Jan 28, 2025

ivankatliarchuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 29, 2025

mloiseleur commented Feb 15, 2025

jpiper commented Feb 15, 2025 via email

mloiseleur left a comment •

edited

Loading

szuecs Sep 18, 2024 •

edited

Loading