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

refactor: set kube network based on cni type instead of provider #3166

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

davidgiga1993
Copy link
Contributor

Description

Changes the behavior how the kubeNetwork environment variable is defined by looking at the actual used CNI type instead of the k8s platform.

Fixes #3165

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

@CLAassistant
Copy link

CLAassistant commented Feb 13, 2024

CLA assistant check
All committers have signed the CLA.

@coutinhop
Copy link
Member

/sem-approve

@davidgiga1993
Copy link
Contributor Author

Anything I should be doing? I can't see the CI logs in case it was caused by this mr

@caseydavenport
Copy link
Member

diff -u pkg/render/windows.go.orig pkg/render/windows.go00:31
--- pkg/render/windows.go.orig	2024-02-21 18:43:44.264553431 +000000:31
+++ pkg/render/windows.go	2024-02-21 18:43:44.264553431 +000000:31
@@ -591,7 +591,7 @@00:31
 	if c.cfg.Installation.CNI != nil && c.cfg.Installation.CNI.Type == operatorv1.PluginAzureVNET {00:31
 		kubeNetwork = "azure.*"00:31
 	}00:31
-	00:31
+00:31
 	windowsEnv = append(windowsEnv, corev1.EnvVar{Name: "KUBE_NETWORK", Value: kubeNetwork})00:31
 00:31
 	// Determine MTU to use. If specified explicitly, use that. Otherwise, set defaults based on an overall00:31
  goimports would make changes to file: pkg/render/windows.go00:31
Some files failed gofmt check.  Run 00:31
  make fix

It's failing the static checks (which can be run locally with make static-checks).

@coutinhop
Copy link
Member

/sem-approve

@caseydavenport
Copy link
Member

Looks like an actual test failure now:

Class name
pkg/render Suite
Failure
/go/src/github.com/tigera/operator/pkg/render/windows_test.go:1041
Expected
    <int>: 1
to equal
    <int>: 2
/go/src/github.com/tigera/operator/pkg/render/windows_test.go:1061

@davidgiga1993
Copy link
Contributor Author

I did some adjustments to the test as it seems like it wasn't checking the correct case according to the description. Let me know if that is the expected behavior now

@song-jiang
Copy link
Contributor

@davidgiga1993 I can see the code changes you are making for the test is correct. Restarted CI run to see if it'll pass.

@song-jiang
Copy link
Contributor

/sem-approve

@song-jiang
Copy link
Contributor

LGTM

@song-jiang song-jiang self-requested a review February 28, 2024 13:23
@song-jiang
Copy link
Contributor

@coutinhop @caseydavenport Could you take a look and if everything's ok please help to approve&merge. Thanks.

@coutinhop coutinhop merged commit 9f20325 into tigera:master Mar 6, 2024
5 checks passed
song-jiang pushed a commit to song-jiang/operator that referenced this pull request Mar 6, 2024
caseydavenport pushed a commit that referenced this pull request Mar 6, 2024
…) (#3227)

(cherry picked from commit 9f20325)

Co-authored-by: David <davidgiga1993@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't use calico on windows on EKS due to forced network mode
6 participants