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.

Sign up for GitHub

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
+88 −15
Open

[NPM Lite] Support Network Policies Through CNS #3287

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c0130eb
added npm lite default deny cni changes
rejain456 Dec 19, 2024
4d4eab1
added changes for unit tests
rejain456 Dec 21, 2024
4a4c990
fixed test message
rejain456 Dec 23, 2024
162714b
moved default deny acl under interfaceinfo
rejain456 Dec 24, 2024
315a544
reverted a change in network
rejain456 Dec 24, 2024
fc347fd
removed a logging line
rejain456 Dec 24, 2024
4aa3a54
added a new line
rejain456 Dec 24, 2024
86311a7
updated unit test
rejain456 Dec 24, 2024
8b79796
moved cni kv pair to common folder
rejain456 Jan 6, 2025
49da7b6
updated cni code to match network container contract update
rejain456 Jan 9, 2025
a8a4164
updated unit test case
rejain456 Jan 10, 2025
9ad9b5e
updated unit test to add cns not sending default deny acl to cni
rejain456 Jan 10, 2025
adcabcd
removed an infra nic check
rejain456 Jan 10, 2025
9ca7d88
removed an infra nic check
rejain456 Jan 10, 2025
ab735cc
removed for loop
rejain456 Jan 10, 2025
4cc444c
removed an extra spacing
rejain456 Jan 10, 2025
19c40d7
Merge branch 'master' into jainriya/npmliteCNIchange
rejain456 Jan 13, 2025
227be2f
update from pr comment
rejain456 Jan 16, 2025
0efb03f
updated unit test
rejain456 Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cni/network/invoker_cns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/Azure/azure-container-networking/iptables"
"github.com/Azure/azure-container-networking/network"
"github.com/Azure/azure-container-networking/network/networkutils"
"github.com/Azure/azure-container-networking/network/policy"
cniSkel "github.com/containernetworking/cni/pkg/skel"
"github.com/pkg/errors"
"go.uber.org/zap"
Expand Down Expand Up @@ -55,6 +56,7 @@ type IPResultInfo struct {
skipDefaultRoutes bool
routes []cns.Route
pnpID string
endpointPolicies []policy.Policy
}

func (i IPResultInfo) MarshalLogObject(encoder zapcore.ObjectEncoder) error {
Expand Down Expand Up @@ -159,6 +161,7 @@ func (invoker *CNSIPAMInvoker) Add(addConfig IPAMAddConfig) (IPAMAddResult, erro
skipDefaultRoutes: response.PodIPInfo[i].SkipDefaultRoutes,
routes: response.PodIPInfo[i].Routes,
pnpID: response.PodIPInfo[i].PnPID,
endpointPolicies: response.PodIPInfo[i].EndpointPolicies,
}

logger.Info("Received info for pod",
Expand Down Expand Up @@ -444,6 +447,7 @@ func configureDefaultAddResult(info *IPResultInfo, addConfig *IPAMAddConfig, add
Gw: ncgw,
})
}

// if we have multiple infra ip result infos, we effectively append routes and ip configs to that same interface info each time
// the host subnet prefix (in ipv4 or ipv6) will always refer to the same interface regardless of which ip result info we look at
addResult.interfaceInfo[key] = network.InterfaceInfo{
Expand All @@ -452,6 +456,7 @@ func configureDefaultAddResult(info *IPResultInfo, addConfig *IPAMAddConfig, add
IPConfigs: ipConfigs,
Routes: resRoute,
HostSubnetPrefix: *hostIPNet,
EndpointPolicies: info.endpointPolicies,
}
}

Expand Down
81 changes: 66 additions & 15 deletions cni/network/invoker_cns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/Azure/azure-container-networking/cns"
"github.com/Azure/azure-container-networking/iptables"
"github.com/Azure/azure-container-networking/network"
"github.com/Azure/azure-container-networking/network/policy"
cniSkel "github.com/containernetworking/cni/pkg/skel"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -521,14 +522,38 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
hostSubnetPrefix *net.IPNet
options map[string]interface{}
}
valueOut := []byte(`{
"Type": "ACL",
"Action": "Block",
"Direction": "Out",
"Priority": 10000
}`)

valueIn := []byte(`{
"Type": "ACL",
"Action": "Block",
"Direction": "In",
"Priority": 10000
}`)

