CFP-30984 : Add CFP for DNS proxy HA

[hemanthmalla]

Add CFP for cilium/cilium#30984

[vipul-21]

Did we benchmark both the options to get an idea for performance ?

[vipul-21]

Will there be a queue to renotify in case of failure to notify the agent ?

[tamilmani1989]

This is for redirecting packets either to SDP or CA dnsproxy. For updating dns->ip mappings it uses grpc channel and there will be retry on failures i guess

[joestringer]

Did this part get some more thought? I don't think it necessarily has to be in scope to solve this problem in the initial CFP, but I can see you're already thinking about this. It would be nice to at least clarify what the intended behavior is right now even if a better solution is planned for later. This is also what I'm thinking about from this thread below: https://github.com/cilium/design-cfps/pull/32/files/616ae893539fcab4a47e15de023215ddae46eec9#r1710516516 .

[joestringer]

Is this true? A key feature of the DNS proxy is learning DNS -> IP mappings and informing the policy engine about them, so that the policy engine inside the cilium-agent can calculate policy impacts, allocate identities for those IPs, and populate BPF policymaps based on the new identities. If we do not ensure policy is plumbed before releasing DNS responses to the clients, then there is a high chance to automatically impose ~1s latency on subsequent TCP connections, because the first SYN of the connection will be dropped due to policy and the networking stack will delay a second before retrying.

(We could mitigate this in a number of ways, but I think that if we explore the use of inter-process RPC in hotpath holistically then it may not be the suboptimal path)

[joestringer]

Maybe something that would help to explore this question would be if there's a more detailed breakdown of the expected lifecycle of policy computation and separately lifecycle of a DNS request+response path so we can analyze exactly what the dependencies are and how the handling of different events may impact the overall behaviour.

[tamilmani1989]

iiuc, there is no change to policy computation by agent. dns->ip mappings will be updated to agent via grpc api and agent does policy enforcement and only after getting ack from agent, dns response is returned back to client. Reading bpf maps is mainly for dnsproxy to get reconcile with ip->identity mappings and dnsproxy to bootup without relying on agent. @hemanthmalla correct me if i'm wrong

[joestringer]

I'm probably not reading the "agent doesn't need to process rpc calls in the hotpath" with the same set of assumptions then. @tamilmani1989 your description makes sense to me for the typical path when newly learned IP mappings are discovered, and for that specific scenario it sounds like the hotpath to me.

[hemanthmalla]

@joestringer I agree with you WRT the RPC method to update DNS<>IP mappings. gRPC overhead for this method will likely be low relative to the computation needed to process the request.

But I intended this section to discuss how SDP would discover metadata to enforce L7 DNS policy and requests in this context are rpc calls to fetch identity and endpoint mappings. If we fetch those directly from bpf I was thinking we can skip a couple of rpc calls.

I'll update this to qualify the "agent doesn't need to process rpc" part.

[joestringer]

Ah right. Yeah in terms of network handling latency the order of delay is likely to be local cache < syscall < rpc.

[joestringer]

Discussion(use-case): I don't see a recent update related to this.

As I understand, Hemanth's point is that L7 policy could plausibly be more efficient (albeit still with bpf map lookup syscalls in the hotpath rather than direct userspace cache like option 1b). But DNS L7 policy is a much less common use case than ToFQDN policies. For ToFQDNs the point is moot because we want to ensure policy plumbing occurs before continuing, so on average the agent ends up in the hotpath anyway.

[joestringer]

This option definitely imposes some security permissions requirements on the SDP, because fetching BPF map information requires certain privileges (at least CAP_BPF on newer systems). Maybe if SDP also relies on SO_REUSEPORT then the privilege level is already fairly high? Not sure if this is specifically a comment about this option for the key question or whether we may want to have a security posture section to document the threat model and privilege expectations for this new component.

(This is not necessarily new; there are parallels with the Envoy implementation as well, which itself already looks up the IPCache for IP->Identity information IIRC).

[tamilmani1989]

this helps solving dnsproxy to bootup without agent dependency

[joestringer]

What does the "atleast for names that are already resolved" part here mean?

Are you assuming this is a caching proxy or a passthrough proxy? Cilium currently only ever does passthrough AFAIK, and changing that could have implications on things like state coherency.

[tamilmani1989]

I think what he meant here is, with current dnsproxy if cilium agent down, then dns resolution fails and that impacts network connectivity. With Standalone dnsproxy, even if cilium agent is down, dns requests could be resolved and if it resolves to same IP, then existing bpf policy if any would be matched and connectivity works.

[hemanthmalla]

Ah. I understand how this is ambiguous. Will update this with "eliminate hard dependency for names that already have policy map entries in place". I do not intend to change the pass through aspect of the proxy for DNS, but if we are not accessing bpf maps directly for EP and identity info, then we would need to cache the mappings in SDP.

[joestringer]

Makes sense 👍

We may want to evaluate a userspace cache anyway, depending on the sort of performance characteristics we're looking for. For instance whether bpf map lookup syscalls have an impact on per-message handling latency. But I'm probably getting ahead of myself, that could always be explored or added later.

[tamilmani1989]

One requirement is to have a config option to disable Cilium Agent dnsproxy and run just SDP so that allows any delegated dns proxy to be plugged and also an option to reduce the memory/cpu of agent.

[joestringer]

I think it'd be worth putting a test plan in place to evaluate the impact of that. For instance I can see potential memory/cpu of agent decrease but that work would be transferred equivalently to SDP. Also SDP-only would have higher latency for enforcing policies for newly learned names.

[tamilmani1989]

Atleast for our usecase, we don't require 2 instances of dnsproxy to be running since we make sure SDP is available all the time even during upgrade. Also this allows user to run delegated (custom) dnsproxy as long as it interop with cilium agent.

[christarazi]

run multiple instances of SDP

How will multiple instances on the same node be managed? If we are assuming daemonsets, AFAIK you cannot scale them to multiple instances on the same node.

[tamilmani1989]

multiple instances refers to CA dnsproxy and SDP. In another instance, while upgrading SDP, there can be 2 instances of SDP running at same time and still design allows that to be handled.

[joestringer]

Discussion(use-case): I'm largely assuming that handling the DNS traffic directly in the agent will on average provide better performance characteristics, particularly because it is able to more easily trigger and handle policy reaction events and does not require encoding into the gRPC channel or the related scheduling to receive, handle, and respond to the messages.

With that in mind, what is the use case for two external proxies and disabling the in-built one? I ask because multiplexing DNS agents is additional complication to the implementation so it would be good to understand why that complexity is worthwhile.

[hemanthmalla]

See hemanthmalla#1 and hemanthmalla#2 for additional discussion on this.

[joestringer]

I reviewed afresh. I prefixed my feedback with either:

• Fixup: Easy change, typos, etc
• Discussion: Key questions I think we should be considering and probably resolving before marking the CFP as "implementable" (ie agreement in principle).
• Question: Something I wasn't sure about but I didn't necessarily expect it to result in a change unless you think the question highlights a key aspect
• Brainstorm: Throwing ideas out in case they are useful for discussion.
• nit: Low consequence side comment, take them or leave them.

Here's some of the highlights of what I'm thinking about:

• Upgrade. What's the scope of expected scenarios in which you would get HA? I see upgrade being out of scope, but then let's say you have Cilium X.Y.Z, do you require SDP X.Y.Z to get HA, then as soon as you get a patch update, upgrade is no longer considered and HA is lost until all components are upgraded? Or are you expecting stability by default within a patch release series? Is that with arbitrary agent/SDP versions as long as X.Y matches?
• Complexity: Is it possible for some aspects of the solution to be simplified, notably the use of three different sources of truth? Do we need the state transfer via file, and if yes, does it have to be an API between the components or could it be handled completely by the DNS proxy?
• Related... use cases: Some aspects of the design are trying to solve HA when HA is already broken because both components went down.
• Lifecycle: I didn't really follow the argument to disable the agent proxy. I'm not against it, I just don't think that any of the arguments really motivate the work. Maybe I'm missing something.

