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

✨ Add ExtraEnvs and ImagePullSerial to KubeadmConfig #10846

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 28 additions & 1 deletion bootstrap/kubeadm/api/v1beta1/kubeadm_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,12 @@ type ControlPlaneComponent struct {
// ExtraVolumes is an extra set of host volumes, mounted to the control plane component.
// +optional
ExtraVolumes []HostPathMount `json:"extraVolumes,omitempty"`

// ExtraEnvs is an extra set of environment variables to pass to the control plane component.
// Environment variables passed using ExtraEnvs will override any existing environment variables, or *_proxy environment variables that kubeadm adds by default.
// This option takes effect only on Kubernetes >=1.31.0.
// +optional
ExtraEnvs []EnvVar `json:"extraEnvs,omitempty"`
}

// APIServer holds settings necessary for API server deployments in the cluster.
Expand Down Expand Up @@ -192,7 +198,7 @@ type ImageMeta struct {
// +optional
ImageTag string `json:"imageTag,omitempty"`

//TODO: evaluate if we need also a ImageName based on user feedbacks
// TODO: evaluate if we need also a ImageName based on user feedbacks
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
Expand Down Expand Up @@ -260,6 +266,12 @@ type NodeRegistrationOptions struct {
// +kubebuilder:validation:Enum=Always;IfNotPresent;Never
// +optional
ImagePullPolicy string `json:"imagePullPolicy,omitempty"`

// ImagePullSerial specifies if image pulling performed by kubeadm must be done serially or in parallel.
// This option takes effect only on Kubernetes >=1.31.0.
// Default: true (defaulted in kubeadm)
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why do we want the default to be true for this one?

Copy link
Member

Choose a reason for hiding this comment

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

kubeadm defaults to true, I assume because it was the previous behavior

(cc @neolit123)

Copy link
Member

Choose a reason for hiding this comment

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

yes, serial is the default in kubeadm and kubelet

// +optional
ImagePullSerial *bool `json:"imagePullSerial,omitempty"`
}

// MarshalJSON marshals NodeRegistrationOptions in a way that an empty slice in Taints is preserved.
Expand All @@ -281,13 +293,15 @@ func (n *NodeRegistrationOptions) MarshalJSON() ([]byte, error) {
KubeletExtraArgs map[string]string `json:"kubeletExtraArgs,omitempty"`
IgnorePreflightErrors []string `json:"ignorePreflightErrors,omitempty"`
ImagePullPolicy string `json:"imagePullPolicy,omitempty"`
ImagePullSerial *bool `json:"imagePullSerial,omitempty"`
}{
Name: n.Name,
CRISocket: n.CRISocket,
Taints: n.Taints,
KubeletExtraArgs: n.KubeletExtraArgs,
IgnorePreflightErrors: n.IgnorePreflightErrors,
ImagePullPolicy: n.ImagePullPolicy,
ImagePullSerial: n.ImagePullSerial,
})
}

Expand All @@ -299,13 +313,15 @@ func (n *NodeRegistrationOptions) MarshalJSON() ([]byte, error) {
KubeletExtraArgs map[string]string `json:"kubeletExtraArgs,omitempty"`
IgnorePreflightErrors []string `json:"ignorePreflightErrors,omitempty"`
ImagePullPolicy string `json:"imagePullPolicy,omitempty"`
ImagePullSerial *bool `json:"imagePullSerial,omitempty"`
}{
Name: n.Name,
CRISocket: n.CRISocket,
Taints: n.Taints,
KubeletExtraArgs: n.KubeletExtraArgs,
IgnorePreflightErrors: n.IgnorePreflightErrors,
ImagePullPolicy: n.ImagePullPolicy,
ImagePullSerial: n.ImagePullSerial,
})
}

Expand Down Expand Up @@ -382,6 +398,12 @@ type LocalEtcd struct {
// +optional
ExtraArgs map[string]string `json:"extraArgs,omitempty"`

// ExtraEnvs is an extra set of environment variables to pass to the control plane component.
// Environment variables passed using ExtraEnvs will override any existing environment variables, or *_proxy environment variables that kubeadm adds by default.
// This option takes effect only on Kubernetes >=1.31.0.
// +optional
ExtraEnvs []EnvVar `json:"extraEnvs,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ExtraEnvs []EnvVar `json:"extraEnvs,omitempty"`
ExtraEnvs []corev1.EnvVar `json:"extraEnvs,omitempty"`

Do we need the extra container?

Copy link
Member

@sbueringer sbueringer Jul 9, 2024

Choose a reason for hiding this comment

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

Good question. It aligns to what kubeadm did. Not sure why

(cc @neolit123)

Copy link
Member

Choose a reason for hiding this comment

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

i actually don't remember why. will try to dig out comments.
seems there is no need for that.

should we change it pre-release 1.31?

Copy link
Member

@neolit123 neolit123 Jul 9, 2024

Choose a reason for hiding this comment

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

oh, that was a wild problem related to defaulting of corev1.ExtraEnvs using internal APIs
kubernetes/kubernetes#120561
https://github.com/kubernetes/kubernetes/pull/120561/files#diff-769d744db9aca1b60fb0f12e0c34fea8523740dccd24db5f9315b284ce1b1fefR211
kubernetes/kubeadm#2927

TL;DR a new custom wrapper struct was needed so that we can have a customer defaulter and not end up with generated defaulters that import internal k/k/pkg packages and causing weird behavior (that API machinery folks can't explain)

in CAPI you can maybe work around it, but for kubeadm we have to keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having the same struct that kubeadm uses really helps to do comparisons cross versions (and we have many of them!)

So if there are no objections, I will keep this aligned


// ServerCertSANs sets extra Subject Alternative Names for the etcd server signing cert.
// +optional
ServerCertSANs []string `json:"serverCertSANs,omitempty"`
Expand Down Expand Up @@ -735,3 +757,8 @@ type Patches struct {
// +optional
Directory string `json:"directory,omitempty"`
}

// EnvVar represents an environment variable present in a Container.
type EnvVar struct {
corev1.EnvVar `json:",inline"`
}
35 changes: 35 additions & 0 deletions bootstrap/kubeadm/api/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading