jmorris/document k8s pr 77523#36
Open
MorrisLaw wants to merge 2 commits intocontributing-to-kubernetes:masterfrom
Open
jmorris/document k8s pr 77523#36MorrisLaw wants to merge 2 commits intocontributing-to-kubernetes:masterfrom
MorrisLaw wants to merge 2 commits intocontributing-to-kubernetes:masterfrom
Conversation
MorrisLaw
force-pushed
the
jmorris/document-k8s-pr-77523
branch
from
1d92016 to
0d76d50
Compare
alejandrox1
reviewed
Apr 8, 2020
Collaborator
alejandrox1
left a comment
alejandrox1
left a comment
There was a problem hiding this comment.
review done!
And awesome writeup @MorrisLaw !
Comment on lines
+15
to
+19
| ``` | ||
| "Iptables and ip6tables are used to set up, maintain, and inspect the tables of IPv4 and IPv6 packet filter rules in the Linux kernel. Several different tables may be defined. Each table contains a number of built-in chains and may also contain user-defined chains. | ||
|
|
||
| Each chain is a list of rules which can match a set of packets. Each rule specifies what to do with a packet that matches. This is called a 'target', which may be a jump to a user-defined chain in the same table." | ||
| ``` |
Collaborator
There was a problem hiding this comment.
Can we break this into more lines?
It renders into 2 long lines that you have to scroll for
|
|
||
| In the context of `Kubernetes`, the `iptables proxier` is described as follows: | ||
| ``` | ||
| Proxier is an `iptables` based proxy that is used for connections between a localhost:lport and services that provide the actual backends. |
Collaborator
There was a problem hiding this comment.
localhost:lport , is that intentional?
|
|
||
| ### What are iptables? | ||
|
|
||
| In order to understand what the `iptables proxier` is and does, we first have to understand what `iptables` are and why they are relevant. |
Collaborator
There was a problem hiding this comment.
Can you write a short snippet introducing the "iptables proxier" before this line?
It reads like it just came out of nowhere.
|
|
||
| ## Description | ||
|
|
||
| This write up is on [PR #77523](https://github.com/kubernetes/kubernetes/pull/77523) and [PR #78662](https://github.com/kubernetes/kubernetes/pull/78662). This PR re-routes traffic that originates from the local node, so that it goes to the Kubernetes service chain. |
Collaborator
There was a problem hiding this comment.
This PR re-routes traffic that originates from the local node, so that it goes to the Kubernetes service chain.
Is superfluous given the next sentence.
Comment on lines
+38
to
+50
| ```go | ||
| // Next, redirect all src-type=LOCAL -> LB IP to the service chain for externalTrafficPolicy=Local | ||
| // This allows traffic originating from the host to be redirected to the service correctly, | ||
| // otherwise traffic to LB IPs are dropped if there are no local endpoints. | ||
| args = append(args[:0], "-A", string(svcXlbChain)) | ||
| writeLine(proxier.natRules, append(args, | ||
| "-m", "comment", "--comment", fmt.Sprintf(`"masquerade LOCAL traffic for %s LB IP"`, svcNameString), | ||
| "-m", "addrtype", "--src-type", "LOCAL", "-j", string(KubeMarkMasqChain))...) | ||
| writeLine(proxier.natRules, append(args, | ||
| "-m", "comment", "--comment", fmt.Sprintf(`"route LOCAL traffic for %s LB IP to service chain"`, svcNameString), | ||
| "-m", "addrtype", "--src-type", "LOCAL", "-j", string(svcChain))...) | ||
| ``` | ||
|
|
Collaborator
There was a problem hiding this comment.
This renders weird.
Could you add surrounding context?
For example,
func DoSomething(args) error {
...
args = append(args[:0], "-A", string(svcXlbChain))
...
}Also, enough context to be able to tell what the args (is it a slice?) are, what svcXLBChain ()is it a string) is, what the proxier.natRules (this one i know is a struct from the block below) is etc.
| ```go | ||
| // Proxier is an iptables based proxy for connections between a localhost:lport | ||
| // and services that provide the actual backends. | ||
| type Proxier struct { |
Collaborator
There was a problem hiding this comment.
May be introduce this bit beforehand?
It helps make sense of the code above.
Comment on lines
+70
to
+75
| // endpointsChanges and serviceChanges contains all changes to endpoints and | ||
| // services that happened since iptables was synced. For a single object, | ||
| // changes are accumulated, i.e. previous is state from before all of them, | ||
| // current is state after applying all of those. | ||
| endpointsChanges *proxy.EndpointChangeTracker | ||
| serviceChanges *proxy.ServiceChangeTracker |
Collaborator
There was a problem hiding this comment.
Are all the fields here needed?
This write up only mentions some of of them.
Comment on lines
+160
to
+164
| ```go | ||
| writeLine(proxier.natRules, append(args, | ||
| "-m", "comment", "--comment", fmt.Sprintf(`"route LOCAL traffic for %s LB IP to service chain"`, svcNameString), | ||
| "-m", "addrtype", "--src-type", "LOCAL", "-j", string(svcChain))...) | ||
| ``` |
Collaborator
There was a problem hiding this comment.
it'd be cool if we could translate these snippets into the actual command being run.
Kinda like "hey, if you want to reproduce it yourself by hand, this is what is being done".
Comment on lines
+173
to
+181
| In this PR, we got a glimpse into the use of `NAT rules`, `iptables` and how the `iptabels proxier`, within Kubernetes, handles connections from local traffic, to the LB, to a service. | ||
|
|
||
| As a community, it is important to keep on learning and sharing the knowledge we gain from our experiences. | ||
|
|
||
| ``` | ||
| "We are only as strong as we are united, as weak as we are divided." | ||
|
|
||
| ― J.K. Rowling, Harry Potter and the Goblet of Fire | ||
| ``` No newline at end of file |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP PR for Issue #35
This was a tough one. Will likely need a few more edits. Please review @alejandrox1 😄