As a matter of process and to facilitate discussion since I can see that even my one review added 30+ comments... I would be open to amending the current PR with TODOs for key points we think need to be resolved, and merging the PR maybe with draft state or something like that. It might make it a bit easier to divide up discussion and review on some aspects in subsequent PRs. That said, I'm not too fussed either way. If it helps, we can do it, if it doesn't help, we can continue as-is. We've been discussing how the process should work to provide good developer feedback and experience over in #42 so I am thinking about the meta aspect of process and how we make it easier to contribute and discuss aspects of the design, ideally while keeping reviews and discussion manageable.

[joestringer]

Fixup: Is step 8 accurate in this diagram?

[vipul-21]

You are right, the step 8 should say Response for the request sent by the pod not DNS response.
I think step 8 can be removed too as policy calculation for a request/response to a particular fqdn is not something we are changing as a part of this CFP. (We are only considering the DNS request.)

[joestringer]

Discussion(upgrade): What are the backwards compatibility expectations imposed by this goal?

The reason I ask is that for the most part we assume that the bpf maps keys and values can be modified upon upgrade. For conntrack we typically don't do this as changing the map can be lossy and we don't currently have a good way to migrate that data, but for other map types we can and do delete bpf maps upon upgrade and then repopulate them from userspace, sometimes even with different key/value types.

I recognize that upgrade for SDP is marked as a future milestone so we may not need to resolve that in this current CFP as-is before merging as "Implementable", but then part of the question is---what does it mean for upgrade or mixed agent/SDP versions to be not a valid configuration? Do we require minor version matches and how will the SDP be designed to properly interpret bpf map content other than by first detecting the format (maybe BTF can play a role here?) and then subsequently either handling the content or failing out due to version mismatch.

[tamilmani1989]

afaik, envoy also reads bpf maps directly. How its being handled there? can follow the same.

[joestringer]

I guess I ended up with the same concern down in the other thread below, maybe we can converge there: https://github.com/cilium/design-cfps/pull/32/files/616ae893539fcab4a47e15de023215ddae46eec9#r1714521132

[joestringer]

Fixup: This section would benefit from describing the expected call path behaviour. That is to say, which component is calling this RPC on which other component? There is also the nuts and bolts about how the gRPC stream is opened, who initiates, security protections for the socket and so on. This can be a brief sentence for each call.

[vipul-21]

Make sense, will add the brief description.

[joestringer]

Discussion(use-case): I'm largely assuming that handling the DNS traffic directly in the agent will on average provide better performance characteristics, particularly because it is able to more easily trigger and handle policy reaction events and does not require encoding into the gRPC channel or the related scheduling to receive, handle, and respond to the messages.

With that in mind, what is the use case for two external proxies and disabling the in-built one? I ask because multiplexing DNS agents is additional complication to the implementation so it would be good to understand why that complexity is worthwhile.

[joestringer]

