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

Revise glossary entry for kube-proxy #15172

Merged
merged 1 commit into from
Jul 29, 2019
Merged

Revise glossary entry for kube-proxy #15172

merged 1 commit into from
Jul 29, 2019

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Jun 27, 2019 •
edited
Loading

  • Mark as relevant to networking.
  • Unmark as a core object. kube-proxy is not an API object.
  • Fix hyperlink to reference docs.
  • Include hyperlink in definition. The definition is used in /docs/concepts/overview/components/ and the hyperlink is particularly useful to have on that page.
  • Revise wording.

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 27, 2019
@netlify
Copy link

netlify bot commented Jun 27, 2019 •
edited
Loading

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 3bf62ab

https://deploy-preview-15172--kubernetes-io-master-staging.netlify.com

@sftim
Copy link
Contributor Author

sftim commented Jun 27, 2019

/assign @MistyHacks

jaypipes
jaypipes approved these changes Jun 28, 2019
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice improvements, Tim

mdlinville
mdlinville reviewed Jun 28, 2019
View reviewed changes
Copy link
Contributor

@mdlinville mdlinville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good contribution. I left a suggestion for further improvement. :)

content/en/docs/reference/glossary/kube-proxy.md Outdated

kube-proxy maintains network rules on nodes so that sessions (that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be improved by more active voice:

kube-proxy maintains network rules on nodes, such as iptables rules on Linux nodes or <what is this called on Windows nodes??> on Windows Server nodes. These network rules allow network communication to your Pods from network sessions inside or outside of your cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good feedback @MistyHacks. I think the wording I proposed is not strictly wrong but it would be misleading, especially for less common cluster configutations. On Windows nodes (and Linux nodes where iptables isn't available) I think kube-proxy is a userland proxy for TCP & SCTP. For UDP, kube-proxy in userland mode acts as a rewriting UDP forwarder.
There are acceleration options on Linux and, AFAIK, just Linux.

Sometimes kube-proxy manages the list of rules and implements the forwarding itself. Sometimes, kube-proxy gets OS kernel help: it manages the list of rules and lets another component do the forwarding (iptables mode / ipvs mode). iptables mode and ipvs mode are more efficient than userland forwarding.

Also relevant:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe “an internal set of traffic forwarding rules” would work for Windows(?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this rewording?

@sftim sftim changed the title Revise glossary entry for kube-proxy [WIP] Revise glossary entry for kube-proxy Jun 28, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2019
@sftim
Copy link
Contributor Author

sftim commented Jun 28, 2019 •
edited
Loading

Marked this as WIP. To find the right new wording, I need to take more care than I'd realized.

Comments & suggestions: welcome!

@sftim sftim force-pushed the 20190627_kube-proxy_glossary_update branch from cf2131e to 8804322 Compare July 28, 2019 16:37
@sftim sftim changed the title [WIP] Revise glossary entry for kube-proxy Revise glossary entry for kube-proxy Jul 28, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2019
mdlinville
mdlinville reviewed Jul 29, 2019
View reviewed changes
content/en/docs/reference/glossary/kube-proxy.md Outdated

kube-proxy can optionally copy those rules to the operating system's packet
filtering layer (eg iptables on Linux). Using the OS' packet filtering layer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider avoiding Latin (eg). Also you should put iptables in backticks. Also don't use plural possessive for "OS'". You could reword this to "To improve performance, kube-proxy can optionally....." But can you provide some info about when it would or wouldn't improve performance? Presumably there is a trade-off since it's not enabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kube-proxy uses the operating system packet filtering layer if there is one and it's available. I'll say that.

@sftim sftim force-pushed the 20190627_kube-proxy_glossary_update branch from 8804322 to 6bab083 Compare July 29, 2019 17:31
@mdlinville
Copy link
Contributor

/lgtm
/approve
/hold

You can lift the hold after you've made the final change. Thanks!

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 29, 2019
@sftim
Revise glossary entry for kube-proxy
3bf62ab 
- Mark as relevant to networking.
- Unmark as a core object. kube-proxy is not an API object.
- Fix hyperlink to reference docs.
- Include hyperlink in definition. This is used in /docs/concepts/overview/components/
- Revise wording.
@sftim sftim force-pushed the 20190627_kube-proxy_glossary_update branch from 6bab083 to 3bf62ab Compare July 29, 2019 19:45
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2019
@mdlinville
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mistyhacks

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

@sftim
Copy link
Contributor Author

sftim commented Jul 29, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2019
@sftim
Copy link
Contributor Author

sftim commented Jul 29, 2019

(also needs an /lgtm I think)

@mdlinville
Copy link
Contributor

It's a funny quirk but you an supply that LGTM, as the author. But I'll do it for you again. SOrry for the churn.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2019
@k8s-ci-robot k8s-ci-robot merged commit a589e55 into kubernetes:master Jul 29, 2019
@sftim sftim deleted the 20190627_kube-proxy_glossary_update branch July 29, 2019 20:56
@sftim
Copy link
Contributor Author

sftim commented Jul 29, 2019

Thanks. It feels odd to /lgtm my own PR. Noted for next time.

@raelga raelga mentioned this pull request Jul 29, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdlinville mdlinville mdlinville left review comments

@jaypipes jaypipes jaypipes approved these changes

@Rajakavitha1 Rajakavitha1 Awaiting requested review from Rajakavitha1

@stewart-yu stewart-yu Awaiting requested review from stewart-yu

Assignees

@mdlinville mdlinville

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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sftim @mdlinville @k8s-ci-robot @jaypipes