Skip to content

Making consolidated 'overlay' option to cns ipam mode #1977

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

Closed
wants to merge 5 commits into from
Closed

Making consolidated 'overlay' option to cns ipam mode #1977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e63b4e
Making third 'overlay' option to cns ipam mode
pjohnst5 May 23, 2023
32aacb7
Consolidated cni conflists for both linux and windows
pjohnst5 Jun 7, 2023
16e537c
Fix makefile addition and add newline to linux consolidated conflist
pjohnst5 Jun 7, 2023
f2d1d8b
Merge branch 'master' into paujohns/cnsoverlayipammode
pjohnst5 Jun 8, 2023
f95bb06
One more place in network_windows.go where dualstackoverlay was used,…
pjohnst5 Jun 8, 2023
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
8 changes: 8 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ CNI_SWIFT_BUILD_DIR = $(BUILD_DIR)/cni-swift
CNI_OVERLAY_BUILD_DIR = $(BUILD_DIR)/cni-overlay
CNI_BAREMETAL_BUILD_DIR = $(BUILD_DIR)/cni-baremetal
CNI_DUALSTACK_BUILD_DIR = $(BUILD_DIR)/cni-dualstack
CNI_OVERLAY_CONSOLIDATED_BUILD_DIR = $(BUILD_DIR)/cni-overlay-consolidated
Copy link
Collaborator

@rbtr rbtr Jun 8, 2023

Choose a reason for hiding this comment

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

I am opposed to adding a new build artifact like this, as we will then need to migrate off of it in the future and back to the bare "overlay" artifact.
Can we not simply consolidate on to "cni-overlay" immediately?

Copy link
Contributor Author

@pjohnst5 pjohnst5 Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think that is a good point, one concern was backwards compatibility, to still have 'v4overlay' and 'dualstack' be valid options until we know 'overlay' works properly in AKS

So being able to roll back and still support 'dualstack' and 'v4overlay' was the major concern

Edit: Another reason was, to have some overlap in time, where CNS suports any overlay option, while we migrate off of 'v4overlay' and 'dualstackoverlay' in AKS

@ashvindeodhar wdyt?

Copy link
Contributor Author

@pjohnst5 pjohnst5 Jun 8, 2023

Choose a reason for hiding this comment

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

@thatmattlong what do you think as well?

Copy link
Contributor Author

@pjohnst5 pjohnst5 Jun 8, 2023

Choose a reason for hiding this comment

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

@rbtr what do you think, about having that overlap period in AKS (where all three options are viable)?

It maybe simpler to consolidate immediately, we would just need to keep these changes right in sync with AKS-RP changes and a new CNS version, might miss something

Copy link
Collaborator

@rbtr rbtr Jun 8, 2023

Choose a reason for hiding this comment

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

I was specifically referring to the tgz build artifacts, but looking further I think maybe too many things are changing in this single PR (or they should be reordered).
Should the changes actually be:

  • release CNI that supports the consolidated "executionMode":"swift" and ipam mode overlay
    • this should be end-to-end and should collapse v4/dualstack overlay in to the single cni-overlay build artifact
  • release dropgz with ^
  • release CNS that supports the consolidated overlay conflist generation
  • migrate AKS to consolidate overlay config argument everywhere
  • deprecate and remove v4overlay/dualstack overlay everywhere

vs the updates you have proposed, this way we should never need to update anything to an intermediate "overlay-consolidated" that we later need to change back.

Copy link
Contributor Author

@pjohnst5 pjohnst5 Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I didn't know about "executionMode": "swift" conslidation, is there someone working on that currently?

release dropgz with ^

After releasing dropgz, would we update AKS-RP anywhere?
One change we would need to do for sure, is make dualstack overlay point to overlay conflist

release CNS that supports the consolidated overlay conflist generation

Would dualstack and v4overlay conflist generation be gone at this point completely? Or there, and not used?

Copy link
Contributor Author

@pjohnst5 pjohnst5 Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We update azure-cni binary and conflist at once, by virtue of tarball
> Check this new binary and conflist don't break V4
> Maybe add pipeline step for overlay cni
> what aks checks are there?
> Still would release dualstack version for backcompat

