Adds support for MultiNetworkPolicy definitions #907
Conversation
We want to be able to apply NetworkPolicy definitions to experiment pods, but your CNI plugin needs to support applying NetworkPolicy definitions. We use both flannel and multus, either of which support this. However, there is another effort from the k8s Network Plumbing Working Group (same people who maintain multus) to implement a system that allows NetworkPolicy definitions to pods with multiple interfaces. https://github.com/k8snetworkplumbingwg/multi-networkpolicy https://github.com/k8snetworkplumbingwg/multi-networkpolicy-iptables
It was previously v0.2.0, which is what index2ip was designed around, but I discovered that using that version was impacting multus' functionality with regard to annotating pods correctly, and possibly in other ways we weren't even noticing.
This version introduces the "endPort" field, allowing you to specify a port range, which we need to do for ndt5. Also, allow additional ports for ndt: 3001, 3010, and ephemeral port range. The range reflects the value of net.ipv4.ip_local_port_range on our Ubuntu systems.
It was not valid in that location.
Also updates ICMP flags to use "accept" instead of "allow"
After adding multiple MultiNetworkPolicies the multi-networkpolicy DaemonSets seems to not be applying the rules properly. I had figured that the NetworkAttachmentDefinition would be sufficient. This may or may not change/fix anything.
I had accidentally inserted some plain JSON.
It turns out the issue was apparently just me pushing changes to k8s-support in sandbox with the multi-networkpolicy changes, and then another branch without, and then with, and then without, which got pod annotations all messed up.
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @nkinkade)
config/multi-networkpolicy.jsonnet line 13 at r1 (raw file):
}, data: { 'custom-v4-rules.txt': '# accept redirect\n-p icmp --icmp-type redirect -j ACCEPT\n# accept fragmentation-needed (for MTU discovery)\n-p icmp --icmp-type fragmentation-needed -j ACCEPT\n',
I'm imagining these are really important commands for the network policy to work as intended.
Please use ||| for multiline, inline text so the content/script commands are easier to read.
k8s/custom-resource-definitions/multi-networkpolicy.jsonnet line 1 at r1 (raw file):
{
You said this was essentially copied from an upstream source unmodified?
Was any part modified?
k8s/networkpolicies/msak.jsonnet line 1 at r1 (raw file):
{
Since these network policies are so tightly related to the service configuration, is there a way we can imagine this being integrated into the k8s/daemonsets/experiments configs? Otherwise, it could be confusing updating the service but not knowing about or forgetting the network policy to allow a new port. Also, these aren't expected to change often so this could be long term rather than short term priority.
I could imagine this two ways. One is just an inline declaration like this. Second, it could be added as part of the Experiment template that passes a set of parameters to the relevant MultiNetworkPolicy object.
The rules are not utilized by multi-networkpolicy unless you pass special flags, which are outlined in a comment in the file.
These are now handled by a new template, and called from each experiment manifest.
The imports for experiments are now an array consisting of the experiment DaemonSet definition and it's corresponsing MultiNetworkPolicy definition.
The MultiNetworkPolicy CRD schema doesn't allow for that field to be a string, unlike "port", which is odd, but whatever.
There was a problem hiding this comment.
@stephen-soltesz: PTAL?
Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)
config/multi-networkpolicy.jsonnet line 13 at r1 (raw file):
Previously, stephen-soltesz (Stephen Soltesz) wrote…
I'm imagining these are really important commands for the network policy to work as intended.
Please use
|||for multiline, inline text so the content/script commands are easier to read.
They actually aren't important, or even used. The custom rules are not applied unless you pass special flags to multi-networkpolicy:
--custom-v4-igress-rule-file
--custom-v4-egress-rule-file
--custom-v6-igress-rule-file
--custom-v4-egress-rule-file
I removed the sample rules, expanded the ConfigMap data section, and added a comment about how to use them. Even though they are unused I left the ConfigMap in place just to facilitate adding custom rules in the future.
k8s/custom-resource-definitions/multi-networkpolicy.jsonnet line 1 at r1 (raw file):
Previously, stephen-soltesz (Stephen Soltesz) wrote…
You said this was essentially copied from an upstream source unmodified?
Was any part modified?
The only change I made was this:
--- scheme.yml 2024-10-07 14:50:42.090879884 -0600
+++ scheme.yml.new 2024-10-07 14:51:22.596794739 -0600
@@ -15,7 +15,7 @@
versions:
- name: v1beta1
served: true
- storage: true
+ storage: false
schema:
openAPIV3Schema:
description: "MultiNetworkPolicy is a CRD schema to provide NetworkPolicy
@@ -454,8 +454,8 @@
type: object
type: object
- name: v1beta2
- served: false
- storage: false
+ served: true
+ storage: true
schema:
openAPIV3Schema:
description: "MultiNetworkPolicy is a CRD schema to provide NetworkPolicy
... basically to enable v1beta2, and make it the default stored version.
k8s/networkpolicies/msak.jsonnet line 1 at r1 (raw file):
Previously, stephen-soltesz (Stephen Soltesz) wrote…
Since these network policies are so tightly related to the service configuration, is there a way we can imagine this being integrated into the k8s/daemonsets/experiments configs? Otherwise, it could be confusing updating the service but not knowing about or forgetting the network policy to allow a new port. Also, these aren't expected to change often so this could be long term rather than short term priority.
I could imagine this two ways. One is just an inline declaration like this. Second, it could be added as part of the
Experimenttemplate that passes a set of parameters to the relevant MultiNetworkPolicy object.
I added a new MultiNetworkPolicy template to templates.jsonnet, and in each experiment manifest there is now a ports variable that gets passed to the call to the new template. The experiment jsonnet files now contain both the DaemonSet definition and the experiment's corresponding MultiNetworkPolicy. Does this satisfy your concern? The two things are now more tightly coupled and configured in the same file.
There was a problem hiding this comment.
This is great. I think this looks good to me. What else is merging conditional on?
Reviewed 1 of 13 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @nkinkade)
k8s/custom-resource-definitions/multi-networkpolicy.jsonnet line 1 at r1 (raw file):
Previously, nkinkade wrote…
The only change I made was this:
--- scheme.yml 2024-10-07 14:50:42.090879884 -0600 +++ scheme.yml.new 2024-10-07 14:51:22.596794739 -0600 @@ -15,7 +15,7 @@ versions: - name: v1beta1 served: true - storage: true + storage: false schema: openAPIV3Schema: description: "MultiNetworkPolicy is a CRD schema to provide NetworkPolicy @@ -454,8 +454,8 @@ type: object type: object - name: v1beta2 - served: false - storage: false + served: true + storage: true schema: openAPIV3Schema: description: "MultiNetworkPolicy is a CRD schema to provide NetworkPolicy... basically to enable v1beta2, and make it the default stored version.
Thank you!
k8s/daemonsets/core/multi-networkpolicy.jsonnet line 50 at r2 (raw file):
'/usr/bin/multi-networkpolicy-iptables', ], args: [
Clarifying: this is where someone might add --custom-v4-igress-rule-file and the others? If so, please make that explicit. The earlier file mentions "flags to multi-networkpolicy" and I see this is "multi-networkpolicy-iptables". If it's not this, where are the "multi-networkpolicy" flags?
k8s/networkpolicies/msak.jsonnet line 1 at r1 (raw file):
Previously, nkinkade wrote…
I added a new MultiNetworkPolicy template to templates.jsonnet, and in each experiment manifest there is now a
portsvariable that gets passed to the call to the new template. The experiment jsonnet files now contain both the DaemonSet definition and the experiment's corresponding MultiNetworkPolicy. Does this satisfy your concern? The two things are now more tightly coupled and configured in the same file.
This is great!
There was a problem hiding this comment.
I don't believe there is anything blocking or pending that prevents this from being merged.
Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)
k8s/custom-resource-definitions/multi-networkpolicy.jsonnet line 1 at r1 (raw file):
Previously, stephen-soltesz (Stephen Soltesz) wrote…
Thank you!
If/when v1beta2 gets fixed/deployed upstream, then we could possible get rid of this entire file and simply apply the CRD using the URL of the upstream schema.yml file.
k8s/daemonsets/core/multi-networkpolicy.jsonnet line 50 at r2 (raw file):
Previously, stephen-soltesz (Stephen Soltesz) wrote…
Clarifying: this is where someone might add
--custom-v4-igress-rule-fileand the others? If so, please make that explicit. The earlier file mentions "flags to multi-networkpolicy" and I see this is "multi-networkpolicy-iptables". If it's not this, where are the "multi-networkpolicy" flags?
That is correct. The args array in this file is where those flags would go to define custom rules. I added the flags, but commented them out, and additionally added a small explanation. I have also updated config/multi-networkpolicy.jsonnet with additional comments to clarify how all this works.
There was a problem hiding this comment.
Reviewed 3 of 11 files at r1, 7 of 13 files at r2, 2 of 2 files at r3.
Reviewable status: 0 of 1 approvals obtained
k8s/daemonsets/core/multi-networkpolicy.jsonnet line 50 at r2 (raw file):
Previously, nkinkade wrote…
That is correct. The args array in this file is where those flags would go to define custom rules. I added the flags, but commented them out, and additionally added a small explanation. I have also updated config/multi-networkpolicy.jsonnet with additional comments to clarify how all this works.
Beautiful!
This PR deploys the multi-networkpolicy DaemonSet, which reads in a set of MultiNetworkPolicy objects and enforces them via iptables rules in each pod. A MultiNetworkPolicy is almost identical to the native k8s NetworkPolicy, with a couple of very small differences. MultiNetworkPolicies are designed to work in conjunction with the
multusCNI plugin and NetworkAttachmentDefinitions. The multi-networkpolicy DaemonSet will apply a certain MultiNetworkPolicy to any pod with an annotation that matches the NetworkAttachmentDefinition specified in the'k8s.v1.cni.cncf.io/policy-forannotation of the MultiNetworkPolicy.More infomation:
https://github.com/k8snetworkplumbingwg/multi-networkpolicy
https://github.com/k8snetworkplumbingwg/multi-networkpolicy-iptables
multi-networkpolicy-iptables does not yet support the v1beta2 API version, which added support for the
endPortfield in a port specification, which allows you to define a port range. We require this functionality to support ndt5 testing on ephemeral ports. I forked the repository and made the changes necessary to support v1beta2:k8snetworkplumbingwg/multi-networkpolicy-iptables@master...m-lab:multi-networkpolicy-iptables:v1beta2
The changes seems to work, though I have not yet tried to add any unit tests.
This change is