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 kubernetes-version/injectKubernetesVersion to Metadata #360

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rybnico
Copy link
Contributor

@rybnico rybnico commented Dec 10, 2024

Description of changes:

It would be very helpful to have access to the Kubernetes version specified in a machine's spec.version in the cloud-init/ignition metadata. With this information, it is possible to install Kubernetes version specific resources in cloud-init/ignition.

I used this PR as a template, but I'm not sure if the way to control the injection is particularly elegant.
I would suggest injecting the field without a flag in the default or always passing the value of MetadataSettings completely to cloudinit.NewMetadata so that you don't have to add another argument for each flag.
I would be very happy to receive feedback from the maintainers on this suggestion and would then also carry out the desired implementation.

Testing performed:

We have tested this with the following preKubeadmCommands:

      - MINOR_KUBERNETES_VERSION=$(echo {{ '{{ ds.meta_data.kubernetes_version }}' }} | cut -d. -f1-2 )
      - curl -fsSL https://pkgs.k8s.io/core:/stable:/${MINOR_KUBERNETES_VERSION}/deb/Release.key | gpg --dearmor -o /etc/apt/keyrings/kubernetes-apt-keyring.gpg
      - echo "deb [signed-by=/etc/apt/keyrings/kubernetes-apt-keyring.gpg] https://pkgs.k8s.io/core:/stable:/${MINOR_KUBERNETES_VERSION}/deb/ /" > /etc/apt/sources.list.d/kubernetes.list
      - apt-get update -y
      - TRIMMED_KUBERNETES_VERSION=$(echo {{ '{{ ds.meta_data.kubernetes_version }}' }} | sed 's/\./\\\\./g' | sed 's/^v//')
      - RESOLVED_KUBERNETES_VERSION=$(apt-cache madison kubelet | awk -v VERSION=${TRIMMED_KUBERNETES_VERSION} '$3~ VERSION { print $3 }' | head -n1)
      - apt-get install -y kubelet=${RESOLVED_KUBERNETES_VERSION} kubeadm=${RESOLVED_KUBERNETES_VERSION} kubectl=${RESOLVED_KUBERNETES_VERSION}
      - apt-mark hold kubelet kubeadm kubectl

Copy link

🚀 e2e tests run

We add labels to the PRs to control the e2e test runs by running specific tests and skipping some test contexts,
please follow this guide:

Label Behaviour
none Run Generic tests
e2e/none skip all e2e tests (documentation etc) - overrides all e2e/* labels Do not run any tests (overrides all e2e/ flags)
e2e/flatcar run Flatcar e2e tests Add Flatcar tests to the run

ℹ️ Ask a Member to add the requested labels if you don't have enough permissions.

@rybnico rybnico marked this pull request as draft December 10, 2024 16:02
@@ -32,12 +35,14 @@ type Metadata struct {
}

// NewMetadata returns a new Metadata object.
func NewMetadata(instanceID, hostname string, injectProviderID bool) *Metadata {
func NewMetadata(instanceID, hostname string, kubernetesVersion string, injectProviderID bool, injectKubernetesVersion bool) *Metadata {
Copy link
Collaborator

@wikkyk wikkyk Dec 11, 2024

Choose a reason for hiding this comment

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

Could we do without both kubernetesVersion and injectKubernetesVersion? You could set BaseCloudInitData.KubernetesVersionInjection depending on whether kubernetesVersion is unset (say, empty string, or nil pointer).

If not, I would prefer the two args be kept together, like so
func NewMetadata(instanceID, hostname string, injectProviderID bool, injectKubernetesVersion bool, kubernetesVersion string) *Metadata {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback!

I have removed the injectKubernetesVersion field. The kubernetes-version will now be injected into the metadata if spec.version is set on the machine.

I had to add a new argument to NewMetadata because I don't have access to the MachineScope there and don't know of any other way to get the Kubernetes version here.

What do you think of the new, much simpler implementation? I removed the draft flag because I think it is ready now. I have successfully tested the implementation on our infrastructure.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
62.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@rybnico rybnico marked this pull request as ready for review December 18, 2024 11:14
@rybnico rybnico changed the title Draft: Add kubernetes-version/injectKubernetesVersion to Metadata Add kubernetes-version/injectKubernetesVersion to Metadata Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants