fix: enforce Azure CNI config via jq instead of sed #4060

jackfrancis · 2020-11-19T23:03:15Z

Reason for Change:

This PR reduces the bash complexity of the jq usage inside the Azure CNI setup foo so that the Azure CNI configuration is always a multi-line file to accommodate all the pre-existing (known-working) sed foo.

A future workstream would be to replace the sed foo with jq foo.

Before:

azureuser@k8s-agentpool1-40173905-vmss000001:~$ echo $(sudo cat "/etc/cni/net.d/10-azure.conflist" | jq '.plugins[0].ipv6Mode="ipv6nat"') > test.json
azureuser@k8s-agentpool1-40173905-vmss000001:~$ cat test.json 
{ "cniVersion": "0.3.0", "name": "azure", "plugins": [ { "type": "azure-vnet", "mode": "transparent", "bridge": "azure0", "ipsToRouteViaHost": [ "169.254.20.10" ], "ipam": { "type": "azure-vnet-ipam" }, "ipv6Mode": "ipv6nat" }, { "type": "portmap", "capabilities": { "portMappings": true }, "snat": true } ] }

In other words, the resultant JSON is a string with no newlines (which the sed commands aren't able to interpret).

After:

azureuser@k8s-agentpool1-40173905-vmss000001:~$ sudo jq '.plugins[0].ipv6Mode="ipv6nat"' "/etc/cni/net.d/10-azure.conflist" > test.json
azureuser@k8s-agentpool1-40173905-vmss000001:~$ cat test.json 
{
  "cniVersion": "0.3.0",
  "name": "azure",
  "plugins": [
    {
      "type": "azure-vnet",
      "mode": "transparent",
      "bridge": "azure0",
      "ipsToRouteViaHost": [
        "169.254.20.10"
      ],
      "ipam": {
        "type": "azure-vnet-ipam"
      },
      "ipv6Mode": "ipv6nat"
    },
    {
      "type": "portmap",
      "capabilities": {
        "portMappings": true
      },
      "snat": true
    }
  ]
}

This PR also rationalizes the ipv6 dual stack implementation and configuration, which requires bridge mode in order to work.

Issue Fixed:

Credit Where Due:

Does this change contain code from or inspired by another project?

No
Yes

If "Yes," did you notify that project's maintainers and provide attribution?

No
Yes

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

codecov · 2020-11-19T23:17:13Z

Codecov Report

Merging #4060 (f18149f) into master (cff0a34) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4060      +/-   ##
==========================================
+ Coverage   73.23%   73.25%   +0.02%     
==========================================
  Files         135      135              
  Lines       20640    20542      -98     
==========================================
- Hits        15115    15048      -67     
+ Misses       4549     4521      -28     
+ Partials      976      973       -3

Impacted Files	Coverage Δ
pkg/api/types.go	`92.15% <ø> (-1.17%)`	⬇️
pkg/engine/templates_generated.go	`43.70% <ø> (ø)`
pkg/api/vlabs/validate.go	`81.85% <100.00%> (+0.05%)`	⬆️
cmd/addpool.go	`18.07% <0.00%> (-0.11%)`	⬇️
pkg/api/mocks.go	`100.00% <0.00%> (ø)`
pkg/api/converterfromapi.go	`95.66% <0.00%> (+<0.01%)`	⬆️
pkg/api/defaults.go	`93.42% <0.00%> (+0.01%)`	⬆️
pkg/api/convertertoapi.go	`94.00% <0.00%> (+0.01%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cff0a34...f18149f. Read the comment docs.

jackfrancis · 2020-11-20T00:16:34Z

parts/k8s/cloud-init/artifacts/cse_config.sh

-    if [[ "${IPV6_DUALSTACK_ENABLED}" == "true" ]]; then
-      echo $(cat "$CNI_CONFIG_DIR/10-azure.conflist" | jq '.plugins[0].ipv6Mode="ipv6nat"') > "$CNI_CONFIG_DIR/10-azure.conflist"
-    fi
+{{- if IsIPv6DualStackFeatureEnabled}}


saving cloud-init payload here

jackfrancis · 2020-11-20T18:14:04Z

parts/k8s/cloud-init/artifacts/cse_config.sh

    fi
    if [[ "${NETWORK_MODE}" == "bridge" ]]; then
-      sed -i 's#"mode":"transparent"#"mode":"bridge"#g' $CNI_CONFIG_DIR/10-azure.conflist
+      jq '.plugins[0].mode="bridge"' "$CNI_CONFIG_DIR/10-azure.conflist" > $tmpDir/tmp


Every time you replace a sed with a jq a @jchauncey gets his wings

jackfrancis · 2020-11-20T19:17:14Z

test/e2e/test_cluster_configs/everything.json

@@ -14,6 +14,8 @@
 		"properties": {
 			"orchestratorProfile": {
 				"kubernetesConfig": {
+					"networkPlugin": "azure",


These changes ensure we still have test coverage for Azure CNI + bridge mode now that Azure CNI 1.2.0 has merged with transparent mode enabled by default.

aramase

/lgtm

acs-bot · 2020-11-20T20:52:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, jackfrancis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jackfrancis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

acs-bot added size/XS approved labels Nov 19, 2020

acs-bot added size/S and removed size/XS labels Nov 19, 2020

jackfrancis commented Nov 20, 2020

View reviewed changes

acs-bot added size/M and removed size/S labels Nov 20, 2020

jackfrancis added 5 commits November 20, 2020 10:08

fix: ensure Azure CNI config and sed can play together

5844e3c

use tmp file

ff26df1

optimize CSE payload

67a9036

update validation message

ae4a95b

use jq to change JSON contents

52c5cee

jackfrancis force-pushed the ipv6-jq-cse branch from 004898d to 52c5cee Compare November 20, 2020 18:12

jackfrancis commented Nov 20, 2020

View reviewed changes

add bridge=azure0

e50834d

jackfrancis changed the title ~~fix: ensure Azure CNI config and sed can play together~~ fix: enforce Azure CNI config via jq instead of sed Nov 20, 2020

ensure that we test bridge mode

4c11680

jackfrancis commented Nov 20, 2020

View reviewed changes

jackfrancis added 2 commits November 20, 2020 11:33

always configure bridge or transparent

4b0287b

enforce bridge mode for dual stack

f18149f

aramase reviewed Nov 20, 2020

View reviewed changes

acs-bot assigned aramase Nov 20, 2020

acs-bot added the lgtm label Nov 20, 2020

jackfrancis merged commit 7fdbef0 into Azure:master Nov 20, 2020

jackfrancis deleted the ipv6-jq-cse branch November 20, 2020 23:58

fmotrifork mentioned this pull request Dec 8, 2020

aks-engine 0.58.0 fishworks/fish-food#1098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enforce Azure CNI config via jq instead of sed #4060

fix: enforce Azure CNI config via jq instead of sed #4060

jackfrancis commented Nov 19, 2020 •

edited

codecov bot commented Nov 19, 2020 •

edited

jackfrancis Nov 20, 2020

jackfrancis Nov 20, 2020

jackfrancis Nov 20, 2020

aramase left a comment

acs-bot commented Nov 20, 2020

fix: enforce Azure CNI config via jq instead of sed #4060

fix: enforce Azure CNI config via jq instead of sed #4060

Conversation

jackfrancis commented Nov 19, 2020 • edited

codecov bot commented Nov 19, 2020 • edited

Codecov Report

jackfrancis Nov 20, 2020

Choose a reason for hiding this comment

jackfrancis Nov 20, 2020

Choose a reason for hiding this comment

jackfrancis Nov 20, 2020

Choose a reason for hiding this comment

aramase left a comment

Choose a reason for hiding this comment

acs-bot commented Nov 20, 2020

jackfrancis commented Nov 19, 2020 •

edited

codecov bot commented Nov 19, 2020 •

edited