-
Notifications
You must be signed in to change notification settings - Fork 150
Support for custom CNI file name #4382
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -957,6 +957,14 @@ type CNISpec struct { | |||||||
| // +optional | ||||||||
| // +kubebuilder:validation:Type=string | ||||||||
| ConfDir *string `json:"confDir,omitempty"` | ||||||||
|
|
||||||||
| // ConfName is the name of the CNI config file. | ||||||||
| // If you have changed the name of the CNI configuration file in the container runtime configuration, | ||||||||
| // please ensure that this field matches the same name as specified in the container runtime settings. | ||||||||
| // Default: "10-calico.conflist" | ||||||||
| // +optional | ||||||||
| // +kubebuilder:validation:Type=string | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add a kubebuilder default here as well
Suggested change
|
||||||||
| ConfName *string `json:"confName,omitempty"` | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing we are going to have to solve is what to do if this value is changed to something with less priority. e.g., if you switch from We have some very rudimentary support for removing old config files here: https://github.com/projectcalico/calico/blob/master/cni-plugin/pkg/install/install.go#L413-L420 But it requires knowing the name of the file you want to remove! |
||||||||
| } | ||||||||
|
|
||||||||
| // InstallationStatus defines the observed state of the Calico or Calico Enterprise installation. | ||||||||
|
|
||||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -636,6 +636,12 @@ func fillDefaults(instance *operatorv1.Installation, currentPools *crdv1.IPPoolL | |||||||
| instance.Spec.CNI.BinDir = &defaultCNIBinDir | ||||||||
| } | ||||||||
|
|
||||||||
| // For compatability reasons, default the CNI config name if not specified. | ||||||||
| if instance.Spec.CNI.ConfName == nil { | ||||||||
| defaultCNIConfName := "10-calico.conflist" | ||||||||
| instance.Spec.CNI.ConfName = &defaultCNIConfName | ||||||||
|
Comment on lines
+641
to
+642
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| } | ||||||||
|
|
||||||||
| // While a number of the fields in this section are relevant to all CNI plugins, | ||||||||
| // there are some settings which are currently only applicable if using Calico CNI. | ||||||||
| // Handle those here. | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,7 +184,7 @@ func handleCore(c *components, install *operatorv1.Installation) error { | |
| } | ||
| } | ||
|
|
||
| if err := c.node.assertEnv(ctx, c.client, containerInstallCNI, "CNI_CONF_NAME", "10-calico.conflist"); err != nil { | ||
| if err := c.node.assertEnv(ctx, c.client, containerInstallCNI, "CNI_CONF_NAME", *install.Spec.CNI.ConfName); err != nil { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of asserting the env var matches, we should get the CNI_CONF_NAME env var and update the This means we can now upgrade manifest -> operator even if a custom CNI config name has been used. |
||
| return err | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1299,7 +1299,7 @@ func (c *nodeComponent) cniEnvvars() []corev1.EnvVar { | |
| } | ||
|
|
||
| envVars := []corev1.EnvVar{ | ||
| {Name: "CNI_CONF_NAME", Value: "10-calico.conflist"}, | ||
| {Name: "CNI_CONF_NAME", Value: *c.cfg.Installation.CNI.ConfName}, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would like to see a unit test for this field |
||
| {Name: "SLEEP", Value: "false"}, | ||
| {Name: "CNI_NET_DIR", Value: *c.cfg.Installation.CNI.ConfDir}, | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this field needs to match anything in the container runtime settings (looks like copy/paste from the ConfDir param)