Filter nodePortAddresses to proxiers

[uablrek]

What type of PR is this?

/kind bug

What this PR does / why we need it:

To avoid faulty entries in ipvs when both ipv4 and ipv6 addresses are used in "nodePortAddresses".

May also cause some yet unseen faults.

Which issue(s) this PR fixes:

Fixes #89923

Special notes for your reviewer:

I did not see any faults for proxy-mode=iptables, but the same filtering might be appropriate also for proxy-mode=iptables.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[uablrek]

With;

nodePortAddresses: ["::1/128","127.0.0.1/32"]

Ipvs config;

$ ipvsadm -Ln
IP Virtual Server version 1.2.1 (size=4096)
Prot LocalAddress:Port Scheduler Flags
  -> RemoteAddress:Port           Forward Weight ActiveConn InActConn
TCP  12.0.0.1:443 rr
  -> 192.168.1.1:6443             Masq    1      0          0         
TCP  12.0.19.107:443 rr
  -> 11.0.3.5:4443                Masq    1      0          0         
TCP  12.0.208.66:5001 rr
  -> 11.0.0.2:5001                Masq    1      0          0         
  -> 11.0.1.2:5001                Masq    1      0          0         
  -> 11.0.2.2:5001                Masq    1      0          0         
  -> 11.0.3.4:5001                Masq    1      0          0         
TCP  127.0.0.1:30100 rr
  -> 11.0.0.2:5001                Masq    1      0          0         
  -> 11.0.1.2:5001                Masq    1      0          0         
  -> 11.0.2.2:5001                Masq    1      0          0         
  -> 11.0.3.4:5001                Masq    1      0          0         
TCP  [fd00:4000::7710]:5001 rr
  -> [1100::2]:5001               Masq    1      0          0         
  -> [1100:0:0:1::2]:5001         Masq    1      0          0         
  -> [1100:0:0:2::2]:5001         Masq    1      0          0         
  -> [1100:0:0:3::4]:5001         Masq    1      0          0         
TCP  [::1]:30200 rr
  -> [1100::2]:5001               Masq    1      0          0         
  -> [1100:0:0:1::2]:5001         Masq    1      0          0         
  -> [1100:0:0:2::2]:5001         Masq    1      0          0         
  -> [1100:0:0:3::4]:5001         Masq    1      0          0         

[aojea]

I think you should add it to the single stack one too

kubernetes/pkg/proxy/ipvs/proxier.go

Line 483 in 7b20442

nodePortAddresses: nodePortAddresses,

nodePortAddresses:     filterCIDRs(isIPv6, nodePortAddresses),

[uablrek]

@aojea You mean in case of user mistakes?

[uablrek]

The corresponding in the iptables proxier;

kubernetes/pkg/proxy/iptables/proxier.go

Lines 351 to 363 in 7b20442

	ipv4Proxier, err := NewProxier(ipt[0], sysctl,
	exec, syncPeriod, minSyncPeriod, masqueradeAll, masqueradeBit, localDetectors[0], hostname,
	nodeIP[0], recorder, healthzServer, nodePortAddresses)
	if err != nil {
	return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err)
	}
	ipv6Proxier, err := NewProxier(ipt[1], sysctl,
	exec, syncPeriod, minSyncPeriod, masqueradeAll, masqueradeBit, localDetectors[1], hostname,
	nodeIP[1], recorder, healthzServer, nodePortAddresses)
	if err != nil {
	return nil, fmt.Errorf("unable to create ipv6 proxier: %v", err)
	}

[uablrek]

If the "filterCIDRs()" shall be used in the iptables proxier it should be moved to some "util" packade IMO to avoid duplicated code (and duplicated tests).

If so, where shall it go?

[aojea]

@aojea You mean in case of user mistakes?

🤔 well,I think you are right, if you define IPv6 addresses in --nodeport-addresses in an IPv4 cluster is clearly an user mistake.

If the "filterCIDRs()" shall be used in the iptables proxier it should be moved to some "util" packade IMO to avoid duplicated code (and duplicated tests).

If so, where shall it go?

my apologies, I was assuming you were already using this one

kubernetes/pkg/proxy/util/utils.go

Lines 240 to 243 in 4d0e86f

	// FilterIncorrectCIDRVersion filters out the incorrect IP version case from a slice of CIDR strings.
	func FilterIncorrectCIDRVersion(ipStrings []string, isIPv6Mode bool) ([]string, []string) {
	return filterWithCondition(ipStrings, isIPv6Mode, utilnet.IsIPv6CIDRString)
	}

[uablrek]

@aojea Thanks for the pointer 😃 The "filterCIDRs()" is just below the updated code and is already used in the call.

I will remove the "filterCIDRs()" to clean-up the code and also move to the place you pointed out. Then it can't go wrong since "NewProxy()" is called for ipv4/ipv6 in dual-stack. I think it will be cleaner.

Also I will add the update in the iptables proxier to avoid any yet-unseen problems.

[uablrek]

Hmm, IMHO "FilterIncorrectCIDRVersion()" is not intuitive. Not the name and not how it works. You must read the code to use it. I will use it in the iptables proxier, but keep "filterCIDRs()" in the ipvs proxier and let reviewers decide.

[uablrek]

The nodePortAddresses are filtered in NewProxy() as proposed by @aojea in #89998 (comment).

Not only will this catch user mistakes which are probably common in transition to dual-stack and makes the code a little bit cleaner, it also allows a common setting for ipv4-only, ipv6-only and dual-stack in our use-case;

Use case

When K8s is deployed in a DC (in VMs or bare-metal) with no K8s-aware load-balancers, loadbalancing to nodePorts can not be used. The external addresses must be configured in DC-GWs in some way, likely announced with BGP. kube-proxy will then load-balance incoming traffic.

This works fine but since NodePorts are implicit for services with type: LoadBalancer (and can't be turned-off) a number of ports above 30000 are opened but not used.

These ports show up on mandatory security scans, and a request to either describe their purpose or to close them is raised.

We can't close them but by setting;

nodePortAddresses: ["::1/128","127.0.0.1/32"]

we do not expose them externally. They will not show up on port scans any more.

[uablrek]

As the robot suggested;
/assign @bowei

[SataQiu]

/lgtm

[uablrek]

Corrections after review are pushed.

True to;

It does not seem right to crash kube-proxy in case of a misconfigured NodePortAddresses.

a warning is logged if NodePortAddresses of the wrong family is configured in a single-stack cluster;

W0515 07:37:53.620500     254 proxier.go:439] NodePortAddresses of wrong family; [::1/128]

In dual-stack the addresses are filtered in NewDualStackProxier() so the proxier instanses only gets addresses with it's family (no warnings).

[uablrek]

/test pull-kubernetes-integration

[uablrek]

/retest

[uablrek]

Problem does not seem to be related to the PR
/retest

[uablrek]

/retest

[aojea]

/lgtm
/retest

[uablrek]

/retest

[uablrek]

@aojea Can you please help since you have good knowledge of flaky tests.

The pull-kubernetes-e2e-gci-gce-ipvs keeps on failing and I can't see any way this PR can cause the failures. Unless there really are NodePortAddresses of the wrong family this PR should not make any difference.

[aojea]

/test pull-kubernetes-e2e-gci-gce-ipvs

let me check again, but seems the common test failures on all runs are

[sig-storage] Flexvolumes should be mountable when non-attachable
[sig-storage] Flexvolumes should be mountable when attachable 

[aojea]

seems those tests were failing for a long time,
https://prow.k8s.io/job-history/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-e2e-gci-gce-ipvs

in the meantime let me fix the reporting in testgrid so we can notice it sooner next time (cc: @andrewsykim )

[uablrek]

@aojea Thanks a lot!

[aojea]

I'm also removing the storage tests for the ipvs jobs, I don't have time for debugging current failures and we weren't tracking those tests.
It is better having only tests we should care and increase later the scope if necessary kubernetes/test-infra#17623

[andrewsykim]

FYI #91253

@aojea I can investigate soon if you haven't started

[aojea]

FYI #91253

@aojea I can investigate soon if you haven't started

I just did an initial check, but nothing relevant. Please go ahead, I'll be watching the issue to check if I can help, but I'm a bit tied this week

[uablrek]

/retest

[uablrek]

Updated after review please see; #89998 (comment)

@liggitt Please have a new look at it.

The failing test is a problem with the test, not with this PR.

[uablrek]

/retest

[thockin]

Thanks!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, uablrek

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:

• pkg/proxy/OWNERS [thockin]

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

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[k8s-ci-robot]

@uablrek: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gci-gce-ipvs	f54b8f9	link	/test pull-kubernetes-e2e-gci-gce-ipvs

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.