cfp: Adding the options to get the DNS rules#2
cfp: Adding the options to get the DNS rules#2hemanthmalla merged 2 commits intohemanthmalla:tofqdn_hafrom
Conversation
vipul-21
force-pushed
the
singhvipul/dnsrules
branch
12 times, most recently
from
7d2de42 to
3a908e2
Compare
vipul-21
force-pushed
the
singhvipul/dnsrules
branch
from
3a908e2 to
0911110
Compare
vipul-21
force-pushed
the
singhvipul/dnsrules
branch
3 times, most recently
from
6b74a85 to
71013e2
Compare
cilium/CFP-30984-dns-proxy-ha.md
Outdated
| Method : UpdatesDNSRules | ||
|
|
||
| _rpc UpdatesDNSRules(stream DNSPolicyRules) returns (Result){}_ | ||
| _rpc UpdatesDNSRules(Request) returns (stream DNSPolicyRules){}_ |
There was a problem hiding this comment.
Are we sure the flip is necessary here ?
There was a problem hiding this comment.
It seems more intuitive to me as DNSPolicyRules are being returned to the SDP.
There was a problem hiding this comment.
UpdatesDNSRules is intended to be invoked from agent when there is a policy update, so cannot flip the order right ?
There was a problem hiding this comment.
Yes, UpdateDNSRules is intended to invoked from agent. How would the agent know about invoking the SDP ? So in this case, SDP has the responsibility to tell the cilium agent that I need the rules, instead of Cilium agent trying to find to whom it should send the rules.(if that make sense)
There was a problem hiding this comment.
We introduced tracking policy revisions per SDP instance to deal with this. On a connection, agent always sends policy information, on ACK, we track it against the connection, every policy update resulting in a revision bump we notify all SDP instances. On agent restart, we start over.
cilium/CFP-30984-dns-proxy-ha.md
Outdated
|  | ||
|
|
||
|
|
||
| #### Option 2: Running the GRPC server in the SDP as well |
There was a problem hiding this comment.
If option 1 is possible, do we really need option 2 ?
There was a problem hiding this comment.
We tested this option too as well.
I added it keeping in mind that SDP may/may not handle policy in the future. So we might need to consider a grpc server in the SDP.
There was a problem hiding this comment.
SDP not handling policy would be violate the toFQDN spec right ?
There was a problem hiding this comment.
Sorry, I am not sure if I understand this. What do you mean by violating the toFQDN spec ?
There was a problem hiding this comment.
I meant to say that SDP not enforcing L7 DNS policy is not an option right ?
cilium/CFP-30984-dns-proxy-ha.md
Outdated
|
|
||
| ##### Cons | ||
|
|
||
| * Will change the current implmentation/flow of flitering the dns requests. |
There was a problem hiding this comment.
What changes wrt DNS requests ?
There was a problem hiding this comment.
I mean right now we filter based on the identities associated with the dns ip right ? If are writing to the file system similar to what is written currently i.e dns pod ip, the check would be to filter based on the ip rather than identities.
It depends on what we store in the file.
There was a problem hiding this comment.
Alright, after today's sync, we can consider updating this section to include relevant details from ep_policy.json
cilium/CFP-30984-dns-proxy-ha.md
Outdated
| * Will change the current implmentation/flow of flitering the dns requests. | ||
| * Reading a file might not be as efficient as reading from a gRPC server. | ||
|
|
||
| ### Q : Why did we choose grpc option over reading from filesystem? |
There was a problem hiding this comment.
If we understand the rationale / find a workaround for lazy update of DNS rules, ep_config.json might still be preferred ? We should probably leave this open ended and remove option 1 as recommended ?
There was a problem hiding this comment.
I think it depends on the performance of the both the options. Reading from the filesystem make sense to me in case of starting the SDP, but should that be default path to get the rules not sure.
The overhead to read/file watch on every endpoint maybe too much in high scale environment. I think if we can segregate just the dns rules to a different file that might be a bit better though.
vipul-21
force-pushed
the
singhvipul/dnsrules
branch
2 times, most recently
from
bfd15f0 to
a6545fd
Compare
vipul-21
force-pushed
the
singhvipul/dnsrules
branch
from
c4ccb8e to
74f2a75
Compare
|
|
||
| ## Impacts / Key Questions | ||
|
|
||
| ### Getting the DNS Rules |
There was a problem hiding this comment.
I think this section is now decently flushed out. I'm thinking we can move it from Impacts / Key Questions to the main proposal. We now have an agreement that this is how SDP should discover rules right.
hemanthmalla
left a comment
hemanthmalla
left a comment
There was a problem hiding this comment.
LGTM except for couple of nits. Will address them outside this PR
* cfp: Adding the options to get the DNS rules * Addressing comments Signed-off-by: Vipul Singh <[email protected]>
2 participants
No description provided.