Updating kube-proxy to trim space from loadBalancerSourceRanges

[robscott]

What type of PR is this?
/kind bug

What this PR does / why we need it:
This updates kube-proxy to trim any space from loadBalancerSourceRanges to match validation. A better long term fix will require updating Service validation for future Kubernetes versions to not trim spaces before validation.

Special notes for your reviewer:
This feels like the kind of fix that should be part of 1.19 and potentially backported to older Kubernetes versions.

Does this PR introduce a user-facing change?:

kube-proxy now trims extra spaces found in loadBalancerSourceRanges to match Service validation.

/sig network
/priority critical-urgent
/assign @bowei

[robscott]

@bowei I've added another check and test now for better coverage here. Now if syncProxyRules ever runs into an invalid CIDR it simply drops it - this happens after TrimSpace is called so extra spaces do not invalidate a CIDR. I don't think this should ever get hit though. The validation for this field actually trims spaces before running validation. Whether or not that's a good idea I'm not sure, but we were not doing that in kube-proxy as a consumer of the API. Changing the core validation would require a much longer release cycle and would likely be relatively difficult since the actual code that should change is in the utils repo.

If you're curious, here's the validation flow:

1. Core Service validation calls GetLoadBalancerSourceRanges
2. GetLoadBalancerSourceRanges calls utilnet.ParseIPNets
3. utilnet.ParseIPNets call strings.TrimSpace before validating.

[bowei]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, robscott

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 [bowei]

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

[aojea]

shouldn't we caught this in validation?

kubernetes/pkg/apis/core/validation/validation.go

Lines 4320 to 4339 in 14a1106

	// Validate SourceRange field and annotation
	_, ok := service.Annotations[core.AnnotationLoadBalancerSourceRangesKey]
	if len(service.Spec.LoadBalancerSourceRanges) > 0 || ok {
	var fieldPath *field.Path
	var val string
	if len(service.Spec.LoadBalancerSourceRanges) > 0 {
	fieldPath = specPath.Child("LoadBalancerSourceRanges")
	val = fmt.Sprintf("%v", service.Spec.LoadBalancerSourceRanges)
	} else {
	fieldPath = field.NewPath("metadata", "annotations").Key(core.AnnotationLoadBalancerSourceRangesKey)
	val = service.Annotations[core.AnnotationLoadBalancerSourceRangesKey]
	}
	if service.Spec.Type != core.ServiceTypeLoadBalancer {
	allErrs = append(allErrs, field.Forbidden(fieldPath, "may only be used when `type` is 'LoadBalancer'"))
	}
	_, err := apiservice.GetLoadBalancerSourceRanges(service)
	if err != nil {
	allErrs = append(allErrs, field.Invalid(fieldPath, val, "must be a list of IP ranges. For example, 10.240.0.0/24,10.250.0.0/24 "))
	}
	}

[robscott]

@aojea you would think so, but unfortunately not. Outlined the flow in #94107 (comment). Unfortunately strings.TrimSpace is called before any meaningful validation is run but not before kube-proxy used the same field.

[bowei]

The validation function is technically wrong -- validation should not be mutating the input before validating. An IP address with extra whitespace is not a valid IP address.
@robscott will file an issue to track potentially fixing that behavior. It's likely complicated to fix due to how old it is in the codebase.

[aojea]

yeah, didn't mean to ask for changes. I was curious about what will be the "ideal" solution

An IP address with extra whitespace is not a valid IP address.

... , and I was thinking if validation should reject the IP as invalid ...

It's likely complicated to fix due to how old it is in the codebase.

absolutely agree on this

[robscott]

/retest

[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.

[robscott]

/retest

[liggitt]

kubernetes/test-infra#19080 (comment)

/retest

[cpanato]

@robscott @bowei do we need to backport this to 1.18 and 1.17 as well?

[robscott]

@cpanato @bowei Created PRs to backport this to 1.18 and 1.17 as well.