chore: Update calico to 3.5 and allow calico to work with azure CNI #454

song-jiang · 2019-02-07T12:21:49Z

Reason for Change:

Upgrade calico to v3.5
Allow calico to work with azure CNI by setting
NetworkPolicy: "calico" , NetworkPlugin: "azure"
In this case, calico per-host daemon works with azure CNI plugin and azure IPAM to support network policy.
Continue support kubenet network plugin by setting
NetworkPolicy: "calico" , NetworkPlugin: "kubenet"

Issue Fixed:

Requirements:

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

Notes:

codecov · 2019-02-07T13:20:11Z

Codecov Report

Merging #454 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master    #454   +/-   ##
======================================
  Coverage    73.3%   73.3%           
======================================
  Files         130     130           
  Lines       17708   17708           
======================================
  Hits        12981   12981           
  Misses       4026    4026           
  Partials      701     701

CecileRobertMichon · 2019-02-07T21:27:40Z

@jackfrancis should we hold this until we cut the release?

/hold

jackfrancis · 2019-02-07T21:28:29Z

Yes. We want to run this through the appropriate back-channel cluster configuration tests appropriate for this surface area.

jackfrancis · 2019-02-07T21:28:54Z

I mean to say that this doesn't need to go in ASAP with the next aks-engine release, so we're on the same page. :)

jackfrancis · 2019-02-13T23:44:34Z

@song-jiang Can you add more description about the differences between NetworkPlugin: "calico+azureipam" and NetworkPlugin: "azure" in a calico context?

jackfrancis · 2019-02-13T23:45:40Z

parts/k8s/kubernetesconfigs.sh

+        mv $CNI_BIN_DIR/10-azure.conflist $CNI_CONFIG_DIR/
+        chmod 600 $CNI_CONFIG_DIR/10-azure.conflist
+        if [[ "${NETWORK_POLICY}" == "calico" ]] || [[ "${NETWORK_POLICY}" == "" ]]; then
+          sed -i 's#"mode":"bridge"#"mode":"transparent"#g' $CNI_CONFIG_DIR/10-azure.conflist


Do we really want to change the Azure CNI configuration here? This bridge mode setting will now apply to non-calico scenarios given the || [[ "${NETWORK_POLICY}" == "" ]] condition

The purpose for this change is to make life easier for users who want to apply calico for network policy on clusters previously installed without specifying a network policy provider.
My understanding is it should not affect any user experience if they choose a non-calico network policy provider.

I am not sure if user really care about using a bridge mode network or a transparent mode network. Would love to hear your input on this. Thanks.

@sharmasushant @tamilmani1989 can you comment on this change?

By default we should have bridge mode and not transparent mode. To my understanding, the config should be set to transparent mode only if user speicifes calico policy. @sharmasushant can comment on this.

@tamilmani1989 I'm running this through some final smoke tests. Please comment if we still have open concerns about this change before merging.

jackfrancis · 2019-02-15T23:02:01Z

@sharmasushant @tamilmani1989 Could you kindly read through this PR description and comment on @song-jiang's description of the difference between NetworkPolicy: "calico" , NetworkPlugin: "azure" and NetworkPolicy: "calico" , NetworkPlugin: "calico+azureipam".

Based on the reading through the changeset, the "calico+azureipam" flavor is the one that represents the current implementation of calico + Azure CNI. NetworkPolicy: "calico" , NetworkPlugin: "azure" appears to be a novel configuration/implementation.

Any thoughts?

sharmasushant · 2019-02-16T00:09:18Z

@jackfrancis
We should not change the existing defaults.
The newly added "transparent" mode in azure cni is only supported with calico policy, and should not be default.

The supported default mode for azure cni is "bridge", and default policy should be "azure" as well.

song-jiang · 2019-02-19T10:09:08Z

@jackfrancis @sharmasushant @tamilmani1989 Thanks for your input and clarifications. Apologies for my misunderstanding. I will make changes to my PR accordingly.

song-jiang · 2019-02-19T17:45:00Z

@jackfrancis Could you take another look?

tamilmani1989

@song-jiang as per email conversation with tigera, we agreed upon azure cni +azure ipam + calico network policy. NetworkPlugin: "calico+azureipam" - this is something new (calico cni +azure ipam) that we didn't talk about over the email thread.

tamilmani1989 · 2019-02-19T19:09:47Z

parts/k8s/kubernetesinstalls.sh

@@ -169,7 +169,7 @@ installClearContainersRuntime() {
 }

 installNetworkPlugin() {
-    if [[ "${NETWORK_PLUGIN}" = "azure" ]]; then
+    if [[ "${NETWORK_PLUGIN}" = "azure" ]] || [[ "${NETWORK_PLUGIN}" = "calico+azureipam"  ]]; then


what is calico+azureipam? I may be missing something here. Can you please explain us which scenario this is used?

feiskyer · 2019-02-20T08:40:45Z

@sharmasushant @tamilmani1989 Could you kindly read through this PR description and comment on @song-jiang's description of the difference between NetworkPolicy: "calico" , NetworkPlugin: "azure" and NetworkPolicy: "calico" , NetworkPlugin: "calico+azureipam".

Same questions here.

song-jiang · 2019-02-20T18:18:45Z

@tamilmani1989 "NetworkPlugin: calico+azureipam, NetworkPolicy: calico" is the replacement of current implementation of "NetworkPlugin: azure, NetworkPolicy: calico" which uses calico CNI plugin and azure IPAM.

We leave that as an option for anyone who might be using it and continue wish to do so. I will check with PM on email conversations made earlier.

jackfrancis · 2019-02-20T18:22:42Z

@song-jiang I have a preference for simply moving the implementation forward. If we have good reason to change the implementation, we should stand by that opinion, rather than offering two implementations that basically do the same thing, which I would argue is confusing for users.

song-jiang · 2019-02-20T18:34:37Z

@jackfrancis Agree it is less confusing to have just one option. Let me check with PM and come back to you.

ahrkrak · 2019-03-04T02:32:52Z

@song-jiang I think we have agreed on this with PM - can you confirm and give @jackfrancis an idea of when you're likely to get this update done?

song-jiang · 2019-03-04T14:11:15Z

@ahrkrak Yes. It has been confirmed with PM we will drop option calico+azureipam. Another changes I need to put in is to disable azure-cni-networkmonitor daemonset if azure CNI set to transparent mode. I expect to get PR updated by Wednesday. cc @jackfrancis

CecileRobertMichon · 2019-03-14T00:40:32Z

@song-jiang please rebase on latest master to get up to date unit test coverage stats (the last rebase seems to be from 8 days ago).

CecileRobertMichon · 2019-03-14T00:46:16Z

pkg/api/vlabs/validate_test.go

@@ -704,6 +704,10 @@ func Test_Properties_ValidateNetworkPluginPlusPolicy(t *testing.T) {
 			networkPlugin: "azure",
 			networkPolicy: "flannel",
 		},
+		{
+			networkPlugin: "azure",


this combination is already in networkPluginPlusPolicyAllowed in validate.go while this is adding it to the unit test expecting it to not be valid. Unless I'm missing something, it seems contradictory.

I see. My mistake. Thanks!

song-jiang · 2019-03-14T16:33:49Z

@tamilmani1989 It is unlikely pods on same host have mixed veth prefixes. During upgrade process, either calico CNI (old node) or azure CNI (new node) will be used. However, what I found is we could have a scenario where new node gets old version of calico-node ( prefix cali) and azure CNI (prefix azv). This would break network policy.

So current status on upgrading from existing (AKS or bare-metal) cluster with my PR

network connection functions as normal.
network policy is broken.

I am working on possible solutions to fix this. cc @jackfrancis @sauryadas

dtzar · 2019-03-19T23:36:46Z

Don't want to delay this PR! Would be nice to see Calico at 3.5.3 or 3.6.0 to have the latest fixes/functionality, ensure all works, and avoid another PR to update the daemonset yamls.

feiskyer · 2019-03-20T04:53:03Z

network policy is broken.

Is this fixed now?

song-jiang · 2019-03-21T14:18:31Z

@feiskyer Not yet. It is still in progress.

ahrkrak · 2019-03-28T19:19:55Z

We have agreed an upgrade strategy that will work, but needs to be documented. @jackfrancis is picking up ownership of the docs changes.
@jackfrancis will you add those docs changes into this PR or a separate one?

jackfrancis · 2019-03-28T19:33:15Z

@ahrkrak filed this as a follow-up:

#908

jackfrancis · 2019-03-28T19:35:10Z

/azp run pr-e2e

azure-pipelines · 2019-03-28T19:35:25Z

Azure Pipelines successfully started running 1 pipeline(s).

jackfrancis · 2019-03-28T21:15:43Z

smoke tests checked out

lgtm

@tamilmani1989 for sign off on changes as well

tamilmani1989 · 2019-03-28T21:18:13Z

@song-jiang @ahrkrak
If they follow this doc #908 , they won't end up in this issue during upgrade?

network policy is broken.

can you please confirm?

ahrkrak · 2019-03-29T00:34:27Z

Correct. Currently #908 just includes a reference to the yaml that you have to kubectl delete. That removes any older calico-nodes, which will then get updated by the addon manager to the newer version. @jackfrancis will update the actual docs to include the howto instructions for doing that.

jackfrancis · 2019-04-01T22:14:50Z

/lgtm

welcome · 2019-04-01T22:14:59Z

Congrats on merging your first pull request! 🎉🎉🎉

acs-bot · 2019-04-01T22:15:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, song-jiang

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

…454)

acs-bot added the size/XXL label Feb 7, 2019

song-jiang force-pushed the song-calico-3.5 branch from 400dbdc to 7fed98e Compare February 7, 2019 14:29

acs-bot added the do-not-merge/hold label Feb 7, 2019

jackfrancis reviewed Feb 13, 2019

View reviewed changes

tamilmani1989 suggested changes Feb 19, 2019

View reviewed changes

song-jiang force-pushed the song-calico-3.5 branch from addab61 to 0ad9de9 Compare March 4, 2019 11:52

acs-bot added size/S and removed size/XXL labels Mar 4, 2019

song-jiang changed the title ~~chore: Update calico to 3.5 and allow calico to work with azure CNI~~ [WIP] chore: Update calico to 3.5 and allow calico to work with azure CNI Mar 4, 2019

acs-bot added the do-not-merge/work-in-progress label Mar 4, 2019

acs-bot added size/XXL and removed size/S labels Mar 5, 2019

song-jiang force-pushed the song-calico-3.5 branch from 85fb75a to 8c910d9 Compare March 5, 2019 15:33

song-jiang changed the title ~~[WIP] chore: Update calico to 3.5 and allow calico to work with azure CNI~~ chore: Update calico to 3.5 and allow calico to work with azure CNI Mar 5, 2019

CecileRobertMichon reviewed Mar 14, 2019

View reviewed changes

song-jiang force-pushed the song-calico-3.5 branch from f53cee5 to ba8660c Compare March 15, 2019 17:13

song-jiang force-pushed the song-calico-3.5 branch from ba8660c to f0ada3c Compare March 22, 2019 14:16

chore: Update calico to 3.5 and allow calico to work with azure CNI

c1f8add

jackfrancis force-pushed the song-calico-3.5 branch from f0ada3c to c1f8add Compare March 28, 2019 19:34

acs-bot assigned jackfrancis Apr 1, 2019

acs-bot added the lgtm label Apr 1, 2019

jackfrancis merged commit 4b9a5bd into Azure:master Apr 1, 2019

acs-bot added the approved label Apr 1, 2019

mboersma pushed a commit that referenced this pull request Apr 4, 2019

chore: Update calico to 3.5 and allow calico to work with azure CNI (#…

c4da4af

…454)

jackfrancis mentioned this pull request Apr 5, 2019

chore: only use new calico CSE config for AKS Engine #952

Merged

4 tasks

This was referenced Apr 30, 2019

AKS k8s version 1.13.5 Calico pods in CrashLoopBackOff state Azure/AKS#933

Closed

Cluster creation fails with NodesNotReady status if network policy is calico Azure/AKS#905

Closed

chore: Update calico to 3.5 and allow calico to work with azure CNI #454

chore: Update calico to 3.5 and allow calico to work with azure CNI #454

Conversation

song-jiang commented Feb 7, 2019 • edited Loading

codecov bot commented Feb 7, 2019 • edited Loading

Codecov Report

CecileRobertMichon commented Feb 7, 2019

jackfrancis commented Feb 7, 2019

jackfrancis commented Feb 7, 2019

jackfrancis commented Feb 13, 2019

jackfrancis Feb 13, 2019

Choose a reason for hiding this comment

song-jiang Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

jackfrancis Feb 15, 2019

Choose a reason for hiding this comment

tamilmani1989 Feb 15, 2019

Choose a reason for hiding this comment

jackfrancis Mar 28, 2019

Choose a reason for hiding this comment

jackfrancis commented Feb 15, 2019

sharmasushant commented Feb 16, 2019 • edited Loading

song-jiang commented Feb 19, 2019

song-jiang commented Feb 19, 2019

tamilmani1989 left a comment

Choose a reason for hiding this comment

tamilmani1989 Feb 19, 2019

Choose a reason for hiding this comment

feiskyer commented Feb 20, 2019

song-jiang commented Feb 20, 2019

jackfrancis commented Feb 20, 2019

song-jiang commented Feb 20, 2019

ahrkrak commented Mar 4, 2019

song-jiang commented Mar 4, 2019

CecileRobertMichon commented Mar 14, 2019

CecileRobertMichon Mar 14, 2019

Choose a reason for hiding this comment

song-jiang Mar 14, 2019

Choose a reason for hiding this comment

song-jiang commented Mar 14, 2019

dtzar commented Mar 19, 2019

feiskyer commented Mar 20, 2019

song-jiang commented Mar 21, 2019

ahrkrak commented Mar 28, 2019

jackfrancis commented Mar 28, 2019

jackfrancis commented Mar 28, 2019

azure-pipelines bot commented Mar 28, 2019

jackfrancis commented Mar 28, 2019

tamilmani1989 commented Mar 28, 2019 • edited Loading

ahrkrak commented Mar 29, 2019

jackfrancis commented Apr 1, 2019

welcome bot commented Apr 1, 2019

acs-bot commented Apr 1, 2019

song-jiang commented Feb 7, 2019 •

edited

Loading

codecov bot commented Feb 7, 2019 •

edited

Loading

song-jiang Feb 14, 2019 •

edited

Loading

sharmasushant commented Feb 16, 2019 •

edited

Loading

tamilmani1989 commented Mar 28, 2019 •

edited

Loading