nit: Why is this a pro? Seems like just an implementation detail. (I don't necessarily think it's a bad thing, the text just doesn't identify the assumption about why this is a benefit vs an alternative.)

[vipul-21]

We thought about different ways of sending the DNS rules to SDP.
One way was sending rules to a particular ip:port and in that case, Cilium agent has the responsibility to find if SDP is running and then send out the rules.

In this case, where SDP is connecting to the agent. It is the responsibility of the SDP to make sure it connects to agent rather than other way around.
This reduces the overhead for the cilium agent to look out for processes(in case of multiple instances of proxies running) and send them rules.

[joestringer]

I read "Simpler to configure and scale out SDP instances since the agent only needs a socket to be configured and does not need to explicitly reference the SDP instances". That's a pro 👍

[joestringer]

How do you anticipate that DNS packets will flow at stable state? Do you have specific ideas in mind, prefer the agent / split evenly / something more fancy?

[hemanthmalla]

We can start with split evenly (default with SO_REUSEPORT). Eventually we can consider adding a bpf prog to control the socket selection from the reuseport group. I have a standalone PoC for this here https://github.com/hemanthmalla/reuseport_ebpf/blob/main/bpf/reuseport_select.c

[joestringer]

Would be good to mention default balanced in the CFP and add reuseport idea to future milestones?

[hemanthmalla]

Sounds good. Will update the CFP to include this.

[joestringer]

Question: I'm not super familiar with gRPC types here but I assume there's away that the bytes lists can encode variable length and hence IPv4 + IPv6 mappings?

[vipul-21]

Yes, you are right. It can encode ipv4 + ipv6 mappings.
We might need to address how we read from the stream in case there is huge chunk of data. But that would be more of implementation details.

[joestringer]

OK. Not sure if we specifically need to note this down for implementation phase or you'll track it anyway - feel free to either add a note into the CFP or mark this comment as resolved.

[vipul-21]

Will add a note to the CFP itself.

[joestringer]

Fixup: I note that the other API below shares endpoint_id between cilium-agent and dnsproxy, but this API goes for client_ip instead which I assume has a 1:1 mapping. Is there some context behind that?

(I suspect we do need the client context associated with mappings to keep them properly separated, though I couldn't specifically explain why right now, I just recall that's how we structure it in cilium-agent... I'm sure there's good reason)

[vipul-21]

The other API is sending the endpoint_id as that is used as a key in dns rules map lookup.(at the dns proxy end)
We can send the endpoint_id as well here and retrieve the ip from the endpoint id. We need both ip and endpoint id at the cilium agent end so either way should work.
We kept it as client_ip as that is what we are getting we get the DNS response. If we need to send the endpoint_id, there will a subsequent call either in local cache/bpf map to get the ip to endpoint id mapping.

[joestringer]

fixup: Is this the FQDNSelector or port_rules? typo?

[joestringer]

related, what's the difference between this and the selector_string?

[vipul-21]

Yes, we should rename the FQDNSelector to PortRules. (since they FqdnSelector is used for rules in cilium code base, I believe that is why it was kept like that)
The selector_string is the result of the function String()(method used to create a string based on MatchName/MatchPattern) . It is used as a key for the map store in dnsproxy codebase.

[joestringer]

Brainstorm: Something I'm struggling a little bit with these fields is whether they are the logical parts of rules at a specific level of abstraction, or whether it's taking Cilium internals and converting them into public API. In the latter case the concern I have is that we may end up constraining the way that future Cilium versions work because the API is too tied to the implementation details from today.

[tamilmani1989]

valid concern. we should name in generic and not tied to cilium internals

[joestringer]

Question: I can see some terminology here leaking over from Cilium internals. It may be the case that the API makes sense to export these things but it does make me wonder exactly how well abstracted the underlying mechanisms are, and to what degree this API is baking in expectations about the Cilium implementation. For instance how is match_labels different from selections? Are they both needed? What assumptions are we making about how Cilium's internals behaves and what it will need to do in order to properly inform the FQDN proxy about what it should do? How did you come up with this specific list of parameters, and have you compared it with how Envoy handles L7 policy?

[joestringer]

To some degree, this is getting into implementation details and we don't necessarily need to resolve these prior to merging the CFP as "implementable". I mainly raise these because the API is provided here in the CFP. Ultimately though the API design probably needs some dedicated consideration and some of the implementation details may not be known until a PR is opened, so I don't know whether it makes sense to drill deeper on these aspects here in the CFP or defer to the Code PR.

[vipul-21]

We can name them in more generic way so that we don't tie it with the cilium internals. Let me update those.

[hemanthmalla]

This PR in the current state got into implementation details, included optimizations that aren't really necessary for the core HA part and also leaked some internal abstractions into the API. We significantly trimmed the CFP down and reworked the API in the new iteration. It also discusses how and when information is exchanged in simple bullet points.

The idea is to focus on getting consensus for the contract between SDP and agent first and defer other optimizations to a later CFP.

cc @joestringer @tamilmani1989 @vipul-21