Extend BPF policymap to include proxy_port#2918
Merged
tgraf merged 10 commits intocilium:masterfrom Mar 1, 2018
Merged
Conversation
7872866 to
9180cdd
Compare
5 tasks
9180cdd to
789b41e
Compare
789b41e to
8f4a7d3
Compare
The 'Action' field of a 'policyEntry' was never used to determine the action to execute on the datapath, but userspace always populated this field. Remove the userspace bits which populate the field so it can be reused in future. Signed-off-by: Joe Stringer <joe@covalent.io>
All of the current policy_can_access() calls are for ingress only. Indicate this and hide the map argument while we're at it. Signed-off-by: Joe Stringer <joe@covalent.io>
Update the reverse_proxy() final argument to pass the only field in the CT tuple which it uses, the nexthdr. This makes the IPv4 path more similar to the IPv6 path. Signed-off-by: Joe Stringer <joe@covalent.io>
Signed-off-by: Joe Stringer <joe@covalent.io>
Signed-off-by: Joe Stringer <joe@covalent.io>
Just before this gets hit on the ingress path in ipv4_policy(), the ct_state's orig_dport field gets updated to be the same as the tuple's dport. Replacing it here as well makes the code more consistent with the IPv6 codepath and should have no outward effect. Signed-off-by: Joe Stringer <joe@covalent.io>
Extricate proxy_port determination from conntrack. This will be replaced by a map lookup in future, so factoring it out helps to move it in that direction. Signed-off-by: Joe Stringer <joe@covalent.io>
Signed-off-by: Joe Stringer <joe@covalent.io>
If userspace populates the 'proxy_port' field of a policy entry, return it as part of policy_can_access_ingress(). Userspace can now populate this portion of policy map entries and the proxy port will be respected by the datapath. Related: cilium#2904 Signed-off-by: Joe Stringer <joe@covalent.io>
L4 policy will in future be tied in with the standard L3/L4 BPF map lookup, so shift this logic to there. In the case of egress policy, there is no blob there yet so just shift the proxy logic out of conntrack creation, closer to where it will be implemented when label-based+l4 egress policy is implemented. Signed-off-by: Joe Stringer <joe@covalent.io>
8f4a7d3 to
a7cf33d
Compare
tgraf
approved these changes
Mar 1, 2018
Contributor
tgraf
left a comment
There was a problem hiding this comment.
Awesome cleanup. Does the addition of the L4 policy lookup to the can_access() logic fix a bug or is required for the transition?
Member
Author
|
@tgraf At this point it's only what's required for the transition. I realised though that one minor tweak on top may fix #2907: With the current state, l4policy via the embedded maps is only triggered if there's an l4-only entry in the l3/l4 policy map. However, today userspace won't generate such entries. I was thinking that some reworking of the userspace would be necessary to fix the above bug, but we could just unconditionally attempt to use the embedded map if all the other cases fall through. I'll look into that, since it's one of the same core problems blocking #2954. |
Member
Author
|
Confirmed that userspace changes are necessary to fix #2907 without otherwise breaking Label-dependent L4, so I think that we should go ahead with this PR as-is. |
This was referenced Jul 3, 2018
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.
This PR removes the
actionfield from thepolicy_entryvalues that are held inPOLICY_MAP, as it was not being used (the map is a whitelist and there are no other actions to take other than allow). In its place, it introducesproxy_port. This lays the groundwork for userspace to populate this map instead of writing theBPF_L4_MAPembedded macro to specify proxy ports to redirect traffic towards.As part of this, the datapath logic is shifted around a bit to push the L4 policy lookup earlier, to occur alongside the Label-based, and Label-dependent L4, and CIDR policy lookups.
The existing
BPF_L4_MAPmethod for fetching theproxy_portwill still work for now, so this PR can be merged prior to the userspace work. Once userspace switches over to implementing L4-L7 policy via thePOLICY_MAP, all of theBPF_L4_MAPlogic can be removed.Note that this requires the
cilium_policy_*maps to be flushed upon cilium upgrade, otherwise all existing policies in the BPF map will cause forwarding to the local port 1 (interpreting the previousaction=1as aproxy_portto redirect to), and the kernel will reset those connections.This PR also rolls in a few unrelated style / documentation fixes for the datapath.
If you feel that the
actionfield should still exist in this entry, or that this kind of upgrade breakage is unacceptable, please voice your opinions below.