Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kube-proxy: validate dual-stack cidrs #87353

Merged
merged 1 commit into from Feb 13, 2020
Merged

Conversation

@aojea
Copy link
Member

aojea commented Jan 18, 2020

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change

/kind bug

/kind cleanup

/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

kube-proxy was not validating correctly the clusterCIDRs, if
dual-stack it MAY have 1 or more clusterCIDRs. If it has 2 cidrs,
at least one of each IP family.

It also fixes a bug where validation was not taking into account
the feature gates global state.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix kube-proxy dual-stack configuration validation

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


@aojea

This comment has been minimized.

Copy link
Member Author

aojea commented Jan 18, 2020

@aojea aojea force-pushed the aojea:kproxy_dual branch from 4c9a4f0 to 43eb774 Jan 18, 2020
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Jan 18, 2020
@aojea aojea force-pushed the aojea:kproxy_dual branch from 43eb774 to ff5538d Jan 19, 2020
@aojea

This comment has been minimized.

Copy link
Member Author

aojea commented Jan 19, 2020

/retest

@aojea aojea force-pushed the aojea:kproxy_dual branch from ff5538d to b516042 Jan 21, 2020
}
cidrs := strings.Split(config.ClusterCIDR, ",")

switch {

This comment has been minimized.

Copy link
@aojea

aojea Jan 21, 2020

Author Member

please review carefully my logic, since config.ClusterCIDR != "" I assume len(cidrs)will be always >0

@aojea aojea force-pushed the aojea:kproxy_dual branch from b516042 to 68f42f7 Jan 21, 2020
@aojea aojea force-pushed the aojea:kproxy_dual branch from ca3c4c6 to cf036ac Jan 21, 2020
@aojea

This comment has been minimized.

Copy link
Member Author

aojea commented Jan 22, 2020

PTAL, I've submitted a new commit so you can check the diff based on the comments. It will be squashed later

@aojea

This comment has been minimized.

Copy link
Member Author

aojea commented Jan 22, 2020

/hold
until this has a wider consensus

@aojea aojea mentioned this pull request Jan 29, 2020
kube-proxy was not validating correctly the clusterCIDRs, if
dual-stack it MAY have 1 or more clusterCIDRs. If it has 2 cidrs and
at least one of each IP family.

It also fixes a bug where validation was not taking into account
the feature gates global state.
@aojea aojea force-pushed the aojea:kproxy_dual branch from 3fa351b to 4844b38 Jan 31, 2020
@aojea

This comment has been minimized.

Copy link
Member Author

aojea commented Jan 31, 2020

/retest

@aojea

This comment has been minimized.

Copy link
Member Author

aojea commented Jan 31, 2020

/hold cancel
implementing jordan and rosti feedback about validation, I've already sent a PR to update the KEP too,
With dual stack not more than 2 cidrs allowed, dual cidrs one of each IP family or only one cidr

@aojea

This comment has been minimized.

Copy link
Member Author

aojea commented Jan 31, 2020

/retest

1 similar comment
@aojea

This comment has been minimized.

Copy link
Member Author

aojea commented Feb 2, 2020

/retest

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 7, 2020

/lgtm

will defer to sig-network for approval

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 7, 2020
@liggitt liggitt removed their assignment Feb 7, 2020
@aojea

This comment has been minimized.

Copy link
Member Author

aojea commented Feb 8, 2020

/retest
@thockin PTAL

Copy link
Member

thockin left a comment

Thanks!

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 12, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 12, 2020

/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 k8s-ci-robot merged commit b9c57a1 into kubernetes:master Feb 13, 2020
12 of 16 checks passed
12 of 16 checks passed
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
tide Not mergeable. Retesting: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation aojea authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.