Relesae dropgz with ^
Rollout this everywhere, ~4 weeks (CNI is everywhere, and supports 'overlay' mode)
> Can make this faster by cutting new Agent baker image for dropgz right after tarball is release in our repo
Add a third 'overlay' generator, until cns image is rolled out (4 weeks)
Change the configmap, to use 'overlay'
Remove 'dualstack' and 'v4overlay' in CNS and CNI

Unknowns:
executionMode v4swift, what is it? Do we need it in v4overlay?

CNS_BUILD_DIR = $(BUILD_DIR)/cns
NPM_BUILD_DIR = $(BUILD_DIR)/npm
TOOLS_DIR = $(REPO_ROOT)/build/tools
Expand Down Expand Up @@ -96,6 +97,7 @@ CNI_SWIFT_ARCHIVE_NAME = azure-vnet-cni-swift-$(GOOS)-$(GOARCH)-$(CNI_VERSION).$
CNI_OVERLAY_ARCHIVE_NAME = azure-vnet-cni-overlay-$(GOOS)-$(GOARCH)-$(CNI_VERSION).$(ARCHIVE_EXT)
CNI_BAREMETAL_ARCHIVE_NAME = azure-vnet-cni-baremetal-$(GOOS)-$(GOARCH)-$(CNI_VERSION).$(ARCHIVE_EXT)
CNI_DUALSTACK_ARCHIVE_NAME = azure-vnet-cni-overlay-dualstack-$(GOOS)-$(GOARCH)-$(CNI_VERSION).$(ARCHIVE_EXT)
CNI_OVERLAY_CONSOLIDATED_ARCHIVE_NAME = azure-vnet-cni-overlay-consolidated-$(GOOS)-$(GOARCH)-$(CNI_VERSION).$(ARCHIVE_EXT)
CNM_ARCHIVE_NAME = azure-vnet-cnm-$(GOOS)-$(GOARCH)-$(ACN_VERSION).$(ARCHIVE_EXT)
CNS_ARCHIVE_NAME = azure-cns-$(GOOS)-$(GOARCH)-$(CNS_VERSION).$(ARCHIVE_EXT)
NPM_ARCHIVE_NAME = azure-npm-$(GOOS)-$(GOARCH)-$(NPM_VERSION).$(ARCHIVE_EXT)
Expand Down Expand Up @@ -632,6 +634,12 @@ endif
cp $(CNI_BUILD_DIR)/azure-vnet$(EXE_EXT) $(CNI_BUILD_DIR)/azure-vnet-telemetry$(EXE_EXT) $(CNI_DUALSTACK_BUILD_DIR)
cd $(CNI_DUALSTACK_BUILD_DIR) && $(ARCHIVE_CMD) $(CNI_DUALSTACK_ARCHIVE_NAME) azure-vnet$(EXE_EXT) azure-vnet-telemetry$(EXE_EXT) 10-azure.conflist azure-vnet-telemetry.config

$(MKDIR) $(CNI_OVERLAY_CONSOLIDATED_BUILD_DIR)
cp cni/azure-$(GOOS)-swift-overlay-consolidated.conflist $(CNI_OVERLAY_CONSOLIDATED_BUILD_DIR)/10-azure.conflist
cp telemetry/azure-vnet-telemetry.config $(CNI_OVERLAY_CONSOLIDATED_BUILD_DIR)/azure-vnet-telemetry.config
cp $(CNI_BUILD_DIR)/azure-vnet$(EXE_EXT) $(CNI_BUILD_DIR)/azure-vnet-telemetry$(EXE_EXT) $(CNI_OVERLAY_CONSOLIDATED_BUILD_DIR)
cd $(CNI_OVERLAY_CONSOLIDATED_BUILD_DIR) && $(ARCHIVE_CMD) $(CNI_OVERLAY_CONSOLIDATED_ARCHIVE_NAME) azure-vnet$(EXE_EXT) azure-vnet-telemetry$(EXE_EXT) 10-azure.conflist azure-vnet-telemetry.config

