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

Add Controlplane changes for NodeNetworkPolicy #5716

Merged
merged 1 commit into from
Dec 28, 2023

Conversation

Atish-iaf
Copy link
Contributor

For #5671

pkg/controller/grouping/controller.go Outdated Show resolved Hide resolved
pkg/controller/grouping/controller.go Outdated Show resolved Hide resolved
@@ -82,6 +84,12 @@ type Interface interface {
// DeleteExternalEntity deletes an ExternalEntity from the index. If any existing groups are affected, eventHandlers
// will be called with the affected groups.
DeleteExternalEntity(ee *v1alpha2.ExternalEntity)
// AddNode adds or updates a Node to the index. If any existing groups are affected, eventHandlers will be called with
// the affected groups.
AddNode(node *v1.Node)
Copy link
Member

Choose a reason for hiding this comment

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

When NodeSelector is used in AddressGroup, the current code just calculate the selected Nodes via c.nodeLister.List(selector), see

if g.Selector.NodeSelector != nil {
return n.getNodeMemberSet(g.Selector.NodeSelector)
}
return n.getMemberSetForGroupType(addressGroupType, g.Name)

This is based on the assumption that the scale of Node is smaller than Pod, and Node events that we care about are much less frequent.
I think this PR should do the same to be consistent and to avoid making the grouping interface more complicated until it's necessary.

@@ -1180,6 +1180,9 @@ func (n *NetworkPolicyController) getMemberSetForGroupType(groupType grouping.Gr
for _, ee := range externalEntities {
groupMemberSet.Insert(externalEntityToGroupMember(ee, true))
}
for _, node := range nodes {
groupMemberSet.Insert(nodeToGroupMember(node))
Copy link
Member

Choose a reason for hiding this comment

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

Like a Pod member in an AppliedToGroup, we don't need NodeIP in Node member when it's in an AppliedToGroup, otherwise the code needs to keep a Node group up-to-date when a Node IP changes but actually it's never used.

@@ -1342,6 +1345,9 @@ func (n *NetworkPolicyController) syncAppliedToGroup(key string) error {
memberSetByNode[entityNodeKey] = entitySet
appGroupNodeNames.Insert(entityNodeKey)
}
for _, node := range nodes {
appGroupNodeNames.Insert(node.Name)
Copy link
Member

Choose a reason for hiding this comment

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

Like Pod case, we should fill memberSetByNode to only send the Node itself to a Node. Otherwise it will have to resend the whole members everytime a Node joins or leaves the group.
Please have an unit test to validate it.

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

I think you should update build/charts/antrea/crds/clusternetworkpolicy.yaml to allow appliedTo to be set to nodeSelector, otherwise such policy can't be created

func NodeIPsIndexFunc(obj interface{}) ([]string, error) {
node, ok := obj.(*v1.Node)
if !ok {
return nil, fmt.Errorf("obj is not node: %+v", obj)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're checking if obj is of type *v1.Node. However, when the type assertion fails, consider providing more context in the error message. For example: "Expected *v1.Node, got %T"

@luolanzone luolanzone added this to the Antrea v1.15 release milestone Dec 6, 2023
@Atish-iaf Atish-iaf force-pushed the HNP-controller-changes branch 2 times, most recently from 6d988a2 to c888923 Compare December 12, 2023 12:02
@Atish-iaf Atish-iaf marked this pull request as ready for review December 12, 2023 12:14
@Atish-iaf Atish-iaf requested a review from tnqn December 12, 2023 12:14
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

Overall LGTM

tnqn
tnqn previously approved these changes Dec 21, 2023
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM
@hongliangl do you have other comments? Does it work with your PR?

pkg/apis/crd/v1beta1/types.go Outdated Show resolved Hide resolved
pkg/apiserver/openapi/zz_generated.openapi.go Outdated Show resolved Hide resolved
@hongliangl
Copy link
Contributor

hongliangl commented Dec 21, 2023

LGTM @hongliangl do you have other comments? Does it work with your PR?

LGTM overall. Just the json tag NodeSelector should be nodeSelector. If fixing this, it can work with my PR.

Copy link
Contributor

@hongliangl hongliangl left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn tnqn added action/release-note Indicates a PR that should be included in release notes. area/network-policy Issues or PRs related to network policies. labels Dec 28, 2023
@tnqn
Copy link
Member

tnqn commented Dec 28, 2023

/skip-all

@tnqn tnqn merged commit 9e9b073 into antrea-io:main Dec 28, 2023
51 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. area/network-policy Issues or PRs related to network policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants