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.

Sign up for GitHub

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

Jump to bottom

Always create kubeClusterIPSet in ipvs proxier #65388

Merged
merged 1 commit into from
Jun 23, 2018
Merged

Always create kubeClusterIPSet in ipvs proxier #65388

merged 1 commit into from
Jun 23, 2018

Conversation

lbernail
Copy link
Contributor

@lbernail lbernail commented Jun 22, 2018 •
edited
Loading

What this PR does / why we need it:
This PR creates the kubeClusterIPSet ipset even if kube-proxy is started without masqueradeAll and clusterCIDR.
This is necessary to masquerade traffic sent to a clusterIP from the host network namespace. The code to do so is actually already present here: https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/ipvs/proxier.go#L1220-L1244

However the second else (neither masqueradeAll nor clusterCIDR are set) cannot be used because, before this PR, the initial test if !proxier.ipsetList[kubeClusterIPSet].isEmpty() can never return true when masqueradeAll and clusterCIDR are not set because kubeClusterIPSet is empty.

Which issue(s) this PR fixes
Fixes #65158

Allow access to ClusterIP from the host network namespace when kube-proxy is started in IPVS mode without either masqueradeAll or clusterCIDR flags

Additional comment
Issue #65158 is closed because ClusterIP access from the host has already fixed in master, except for the case described here (no masquerade flag). More detail in the issue.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 22, 2018
@lbernail lbernail changed the title Always create kubeClusterIPSet Always create kubeClusterIPSet in ipvs proxier Jun 22, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 22, 2018
@m1093782566
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 23, 2018
@m1093782566
Copy link
Contributor

/lgtm

/approve

Thanks for catching this 👍

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernail, m1093782566

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2018
@m1093782566
Copy link
Contributor

/retest

@fejta-bot
Copy link

/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 comment for consistent failures.

@lbernail
Copy link
Contributor Author

@m1093782566 Thanks. Everything seems to look good now
Can we also get this merged in the 1.11 branch?

@m1093782566
Copy link
Contributor

/milestone v1.11

@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone Jun 23, 2018
@m1093782566
Copy link
Contributor

/kind bug

/sig network

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/network Categorizes an issue or PR as relevant to SIG Network. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Jun 23, 2018
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Needs Approval

@lbernail @m1093782566 @kubernetes/sig-network-misc

Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer.

Pull Request Labels
  • sig/network: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/bug: Fixes a bug discovered during the current release.
Help

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 65388, 64995). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 966c77c into kubernetes:master Jun 23, 2018
@chussenot
Copy link

😮 That's impressive!

@Rigdon Rigdon mentioned this pull request Jun 25, 2018
Closed
@nikhita nikhita mentioned this pull request Jul 6, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request Jul 10, 2018
Merge pull request #65821 from DataDog/automated-cherry-pick-of-#6538…
ec845ce 
…8-origin-release-1.11

Automatic merge from submit-queue.

Automated cherry pick of #65388

This PR has already been merged in master
(this is my first use of the cherry-pick script, let me know if something is missing)

```release-note
Allow access to ClusterIP from the host network namespace when kube-proxy is started in IPVS mode without either masqueradeAll or clusterCIDR flags
```
@lbernail lbernail mentioned this pull request May 13, 2019
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendandburns brendandburns Awaiting requested review from brendandburns

@m1093782566 m1093782566 Awaiting requested review from m1093782566

Assignees

@m1093782566 m1093782566

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/needs-approval priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.11
Development

Successfully merging this pull request may close these issues.

ClusterIPs are not accessible from the host when kube-proxy runs in IPVS mode
6 participants
@lbernail @m1093782566 @k8s-ci-robot @fejta-bot @k8s-github-robot @chussenot