#baremetal mode is windows only (at least for now)
ifeq ($(GOOS),windows)
$(MKDIR) $(CNI_BAREMETAL_BUILD_DIR)
Expand Down
23 changes: 23 additions & 0 deletions cni/azure-linux-swift-overlay-consolidated.conflist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"cniVersion":"0.3.0",
"name":"azure",
"plugins":[
{
"type":"azure-vnet",
"mode":"transparent",
"executionMode":"v4swift",
"ipsToRouteViaHost":["169.254.20.10"],
"ipam":{
"type":"azure-cns",
"mode":"overlay"
}
},
{
"type":"portmap",
"capabilities":{
"portMappings":true
},
"snat":true
}
]
}
50 changes: 50 additions & 0 deletions cni/azure-windows-swift-overlay-consolidated.conflist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
{
"cniVersion": "0.3.0",
"name": "azure",
"adapterName" : "",
"plugins": [
{
"type": "azure-vnet",
"mode": "bridge",
"bridge": "azure0",
"executionMode": "v4swift",
"capabilities": {
"portMappings": true,
"dns": true
},
"ipam": {
"type": "azure-cns",
"mode": "overlay"
},
"dns": {
"Nameservers": [
"10.0.0.10",
"168.63.129.16"
],
"Search": [
"svc.cluster.local"
]
},
"AdditionalArgs": [
{
"Name": "EndpointPolicy",
"Value": {
"Type": "OutBoundNAT",
"ExceptionList": [
"10.240.0.0/16",
"10.0.0.0/8"
]
}
},
{
"Name": "EndpointPolicy",
"Value": {
"Type": "ROUTE",
"DestinationPrefix": "10.0.0.0/8",
"NeedEncap": true
}
}
]
}
]
}
4 changes: 2 additions & 2 deletions cni/network/invoker_cns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func (invoker *CNSIPAMInvoker) Add(addConfig IPAMAddConfig) (IPAMAddResult, erro

ncgw := net.ParseIP(info.ncGatewayIPAddress)
if ncgw == nil {
if (invoker.ipamMode != util.V4Overlay) && (invoker.ipamMode != util.DualStackOverlay) {
if (invoker.ipamMode != util.V4Overlay) && (invoker.ipamMode != util.DualStackOverlay) && (invoker.ipamMode != util.Overlay) {
return IPAMAddResult{}, errors.Wrap(errInvalidArgs, "%w: Gateway address "+info.ncGatewayIPAddress+" from response is invalid")
}

Expand Down Expand Up @@ -199,7 +199,7 @@ func (invoker *CNSIPAMInvoker) Add(addConfig IPAMAddConfig) (IPAMAddResult, erro

// set subnet prefix for host vm
// setHostOptions will execute if IPAM mode is not v4 overlay and not dualStackOverlay mode
if (invoker.ipamMode != util.V4Overlay) && (invoker.ipamMode != util.DualStackOverlay) {
if (invoker.ipamMode != util.V4Overlay) && (invoker.ipamMode != util.DualStackOverlay) && (invoker.ipamMode != util.Overlay) {
if err := setHostOptions(ncIPNet, addConfig.options, &info); err != nil {
return IPAMAddResult{}, err
}
Expand Down
124 changes: 124 additions & 0 deletions cni/network/invoker_cns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,107 @@ func TestCNSIPAMInvoker_Add_Overlay(t *testing.T) {
fields: fields{
podName: testPodInfo.PodName,
podNamespace: testPodInfo.PodNamespace,
ipamMode: util.DualStackOverlay,
cnsClient: &MockCNSClient{
require: require,
requestIPs: requestIPsHandler{
ipconfigArgument: getTestIPConfigsRequest(),
result: &cns.IPConfigsResponse{
PodIPInfo: []cns.PodIpInfo{
{
PodIPConfig: cns.IPSubnet{
IPAddress: "10.0.1.10",
PrefixLength: 24,
},
NetworkContainerPrimaryIPConfig: cns.IPConfiguration{
IPSubnet: cns.IPSubnet{
IPAddress: "10.0.1.0",
PrefixLength: 24,
},
DNSServers: nil,
GatewayIPAddress: "10.0.0.1",
},
HostPrimaryIPInfo: cns.HostIPInfo{
Gateway: "10.0.0.1",
PrimaryIP: "10.0.0.1",
Subnet: "10.0.0.0/24",
},
},
{
PodIPConfig: cns.IPSubnet{
IPAddress: "fd11:1234::1",
PrefixLength: 24,
},
NetworkContainerPrimaryIPConfig: cns.IPConfiguration{
IPSubnet: cns.IPSubnet{
IPAddress: "fd11:1234::",
PrefixLength: 112,
},
DNSServers: nil,
GatewayIPAddress: "fe80::1234:5678:9abc",
},
HostPrimaryIPInfo: cns.HostIPInfo{
Gateway: "fe80::1234:5678:9abc",
PrimaryIP: "fe80::1234:5678:9abc",
Subnet: "fd11:1234::/112",
},
},
},
Response: cns.Response{
ReturnCode: 0,
Message: "",
},
},
err: nil,
},
},
},
args: args{
nwCfg: &cni.NetworkConfig{},
args: &cniSkel.CmdArgs{
ContainerID: "testcontainerid",
Netns: "testnetns",
IfName: "testifname",
},
hostSubnetPrefix: getCIDRNotationForAddress("10.0.0.1/24"),
options: map[string]interface{}{},
},
wantIpv4Result: &cniTypesCurr.Result{
IPs: []*cniTypesCurr.IPConfig{
{
Address: *getCIDRNotationForAddress("10.0.1.10/24"),
Gateway: net.ParseIP("10.0.0.1"),
},
},
Routes: []*cniTypes.Route{
{
Dst: network.Ipv4DefaultRouteDstPrefix,
GW: net.ParseIP("10.0.0.1"),
},
},
},
wantIpv6Result: &cniTypesCurr.Result{
IPs: []*cniTypesCurr.IPConfig{
{
Address: *getCIDRNotationForAddress("fd11:1234::1/112"),
Gateway: net.ParseIP("fe80::1234:5678:9abc"),
},
},
Routes: []*cniTypes.Route{
{
Dst: network.Ipv6DefaultRouteDstPrefix,
GW: net.ParseIP("fe80::1234:5678:9abc"),
},
},
},
wantErr: false,
},
{
name: "Test happy CNI Overlay add in consolidated overlay ipamMode",
fields: fields{
podName: testPodInfo.PodName,
podNamespace: testPodInfo.PodNamespace,
ipamMode: util.Overlay,
cnsClient: &MockCNSClient{
require: require,
requestIPs: requestIPsHandler{
Expand Down Expand Up @@ -877,6 +978,29 @@ func TestCNSIPAMInvoker_Delete_Overlay(t *testing.T) {
options: map[string]interface{}{},
},
},
{
name: "test delete happy path in consolidated overlay ipamMode",
fields: fields{
podName: testPodInfo.PodName,
podNamespace: testPodInfo.PodNamespace,
ipamMode: util.Overlay,
cnsClient: &MockCNSClient{
require: require,
releaseIPs: releaseIPsHandler{
ipconfigArgument: getTestIPConfigsRequest(),
},
},
},
args: args{
nwCfg: nil,
args: &cniSkel.CmdArgs{
ContainerID: "testcontainerid",
Netns: "testnetns",
IfName: "testifname",
},
options: map[string]interface{}{},
},
},
}
for _, tt := range tests {
tt := tt
Expand Down
2 changes: 1 addition & 1 deletion cni/network/network_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ func determineWinVer() {
}

func getNATInfo(nwCfg *cni.NetworkConfig, ncPrimaryIPIface interface{}, enableSnatForDNS bool) (natInfo []policy.NATInfo) {
if nwCfg.ExecutionMode == string(util.V4Swift) && nwCfg.IPAM.Mode != string(util.V4Overlay) && nwCfg.IPAM.Mode != string(util.DualStackOverlay) {
if nwCfg.ExecutionMode == string(util.V4Swift) && nwCfg.IPAM.Mode != string(util.V4Overlay) && nwCfg.IPAM.Mode != string(util.DualStackOverlay) && nwCfg.IPAM.Mode != string(util.Overlay) {
ncPrimaryIP := ""
if ncPrimaryIPIface != nil {
ncPrimaryIP = ncPrimaryIPIface.(string)
Expand Down
1 change: 1 addition & 0 deletions cni/util/const.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ type IpamMode string
const (
V4Overlay IpamMode = "v4overlay"
DualStackOverlay IpamMode = "dualStackOverlay"
Overlay IpamMode = "overlay" // Nothing changes between 'v4overlay' and 'dualStackOverlay' mode, so consolidating to one
)
13 changes: 13 additions & 0 deletions cns/cniconflist/generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ type DualStackOverlayGenerator struct {
Writer io.WriteCloser
}

// OverlayGenerator generates the Azure CNI conflist for all Overlay scenarios
type OverlayGenerator struct {
Writer io.WriteCloser
}

// CiliumGenerator generates the Azure CNI conflist for the Cilium scenario
type CiliumGenerator struct {
Writer io.WriteCloser
Expand All @@ -74,6 +79,14 @@ func (v *DualStackOverlayGenerator) Close() error {
return nil
}

func (v *OverlayGenerator) Close() error {
if err := v.Writer.Close(); err != nil {
return errors.Wrap(err, "error closing generator")
}

return nil
}

func (v *CiliumGenerator) Close() error {
if err := v.Writer.Close(); err != nil {
return errors.Wrap(err, "error closing generator")
Expand Down
29 changes: 29 additions & 0 deletions cns/cniconflist/generator_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,35 @@ func (v *DualStackOverlayGenerator) Generate() error {
return nil
}

// Generate writes the CNI conflist to the Generator's output stream
func (v *OverlayGenerator) Generate() error {
conflist := cniConflist{
CNIVersion: overlaycniVersion,
Name: overlaycniName,
Plugins: []any{
cni.NetworkConfig{
Type: overlaycniType,
Mode: cninet.OpModeTransparent,
ExecutionMode: string(util.V4Swift),
IPsToRouteViaHost: []string{nodeLocalDNSIP},
IPAM: cni.IPAM{
Type: network.AzureCNS,
Mode: string(util.Overlay),
},
},
portmapConfig,
},
}

enc := json.NewEncoder(v.Writer)
enc.SetIndent("", "\t")
if err := enc.Encode(conflist); err != nil {
return errors.Wrap(err, "error encoding conflist to json")
}

return nil
}

// Generate writes the CNI conflist to the Generator's output stream
func (v *CiliumGenerator) Generate() error {
conflist := cniConflist{
Expand Down
15 changes: 15 additions & 0 deletions cns/cniconflist/generator_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,21 @@ func TestGenerateDualStackOverlayConflist(t *testing.T) {
assert.Equal(t, removeNewLines(fixtureBytes), removeNewLines(buffer.Bytes()))
}

func TestGenerateOverlayConflist(t *testing.T) {
fixture := "testdata/fixtures/azure-linux-swift-overlay-consolidated.conflist"

buffer := new(bytes.Buffer)
g := cniconflist.OverlayGenerator{Writer: &bufferWriteCloser{buffer}}
err := g.Generate()
assert.NoError(t, err)

fixtureBytes, err := os.ReadFile(fixture)
assert.NoError(t, err)

// remove newlines and carriage returns in case these UTs are running on Windows
assert.Equal(t, removeNewLines(fixtureBytes), removeNewLines(buffer.Bytes()))
}

func TestGenerateCiliumConflist(t *testing.T) {
fixture := "testdata/fixtures/cilium.conflist"

Expand Down
4 changes: 4 additions & 0 deletions cns/cniconflist/generator_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ func (v *DualStackOverlayGenerator) Generate() error {
return errNotImplemented
}

func (v *OverlayGenerator) Generate() error {
return errNotImplemented
}

func (v *CiliumGenerator) Generate() error {
return errNotImplemented
}
Loading