expectedEndpointPolicies := []policy.Policy{
{
Type: policy.EndpointPolicy,
Data: valueOut,
},
{
Type: policy.EndpointPolicy,
Data: valueIn,
},
}
tests := []struct {
name string
fields fields
args args
wantDefaultResult network.InterfaceInfo
wantMultitenantResult network.InterfaceInfo
wantErr bool
name string
fields fields
args args
wantDefaultDenyEndpoints bool
wantDefaultResult network.InterfaceInfo
wantMultitenantResult network.InterfaceInfo
wantErr bool
}{
{
name: "Test happy CNI add",
Expand Down Expand Up @@ -559,7 +584,8 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
PrimaryIP: "10.0.0.1",
Subnet: "10.0.0.0/24",
},
NICType: cns.InfraNIC,
NICType: cns.InfraNIC,
EndpointPolicies: expectedEndpointPolicies,
},
},
Response: cns.Response{
Expand Down Expand Up @@ -588,6 +614,7 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
Gateway: net.ParseIP("10.0.0.1"),
},
},
EndpointPolicies: expectedEndpointPolicies,
Routes: []network.RouteInfo{
{
Dst: network.Ipv4DefaultRouteDstPrefix,
Expand All @@ -597,7 +624,8 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
NICType: cns.InfraNIC,
HostSubnetPrefix: *parseCIDR("10.0.0.0/24"),
},
wantErr: false,
wantDefaultDenyEndpoints: true,
wantErr: false,
},
{
name: "Test CNI add with pod ip info empty nictype",
Expand Down Expand Up @@ -665,7 +693,8 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
NICType: cns.InfraNIC,
HostSubnetPrefix: *parseCIDR("10.0.0.0/24"),
},
wantErr: false,
wantDefaultDenyEndpoints: false,
wantErr: false,
},
{
name: "Test happy CNI add for both ipv4 and ipv6",
Expand Down Expand Up @@ -696,7 +725,8 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
PrimaryIP: "10.0.0.1",
Subnet: "10.0.0.0/24",
},
NICType: cns.InfraNIC,
NICType: cns.InfraNIC,
EndpointPolicies: expectedEndpointPolicies,
},
{
PodIPConfig: cns.IPSubnet{
Expand All @@ -716,7 +746,8 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
PrimaryIP: "fe80::1234:5678:9abc",
Subnet: "fd11:1234::/112",
},
NICType: cns.InfraNIC,
NICType: cns.InfraNIC,
EndpointPolicies: expectedEndpointPolicies,
},
},
Response: cns.Response{
Expand Down Expand Up @@ -749,6 +780,7 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
Gateway: net.ParseIP("fe80::1234:5678:9abc"),
},
},
EndpointPolicies: expectedEndpointPolicies,
Routes: []network.RouteInfo{
{
Dst: network.Ipv4DefaultRouteDstPrefix,
Expand All @@ -762,7 +794,8 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
NICType: cns.InfraNIC,
HostSubnetPrefix: *parseCIDR("fd11:1234::/112"),
},
wantErr: false,
wantDefaultDenyEndpoints: true,
wantErr: false,
},
{
name: "fail to request IP addresses from cns",
Expand All @@ -773,12 +806,24 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
require: require,
requestIPs: requestIPsHandler{
ipconfigArgument: getTestIPConfigsRequest(),
result: nil,
err: errors.New("failed error from CNS"), //nolint "error for ut"
result: &cns.IPConfigsResponse{
PodIPInfo: []cns.PodIpInfo{
{
EndpointPolicies: expectedEndpointPolicies,
},
},
Response: cns.Response{
ReturnCode: 0,
Message: "",
},
},
err: errors.New("failed error from CNS"), //nolint "error for ut"
paulyufan2 marked this conversation as resolved.
Show resolved Hide resolved

},
},
},
wantErr: true,
wantDefaultDenyEndpoints: false,
wantErr: true,
},
}
for _, tt := range tests {
Expand All @@ -794,6 +839,7 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
}
ipamAddResult, err := invoker.Add(IPAMAddConfig{nwCfg: tt.args.nwCfg, args: tt.args.args, options: tt.args.options})
if tt.wantErr {
require.Equalf([]policy.Policy(nil), ipamAddResult.interfaceInfo[string(cns.InfraNIC)].EndpointPolicies, "There was an error requesting IP addresses from cns")
require.Error(err)
} else {
require.NoError(err)
Expand All @@ -809,6 +855,11 @@ func TestCNSIPAMInvoker_Add(t *testing.T) {
}
if ifInfo.NICType == cns.InfraNIC {
require.Equalf(tt.wantDefaultResult, ifInfo, "incorrect default response")
if tt.wantDefaultDenyEndpoints {
require.Equalf(expectedEndpointPolicies, ifInfo.EndpointPolicies, "Correct default deny ACL")
} else {
require.Equalf([]policy.Policy(nil), ifInfo.EndpointPolicies, "Correct default deny ACL")
}
}
}
})
Expand Down
1 change: 1 addition & 0 deletions cni/network/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


createEpInfoOpt := createEpInfoOpt{
nwCfg: nwCfg,
Expand Down
1 change: 1 addition & 0 deletions network/endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ type InterfaceInfo struct {
HostSubnetPrefix net.IPNet // Move this field from ipamAddResult
NCResponse *cns.GetNetworkContainerResponse
PnPID string
EndpointPolicies []policy.Policy
}

type IPConfig struct {
Expand Down
Loading