-
Notifications
You must be signed in to change notification settings - Fork 370
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
Add Node Selector in ACNP/ANP ingress/egress rules #3038
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3038 +/- ##
==========================================
- Coverage 65.75% 62.00% -3.76%
==========================================
Files 277 277
Lines 27264 37635 +10371
==========================================
+ Hits 17927 23334 +5407
- Misses 7439 12363 +4924
- Partials 1898 1938 +40
Flags with carried forward coverage won't be shown. Click here to find out more.
|
docs/antrea-network-policy.md
Outdated
@@ -526,6 +526,9 @@ since Pod IPs are ephemeral and unpredictable. | |||
select Fully Qualified Domain Names (FQDNs), specified either by exact name or wildcard | |||
expressions, when defining `egress` rules. | |||
|
|||
**nodeSelector**: This selects certain Node IPs as ingress from address or egress to address. | |||
It is applicable only to the `from` section in an `ingress` block or the `to` section in an `egress` block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to the existing doc for podSelector
and namespaceSelector
fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
docs/antrea-network-policy.md
Outdated
@@ -1062,6 +1065,37 @@ spec: | |||
- fqdn: "svcA.default.svc.cluster.local" | |||
``` | |||
|
|||
## Node Selector | |||
|
|||
NodeSelector selects certain Nodes which match the label selector. Add Node IPs to address group memberSet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NodeSelector selects certain Nodes which match the label selector. Add Node IPs to address group memberSet. | |
NodeSelector selects certain Nodes which match the label selector. It adds Node IPs to address group memberSet. |
And this is user facing doc, I'm not sure if users can understand "address group memberSet". At least there is no other fields mentioning it. Could you aligh with the description of other similar fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
docs/antrea-network-policy.md
Outdated
## Node Selector | ||
|
||
NodeSelector selects certain Nodes which match the label selector. Add Node IPs to address group memberSet. | ||
The following rule applied to Pods with label `app=antrea-test-app` and will `Drop` egress traffic to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following rule applied to Pods with label `app=antrea-test-app` and will `Drop` egress traffic to | |
The following rule applies to Pods with label `app=antrea-test-app` and will `Drop` egress traffic to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/antrea-network-policy.md
Outdated
|
||
NodeSelector selects certain Nodes which match the label selector. Add Node IPs to address group memberSet. | ||
The following rule applied to Pods with label `app=antrea-test-app` and will `Drop` egress traffic to | ||
Nodes which has labels `kubernetes.io/hostname=kind-control-plane`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nodes which has labels `kubernetes.io/hostname=kind-control-plane`. | |
Nodes which have the labels `kubernetes.io/hostname=kind-control-plane`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/antrea-network-policy.md
Outdated
- key: kubernetes.io/hostname | ||
operator: In | ||
values: | ||
- kind-control-plane |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a more proper example for your use case is:
matchExpressions:
- key: node-role.kubernetes.io/control-plane
operator: Exists
as hostname
can only match a single host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/apis/crd/v1alpha1/types.go
Outdated
@@ -15,6 +15,7 @@ | |||
package v1alpha1 | |||
|
|||
import ( | |||
_ "github.com/golang/mock/mockgen/model" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget to delete this. It is used in make codegen
. Done.
pkg/apis/crd/v1alpha1/types.go
Outdated
@@ -434,6 +435,11 @@ type NetworkPolicyPeer struct { | |||
// Exact FQDNs, i.e. "google.com", "db-svc.default.svc.cluster.local" | |||
// Wildcard expressions, i.e. "*wayfair.com". | |||
FQDN string `json:"fqdn,omitempty"` | |||
// Select certain Nodes which match the label selector, | |||
// if no nodeSelector is specified, then all Nodes will be selected | |||
// in the cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this behavior is expected. It basically breaks compaibility of existing rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
cdd18cd
to
fb11075
Compare
fb11075
to
99e08ee
Compare
456d0c7
to
f674958
Compare
docs/antrea-network-policy.md
Outdated
@@ -538,6 +539,10 @@ with `namespaces` field. | |||
specifies both namespaceSelector and podSelector selects particular Pods within | |||
particular Namespaces. | |||
|
|||
**nodeSelector**: This selects particular Nodes in cluster. | |||
The selected Node's IPs will set as "sources" if `nodeSelector` set in `ingress` section, or as "destinations" if set in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you align this lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
for _, addressGroupObj := range addressGroupObjs { | ||
addressGroup := addressGroupObj.(*antreatypes.AddressGroup) | ||
nS := addressGroup.Selector.NodeSelector | ||
if nS == nil && !addressGroup.SpanMeta.Has(node.Name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the addressGroup's span must contain this Node?
For example, if an addressGroup which contains Node A is applied to a Pod running on Node B, the addressGroup's span should contain only Node B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
pkg/controller/types/group.go
Outdated
groupSelector.NamespaceSelector, _ = metav1.LabelSelectorAsSelector(nsSelector) | ||
} | ||
|
||
if len(nodeSelectors) > 0 && nodeSelectors[0] != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a proof why variadic function doesn't apply here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
pkg/controller/types/group.go
Outdated
} | ||
|
||
if len(nodeSelectors) > 0 && nodeSelectors[0] != nil { | ||
groupSelector.NodeSelector, _ = metav1.LabelSelectorAsSelector(nodeSelectors[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when nodeSelector is present, no other fields including "Namespace" should be set to avoid ambiguity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
pkg/controller/types/group.go
Outdated
// the following format: "namespace=NamespaceName And podSelector=normalizedPodSelector". | ||
// Note: Namespace and nsSelector may or may not be set depending on the | ||
// selector. However, they cannot be set simultaneously. | ||
func generateNormalizedName(namespace string, podSelector, nsSelector, eeSelector labels.Selector) string { | ||
func GenerateNormalizedName(namespace string, podSelector, nsSelector, eeSelector labels.Selector, nodeSelectors ...labels.Selector) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
01279b8
to
67cc539
Compare
6f3842f
to
275530d
Compare
docs/antrea-network-policy.md
Outdated
NodeSelector selects certain Nodes which match the label selector. It adds Node IPs to egress rules in `to` field | ||
or ingress rules in `from` filed. | ||
The following rule applies to Pods with label `app=antrea-test-app` and will `Drop` egress traffic to | ||
Nodes which have the labels `node-role.kubernetes.io/control-plane`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: on TCP port 6443
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
oldIPs, err := k8s.GetNodeAddrs(oldNode) | ||
if err != nil { | ||
return false, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oldIPs, err := k8s.GetNodeAddrs(oldNode) | |
if err != nil { | |
return false, err | |
} | |
if oldIPs, err := k8s.GetNodeAddrs(oldNode); err != nil { | |
return false, err | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -115,10 +115,14 @@ func (n *NetworkPolicyController) toAntreaPeerForCRD(peers []v1alpha1.NetworkPol | |||
} else if peer.FQDN != "" { | |||
fqdns = append(fqdns, peer.FQDN) | |||
} else if peer.ServiceAccount != nil { | |||
normalizedUID := n.createAddressGroup(peer.ServiceAccount.Namespace, serviceAccountNameToPodSelector(peer.ServiceAccount.Name), nil, nil) | |||
normalizedUID := n.createAddressGroup(peer.ServiceAccount.Namespace, serviceAccountNameToPodSelector(peer.ServiceAccount.Name), nil, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not entirely related to your PR but comment on L98 needs update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
test/e2e/utils/cnpspecbuilder.go
Outdated
Name: name, | ||
AppliedTo: appliedTos, | ||
} | ||
b.Spec.Egress = append(b.Spec.Egress, newRule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is nodeSelector only supported for egress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing. Both egress and ingress rules, and will add more e2e test cases.
bbf96b4
to
90e87b5
Compare
docs/antrea-network-policy.md
Outdated
@@ -1102,6 +1107,35 @@ spec: | |||
- fqdn: "svcA.default.svc.cluster.local" | |||
``` | |||
|
|||
## Node Selector | |||
|
|||
NodeSelector selects certain Nodes which match the label selector. It adds Node IPs to egress rules in `to` field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
When used in the
to
field of an egress rule, it adds the Node IPs to the rule's destination address group; when used in thefrom
field of an ingress rule, it adds the Node IPs to the rule's source address group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
docs/antrea-network-policy.md
Outdated
@@ -545,6 +546,10 @@ with `namespaces` field. | |||
specifies both namespaceSelector and podSelector selects particular Pods within | |||
particular Namespaces. | |||
|
|||
**nodeSelector**: This selects particular Nodes in cluster. The selected Node's | |||
IPs will set as "sources" if `nodeSelector` set in `ingress` section, or as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will set as -> will be used as
set in -> is set in the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/apis/crd/v1alpha1/types.go
Outdated
@@ -439,6 +439,12 @@ type NetworkPolicyPeer struct { | |||
// Cannot be set with any other selector. | |||
// +optional | |||
ServiceAccount *NamespacedName `json:"serviceAccount,omitempty"` | |||
// Select certain Nodes which match the label selector, | |||
// if no nodeSelector is specified, then no additional Nodes IPs will be added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we can remove this sentence (line 443 - 444), as it does add useful information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, done.
pkg/apis/crd/v1alpha1/types.go
Outdated
@@ -439,6 +439,12 @@ type NetworkPolicyPeer struct { | |||
// Cannot be set with any other selector. | |||
// +optional | |||
ServiceAccount *NamespacedName `json:"serviceAccount,omitempty"` | |||
// Select certain Nodes which match the label selector, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
,
-> .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/controller/types/group.go
Outdated
@@ -49,27 +49,36 @@ type GroupSelector struct { | |||
// If Namespace and NamespaceSelector both are unset, it selects the ExternalEntities in all the Namespaces. | |||
// TODO: Add validation in API to not allow externalEntitySelector and podSelector in the same group. | |||
ExternalEntitySelector labels.Selector | |||
|
|||
// This is a label selector which selects certain Node IPs. Within a group NodeSelector cannot be set concurrently with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concurrently -> together
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/util/k8s/node.go
Outdated
@@ -57,3 +58,26 @@ func GetNodeAddrs(node *v1.Node) (*ip.DualStackIPs, error) { | |||
} | |||
return nodeAddrs, nil | |||
} | |||
|
|||
// GetNodeAddressFromAnnotations gets available IPs from the Node Annotation, the annotations are set by Antrea, includes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node Annotation, the -> Node Annotation. The
includes -> including
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/agent/agent.go
Outdated
@@ -641,6 +641,20 @@ func (i *Initializer) setupGatewayInterface() error { | |||
return err | |||
} | |||
|
|||
// Update the Node Antrea gateway IP address in Node's annotation. | |||
gwIPv4Addr, gwIPv6Addr := i.nodeConfig.GatewayConfig.IPv4, i.nodeConfig.GatewayConfig.IPv6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember we reach a consensus in the community meeting that we could just "predict" the gateway IPs as some implementation already does it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
} | ||
if newIPs, err = k8s.GetNodeAddrs(newNode); err != nil { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should return early if change is already identified.
And I think if there is an error, we should return changed as true. Imagine this scenario:
A Node doesn't have a valid Node IP because of a bug or it's not reported yet, after it gets an valid IP, why we don't reconcile the policy with the new IP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second thought, which might make more sense:
We don't need to care whether there is an error when getting oldIPs and newIPs. We should just focus on whether there is a change. If both get error, oldIPs and newIPs are both nil, there is no point to reconcile the rule as well. So the code could just be:
oldIPs, _ = k8s.GetNodeAddrs(oldNode)
newIPs, _ = k8s.GetNodeAddrs(newNode)
!reflect.DeepEqual(newIPs, oldIPs) {
return true
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -1091,18 +1061,36 @@ func (n *NetworkPolicyController) syncAddressGroup(key string) error { | |||
utilsets.MergeString(addrGroupNodeNames, internalNP.SpanMeta.NodeNames) | |||
} | |||
memberSet := n.getAddressGroupMemberSet(addressGroup) | |||
if addressGroup.Selector.NodeSelector != nil { | |||
ms, nodes := n.addNodeSelectorMemberSet(addressGroup.Selector.NodeSelector) | |||
memberSet = memberSet.Union(ms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think the API is already designed in this way. You should just update getAddressGroupMemberSet
to get node IPs when NodeSelector
is not nil, instead of still trying to get pod members first.
a618ab7
to
80fb279
Compare
/test-all |
/test-ipv6-all |
275530d
to
8a014a0
Compare
8e26562
to
8e9146f
Compare
The PR changes 171 files, please revert unrelated changes |
8e9146f
to
b3f2fcc
Compare
I have executed |
b3f2fcc
to
64bb552
Compare
Updated. |
64bb552
to
c61fb19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
c61fb19
to
8fad6f7
Compare
ae7d793
to
f7721e6
Compare
fbb0126
to
8adb079
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one nit
/test-all-features-conformance |
docs/antrea-network-policy.md
Outdated
3. The transport IP (the IP address of the interface used for tunneling or routing the traffic across Nodes) if it's different from Node IP | ||
|
||
Traffic to/from other IPs of the Node will be ignored. | ||
Meanwhile, `NodeSelector` doesn’t affect the traffic from Node to Pods running on that Node. Such traffic will always be allowed to make sure that [agents on a Node (e.g. system daemons, kubelet) can communicate with all Pods on that Node](https://kubernetes.io/docs/concepts/services-networking/#the-kubernetes-network-model) to perform liveness and readiness probes. For more information, see [https://github.com/antrea-io/antrea/pull/104](https://github.com/antrea-io/antrea/pull/104). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please break the line into multiple lines, aligning with other lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
test/e2e/antreapolicy_test.go
Outdated
Connected, | ||
}, | ||
{ | ||
Pod("z/" + clientName1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With or without NodeSelector, this client can access this server, what does it validate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case verified that the other Node to Pod traffic will not be affected by the nodeSelector rule.
However, using the third Node to verify this is more appropriate.
Meanwhile, it seems that there is no test case about local Node traffic to Pod, from this point of view, the test case makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testDefaultDenyIngressPolicy
tests probe traffic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
1. Add `nodeSelector` field in ACNP/ANP ingress/egress rules to restrict traffic to/from particular Nodes. 2. Add Node IPs info in the output of `antctl get addressgroup`. For example: ``` antctl get addressgroup NAME POD-IPS NODE-IPS 5e1bbf8e-67a3-5384-b2bf-e42e93bd68aa <NONE> 192.168.0.1, 10.176.27.105 f8c56571-d6db-51ec-9352-a9a47476a9a0 192.168.0.70,192.168.1.38,192.168.1.39 <NONE> antctl get addressgroup -oyaml - name: 5e1bbf8e-67a3-5384-b2bf-e42e93bd68aa nodes: - ip: 192.168.0.1, 10.176.27.105 node: name: wenqiq01-1 - name: f8c56571-d6db-51ec-9352-a9a47476a9a0 pods: - ip: 192.168.1.38 pod: name: iperf3-55bcff667d-v495l namespace: demo - ip: 192.168.1.39 pod: name: iperf3-55bcff667d-c74ll namespace: demo - ip: 192.168.0.70 pod: name: iperf3-55bcff667d-pz2d2 namespace: demo ``` Fixes: antrea-io#3023 Signed-off-by: Wenqi Qiu <[email protected]> Co-authored-by: Quan Tian <[email protected]>
54bb043
to
37b5c92
Compare
Rebase and squash the commits. |
Signed-off-by: Wenqi Qiu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
unit-test failed:
|
Support nodeSelector in ACNP/ANP ingress/egress rules.
Add
nodeSelector
field in ACNP/ANP ingress/egress rules to restrict traffic from/to particular Nodes.Add Node info in the output of
antctl get addressgroup
.For example:
Fixes: #3023
Signed-off-by: Wenqi Qiu [email protected]