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

datapath: support dhcpv4 discover as a valid lxc source ip #15980

Closed
wants to merge 1 commit into from

Conversation

oblazek
Copy link
Contributor

@oblazek oblazek commented May 3, 2021

Add a new datapath option which allows endpoint to send
a DHCP discover message which has been previously dropped by
"Invalid source ip" filter in lxc_bpf program which by default
allows only LXC_IPV4 as a source IP.

With this option endpoints like OpenStack vms can ask for IP
address using DHCP which is the standard way of IPAM in OS.

Signed-off-by: Ondrej Blazek ondra.blazkuj@gmail.com

Without these changes:

<- endpoint 2942 flow 0x0 identity 52569->0 state new ifindex 0 orig-ip 0.0.0.0: 0.0.0.0:68 -> 255.255.255.255:67 udp
xx drop (Invalid source ip) flow 0x0 to endpoint 0, identity 52569->0: 0.0.0.0:68 -> 255.255.255.255:67 udp

With these changes:

<- endpoint 3860 flow 0x0 identity 52569->0 state new ifindex 0 orig-ip 0.0.0.0: 0.0.0.0:68 -> 255.255.255.255:67 udp
-> stack flow 0x0 identity 52569->2 state new ifindex 0 orig-ip 0.0.0.0: 0.0.0.0:68 -> 255.255.255.255:67 udp
<- stack flow 0x0 identity 1->0 state new ifindex 0 orig-ip 0.0.0.0: 10.248.14.22:67 -> 10.247.2.17:68 udp
Policy verdict log: flow 0x0 local EP ID 3860, remote ID 1, proto 17, ingress, action allow, match L3-Only, 10.248.14.22:67 -> 10.247.2.17:68 udp

Add a new datapath option which allows endpoint to send
a DHCP discover message which has been previously dropped by
"Invalid source ip" filter in lxc_bpf program which by default
allows only LXC_IPV4 as a source IP.

With this option endpoints like OpenStack vms can ask for IP
address using DHCP which is the standard way of IPAM in OS.

Signed-off-by: Ondrej Blazek <ondra.blazkuj@gmail.com>
@oblazek oblazek requested a review from a team as a code owner May 3, 2021 11:35
@oblazek oblazek requested a review from a team May 3, 2021 11:35
@oblazek oblazek requested review from a team as code owners May 3, 2021 11:35
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 3, 2021
@pchaigno pchaigno added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels May 4, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 4, 2021
@pchaigno pchaigno self-requested a review May 4, 2021 22:06
@pchaigno
Copy link
Member

pchaigno commented May 6, 2021

With this option endpoints like OpenStack vms can ask for IP address using DHCP which is the standard way of IPAM in OS.

Could we somehow limit this to those endpoints? With k8s Pods we could maybe rely on annotations, but I'm not sure how that would work in your case, with OpenStack VMs.

@@ -544,13 +544,23 @@ static __always_inline int handle_ipv4_from_lxc(struct __ctx_buff *ctx,

tuple.nexthdr = ip4->protocol;

if (unlikely(!is_valid_lxc_src_ipv4(ip4)))
if (unlikely(!is_valid_lxc_src_ipv4(ip4))) {
#ifndef ENABLE_DHCP_MESSAGES
Copy link
Member

Choose a reason for hiding this comment

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

I think there are use cases for this besides DHCP, so maybe we should make it a bit more generic and allow all srcIP spoofing? I'm also worried of the impact is_valid_dhcpv4_message could have on the BPF complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean you are thinking about having this as a part of is_valid_lxc_src_ipv4 func - for all packets? Also does it make sense to you to switch #ifndef for #ifdef for better extensibility/readability?

Copy link
Member

Choose a reason for hiding this comment

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

I mean we should just accept packets if ENABLE_DHCP_MESSAGES is defined, regardless of whether or not they are DHCP packets.

@oblazek
Copy link
Contributor Author

oblazek commented May 7, 2021

Could we somehow limit this to those endpoints? With k8s Pods we could maybe rely on annotations, but I'm not sure how that would work in your case, with OpenStack VMs.

Well that's what RequireDHCPMessages option in datapath is about? Is that not enough? I mean that only specific endpoints like openstack vms will set that to enabled, others (k8s pods) do not need so those won't call is_valid_dhcpv4_message.

@pchaigno
Copy link
Member

pchaigno commented May 7, 2021

Could we somehow limit this to those endpoints? With k8s Pods we could maybe rely on annotations, but I'm not sure how that would work in your case, with OpenStack VMs.

Well that's what RequireDHCPMessages option in datapath is about? Is that not enough? I mean that only specific endpoints like openstack vms will set that to enabled, others (k8s pods) do not need so those won't call is_valid_dhcpv4_message.

I was wondering how to expose this to users, but I guess in your case, you simply use cilium config?

@oblazek
Copy link
Contributor Author

oblazek commented May 7, 2021

Well that's what RequireDHCPMessages option in datapath is about? Is that not enough? I mean that only specific endpoints like openstack vms will set that to enabled, others (k8s pods) do not need so those won't call is_valid_dhcpv4_message.

I was wondering how to expose this to users, but I guess in your case, you simply use cilium config?

We actually use it like this https://github.com/oblazek/cilium/blob/ob-openstack/plugins/cilium-openstack/plugin/bridge.go#L239

I have created a new "plugin" (https://github.com/oblazek/cilium/blob/ob-openstack/plugins/cilium-openstack/) which allows us to register openstack vms to cilium (with labels - metadata in case of openstack). It's not finished yet, but functional.

@pchaigno
Copy link
Member

I think we should close this PR in favor of #16134.

@oblazek oblazek closed this May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants