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

[NPM Lite] Support Network Policies Through CNS #3287

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

rejain456
Copy link
Contributor

Reason for Change:

As part of adding default deny so pods can't communicate with one another when network policies are not present, this pr is part 3 which updates the cni code, retrieving the default deny acl from cns and creating HNS endpoints of the ACL's for the pods

Issue Fixed:

Requirements:

Notes:

@rejain456 rejain456 requested review from a team as code owners December 20, 2024 23:58
@rejain456 rejain456 requested a review from a team as a code owner December 24, 2024 02:23
@rejain456 rejain456 changed the title [NPM Lite] Default Deny CNI Changes [NPM Lite] Support Default Deny Network Policies Through CNS Jan 10, 2025
@tamilmani1989 tamilmani1989 changed the title [NPM Lite] Support Default Deny Network Policies Through CNS [NPM Lite] Support Network Policies Through CNS Jan 10, 2025
cni/network/invoker_cns_test.go Show resolved Hide resolved
cni/network/network.go Outdated Show resolved Hide resolved
@paulyufan2
Copy link
Contributor

can we add comment to the description that this change is only applied on windows

cni/network/network.go Outdated Show resolved Hide resolved
tamilmani1989
tamilmani1989 previously approved these changes Jan 10, 2025
@paulyufan2 paulyufan2 added the cni Related to CNI. label Jan 10, 2025
paulyufan2
paulyufan2 previously approved these changes Jan 10, 2025
@paulyufan2
Copy link
Contributor

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rejain456 rejain456 dismissed stale reviews from paulyufan2 and tamilmani1989 via ab735cc January 10, 2025 23:15
@rejain456
Copy link
Contributor Author

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rejain456
Copy link
Contributor Author

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@QxBytes QxBytes left a comment

Choose a reason for hiding this comment

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

Could you also add a test in cni/network/network_windows_test.go > TestPluginWindowsAdd (modify either test) such that the returned cns response includes a different endpoint policy to be returned for each response and then confirm that it propagates to the network and endpoint policies fields in the respective wanted EndpointInfo structs properly?

@@ -616,6 +616,7 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {

natInfo := getNATInfo(nwCfg, options[network.SNATIPKey], enableSnatForDNS)
networkID, _ := plugin.getNetworkID(args.Netns, &ifInfo, nwCfg)
policies = append(policies, ipamAddResult.interfaceInfo[key].EndpointPolicies...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is only one "policies" variable won't this keep appending policies from each interface info such that the last item in this loop will have a "policies" field equal to all policies from all interfaceinfos?

Like if in the first iteration I loop over the infra nic and have a deny policy, it'll get added to the policies variable. Then in the second iteration if I then loop over a delegated nic, policies will still contain the default deny but now it will be applied to the delegated nic right?

If these policies are specific to this particular interface info maybe they can be placed here? https://github.com/Azure/azure-container-networking/pull/3287/files#diff-a2f1c51b6e94c2d11383e394bdae9391a1cdef8e6b6ff24210e4ff04f7692f5eR836 .

Alternatively we can create a new endpoint policies variable so the original "policies" variable isn't overwritten after each iteration of the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the suggestion, updated the code

@rejain456 rejain456 force-pushed the jainriya/npmliteCNIchange branch from b799db1 to 19c40d7 Compare January 15, 2025 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cni Related to CNI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants