ExtraPortMapping Validation #3302

aroradaman · 2023-07-09T09:24:43Z

Using random host ports for validating extra port mapping defined without host ports (default behaviour for provider docker and podman)

Fixes #3301

pkg/internal/apis/config/validate.go

pkg/internal/apis/config/validate_test.go

aojea · 2023-07-10T15:03:35Z

pkg/internal/apis/config/validate.go

+		// using default listen address
+		if portMapping.ListenAddress == "" {
+			portMapping.ListenAddress = wildcardAddrIPv4.String()
+		}
+
+		// using default protool
+		if portMapping.Protocol == "" {
+			portMapping.Protocol = PortMappingProtocolTCP
+		}


we have a default stage in pkg/internal/apis/config/default.go

point is we should default things in defaulting stage, not here in validation

should I remove defaults from pkg/cluster/internal/providers/<provider>/provision.go then and add it as part of SetDefaultsCluster in default stage .. defaulting with respect to provider ?

kind/pkg/cluster/internal/providers/docker/provision.go

Lines 363 to 377 in 3610f60

// do provider internal defaulting

// in a future API revision we will handle this at the API level and remove this

if pm.ListenAddress == "" {

switch clusterIPFamily {

case config.IPv4Family, config.DualStackFamily:

pm.ListenAddress = "0.0.0.0" // this is the docker default anyhow

case config.IPv6Family:

pm.ListenAddress = "::"

default:

return nil, errors.Errorf("unknown cluster IP family: %v", clusterIPFamily)

}

}

if string(pm.Protocol) == "" {

pm.Protocol = config.PortMappingProtocolTCP // TCP is the default

}

kind/pkg/cluster/internal/providers/podman/provision.go

Lines 376 to 390 in 3610f60

// do provider internal defaulting

// in a future API revision we will handle this at the API level and remove this

if pm.ListenAddress == "" {

switch clusterIPFamily {

case config.IPv4Family, config.DualStackFamily:

pm.ListenAddress = "0.0.0.0"

case config.IPv6Family:

pm.ListenAddress = "::"

default:

return nil, errors.Errorf("unknown cluster IP family: %v", clusterIPFamily)

}

}

if string(pm.Protocol) == "" {

pm.Protocol = config.PortMappingProtocolTCP // TCP is the default

}

pkg/apis/config/v1alpha4/default.go

aojea · 2023-07-14T13:35:05Z

/lgtm
/assign @BenTheElder

for final approval

aojea · 2023-07-14T13:38:16Z

/lgtm cancel

don't we need to modify ./pkg/internal/apis/config/default.go too?

aroradaman · 2023-07-14T14:25:42Z

@aojea Is ./pkg/internal/apis/config/default.go generated by code-generator ?
If yes, can you point me to documentation for the same ?

k8s-ci-robot · 2023-07-14T14:59:02Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aroradaman
Once this PR has been reviewed and has the lgtm label, please ask for approval from aojea. For more information see the Kubernetes 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

pkg/apis/config/defaults/networking.go

BenTheElder

I've been out sick and I'm going to need some more time to think about how to better structure this, but I don't think exporting all of these values is the answer.

Thanks for working on this problem!

prahaladramji · 2023-10-02T12:47:26Z

I've been out sick and I'm going to need some more time to think about how to better structure this, but I don't think exporting all of these values is the answer.

Thanks for working on this problem!

Just following up to find out what's the current thoughts on this PR, I'm looking for a resolution to #3301.

@BenTheElder Is the feedback more around the implementation details of this PR or is the above mentioned issue the desired behaviour of kind moving forward post v0.18.0?

BenTheElder · 2023-11-29T23:07:05Z

I was hoping for a response to the existing comments before reviewing further.

Signed-off-by: Daman Arora <aroradaman@gmail.com>

aroradaman · 2023-11-30T08:08:32Z

/retest

prahaladramji · 2023-12-04T10:16:11Z

@aroradaman is there anything I can help with here? Or is this a test that could pass with a retest?

aroradaman · 2023-12-05T12:23:36Z

@prahaladramji I don't think retest would help here, and also the error is not related to this PR.

@BenTheElder do we still need pull-kind-e2e-kubernetes-1-24 given that it has already reached EOL. I also see pull-kind-e2e-kubernetes-1-28 to be missing.

k8s-ci-robot · 2023-12-26T21:19:33Z

@aroradaman: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kind-e2e-kubernetes-1-24	`6fa6758`	link	true	`/test pull-kind-e2e-kubernetes-1-24`

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.

aroradaman · 2023-12-26T21:21:12Z

/test pull-kind-e2e-kubernetes-1-29

aroradaman · 2024-02-01T13:14:20Z

ping @BenTheElder

prahaladramji · 2024-02-01T20:20:43Z

The 1 failing check seemes to be related to vagrant as part of the ci step and not related to this change.

A general look at that error seems like it could just be flakey vagrant step (potentially a retry may work). But if not, I'm happy to try and help look at fixing that step.

k8s-ci-robot · 2024-02-10T06:36:38Z

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/test-infra repository.

aroradaman · 2024-02-10T06:55:28Z

/close in favor of #3513
cc. @prahaladramji

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 9, 2023

k8s-ci-robot requested review from aojea and neolit123 July 9, 2023 09:24

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 9, 2023

aojea reviewed Jul 9, 2023

View reviewed changes

pkg/internal/apis/config/validate.go Outdated Show resolved Hide resolved

aroradaman force-pushed the fix/extra-port-mapping branch from bc44865 to 7620fc4 Compare July 9, 2023 09:56

aojea reviewed Jul 10, 2023

View reviewed changes

pkg/internal/apis/config/validate_test.go Show resolved Hide resolved

aojea reviewed Jul 10, 2023

View reviewed changes

aroradaman force-pushed the fix/extra-port-mapping branch from 7620fc4 to 62018bd Compare July 14, 2023 13:01

aojea reviewed Jul 14, 2023

View reviewed changes

pkg/apis/config/v1alpha4/default.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned BenTheElder and aojea Jul 14, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2023

aroradaman force-pushed the fix/extra-port-mapping branch from 62018bd to 9ba8845 Compare July 14, 2023 14:58

aroradaman force-pushed the fix/extra-port-mapping branch 2 times, most recently from fb8fe10 to c5bc252 Compare July 14, 2023 15:28

BenTheElder reviewed Aug 8, 2023

View reviewed changes

pkg/apis/config/defaults/networking.go Outdated Show resolved Hide resolved

BenTheElder reviewed Aug 8, 2023

View reviewed changes

BenTheElder mentioned this pull request Nov 29, 2023

A unique hostPort is not generated when using extraPortMappings without specifying hostPort. #3301

Closed

aroradaman force-pushed the fix/extra-port-mapping branch from c5bc252 to c4c87b7 Compare November 30, 2023 06:36

k8s-ci-robot added area/provider/docker Issues or PRs related to docker area/provider/podman Issues or PRs related to podman labels Nov 30, 2023

aroradaman force-pushed the fix/extra-port-mapping branch from c4c87b7 to dbec326 Compare November 30, 2023 06:38

aroradaman force-pushed the fix/extra-port-mapping branch from dbec326 to dea0eaa Compare November 30, 2023 06:41

aroradaman added 2 commits November 30, 2023 12:40

move extra port mapping defaulting to API level

e9912a3

Signed-off-by: Daman Arora <aroradaman@gmail.com>

fix extra port mapping validation

6fa6758

Signed-off-by: Daman Arora <aroradaman@gmail.com>

aroradaman force-pushed the fix/extra-port-mapping branch from dea0eaa to 6fa6758 Compare November 30, 2023 07:11

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

aojea closed this Feb 10, 2024

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ExtraPortMapping Validation #3302

ExtraPortMapping Validation #3302

aroradaman commented Jul 9, 2023

aojea Jul 10, 2023

aojea Jul 13, 2023

aroradaman Jul 13, 2023

aojea commented Jul 14, 2023

aojea commented Jul 14, 2023

aroradaman commented Jul 14, 2023

k8s-ci-robot commented Jul 14, 2023

BenTheElder left a comment

prahaladramji commented Oct 2, 2023

BenTheElder commented Nov 29, 2023

aroradaman commented Nov 30, 2023

prahaladramji commented Dec 4, 2023

aroradaman commented Dec 5, 2023

k8s-ci-robot commented Dec 26, 2023 •

edited

Loading

aroradaman commented Dec 26, 2023

aroradaman commented Feb 1, 2024

prahaladramji commented Feb 1, 2024

k8s-ci-robot commented Feb 10, 2024

aroradaman commented Feb 10, 2024

	// do provider internal defaulting
	// in a future API revision we will handle this at the API level and remove this
	if pm.ListenAddress == "" {
	switch clusterIPFamily {
	case config.IPv4Family, config.DualStackFamily:
	pm.ListenAddress = "0.0.0.0" // this is the docker default anyhow
	case config.IPv6Family:
	pm.ListenAddress = "::"
	default:
	return nil, errors.Errorf("unknown cluster IP family: %v", clusterIPFamily)
	}
	}
	if string(pm.Protocol) == "" {
	pm.Protocol = config.PortMappingProtocolTCP // TCP is the default
	}

ExtraPortMapping Validation #3302

ExtraPortMapping Validation #3302

Conversation

aroradaman commented Jul 9, 2023

aojea Jul 10, 2023

Choose a reason for hiding this comment

aojea Jul 13, 2023

Choose a reason for hiding this comment

aroradaman Jul 13, 2023

Choose a reason for hiding this comment

aojea commented Jul 14, 2023

aojea commented Jul 14, 2023

aroradaman commented Jul 14, 2023

k8s-ci-robot commented Jul 14, 2023

BenTheElder left a comment

Choose a reason for hiding this comment

prahaladramji commented Oct 2, 2023

BenTheElder commented Nov 29, 2023

aroradaman commented Nov 30, 2023

prahaladramji commented Dec 4, 2023

aroradaman commented Dec 5, 2023

k8s-ci-robot commented Dec 26, 2023 • edited Loading

aroradaman commented Dec 26, 2023

aroradaman commented Feb 1, 2024

prahaladramji commented Feb 1, 2024

k8s-ci-robot commented Feb 10, 2024

aroradaman commented Feb 10, 2024

k8s-ci-robot commented Dec 26, 2023 •

edited

Loading