From c74cca94ec6107d4acb5d621c334ec8ee31ad9c4 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 15:21:05 +0200 Subject: [PATCH 01/14] Add core component definitions in release manifest --- api/v1alpha1/releasemanfest_types_test.go | 35 +++++++++++++++++++++++ api/v1alpha1/releasemanifest_types.go | 32 ++++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/api/v1alpha1/releasemanfest_types_test.go b/api/v1alpha1/releasemanfest_types_test.go index 1efa367..2a8db06 100644 --- a/api/v1alpha1/releasemanfest_types_test.go +++ b/api/v1alpha1/releasemanfest_types_test.go @@ -27,3 +27,38 @@ func TestSupportedArchitectures(t *testing.T) { assert.Contains(t, supported, "amd64") assert.Contains(t, supported, "arm64") } + +func TestConvertContainersSliceToMap(t *testing.T) { + expContainer1 := "foo" + expContainerImage1 := "bar" + expContainer2 := "bar" + expContainerImage2 := "baz" + + component := CoreComponent{ + Containers: []CoreComponentContainer{ + { + Name: expContainer1, + Image: expContainerImage1, + }, + { + Name: expContainer2, + Image: expContainerImage2, + }, + }, + } + + m := component.ConvertContainerSliceToMap() + assert.Equal(t, 2, len(m)) + + v, ok := m[expContainer1] + assert.True(t, ok) + assert.Equal(t, expContainerImage1, v) + + v, ok = m[expContainer2] + assert.True(t, ok) + assert.Equal(t, expContainerImage2, v) + + component = CoreComponent{} + m = component.ConvertContainerSliceToMap() + assert.Empty(t, m) +} diff --git a/api/v1alpha1/releasemanifest_types.go b/api/v1alpha1/releasemanifest_types.go index 8f2bc9b..2235a37 100644 --- a/api/v1alpha1/releasemanifest_types.go +++ b/api/v1alpha1/releasemanifest_types.go @@ -96,7 +96,37 @@ type Kubernetes struct { } type KubernetesDistribution struct { - Version string `json:"version"` + Version string `json:"version"` + CoreComponents []CoreComponent `json:"coreComponents,omitempty"` +} + +// +kubebuilder:validation:Enum=HelmChart;Deployment +type CoreComponentType string + +const ( + HelmChartType CoreComponentType = "HelmChart" + DeploymentType CoreComponentType = "Deployment" +) + +type CoreComponent struct { + Name string `json:"name"` + Version string `json:"version,omitempty"` + Containers []CoreComponentContainer `json:"containers,omitempty"` + Type CoreComponentType `json:"type"` +} + +func (c *CoreComponent) ConvertContainerSliceToMap() map[string]string { + containerMap := map[string]string{} + for _, container := range c.Containers { + containerMap[container.Name] = container.Image + } + + return containerMap +} + +type CoreComponentContainer struct { + Name string `json:"name"` + Image string `json:"image"` } type OperatingSystem struct { From 2921576486bb745b03e1cdc1ece3edb8d4eaecbd Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 15:24:05 +0200 Subject: [PATCH 02/14] Align helm chart CRD indentations with kubebuilder generated CRD --- .../templates/release-manifest-crd.yaml | 230 +++++++++--------- 1 file changed, 115 insertions(+), 115 deletions(-) diff --git a/helm/upgrade-controller/charts/lifecycle-crds/templates/release-manifest-crd.yaml b/helm/upgrade-controller/charts/lifecycle-crds/templates/release-manifest-crd.yaml index 06d416f..118a380 100644 --- a/helm/upgrade-controller/charts/lifecycle-crds/templates/release-manifest-crd.yaml +++ b/helm/upgrade-controller/charts/lifecycle-crds/templates/release-manifest-crd.yaml @@ -12,124 +12,124 @@ spec: singular: releasemanifest scope: Namespaced versions: - - name: v1alpha1 - schema: - openAPIV3Schema: - description: ReleaseManifest is the Schema for the releasemanifests API - properties: - apiVersion: - description: |- - APIVersion defines the versioned schema of this representation of an object. - Servers should convert recognized schemas to the latest internal value, and - may reject unrecognized values. - More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources - type: string - kind: - description: |- - Kind is a string value representing the REST resource this object represents. - Servers may infer this from the endpoint the client submits requests to. - Cannot be updated. - In CamelCase. - More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds - type: string - metadata: - type: object - spec: - description: ReleaseManifestSpec defines the desired state of ReleaseManifest - properties: - components: - properties: - kubernetes: - properties: - k3s: + - name: v1alpha1 + schema: + openAPIV3Schema: + description: ReleaseManifest is the Schema for the releasemanifests API + properties: + apiVersion: + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources + type: string + kind: + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string + metadata: + type: object + spec: + description: ReleaseManifestSpec defines the desired state of ReleaseManifest + properties: + components: + properties: + kubernetes: + properties: + k3s: + properties: + version: + type: string + required: + - version + type: object + rke2: + properties: + version: + type: string + required: + - version + type: object + required: + - k3s + - rke2 + type: object + operatingSystem: + properties: + cpeScheme: + type: string + prettyName: + type: string + supportedArchs: + items: + enum: + - x86_64 + - aarch64 + type: string + minItems: 1 + type: array + version: + type: string + zypperID: + type: string + required: + - cpeScheme + - prettyName + - supportedArchs + - version + - zypperID + type: object + workloads: + properties: + helm: + items: properties: - version: + addonCharts: + x-kubernetes-preserve-unknown-fields: true + chart: type: string - required: - - version - type: object - rke2: - properties: + dependencyCharts: + x-kubernetes-preserve-unknown-fields: true + prettyName: + type: string + releaseName: + type: string + repository: + type: string + values: + x-kubernetes-preserve-unknown-fields: true version: type: string required: - - version + - chart + - prettyName + - releaseName + - version type: object - required: - - k3s - - rke2 - type: object - operatingSystem: - properties: - cpeScheme: - type: string - prettyName: - type: string - supportedArchs: - items: - enum: - - x86_64 - - aarch64 - type: string - minItems: 1 - type: array - version: - type: string - zypperID: - type: string - required: - - cpeScheme - - prettyName - - supportedArchs - - version - - zypperID - type: object - workloads: - properties: - helm: - items: - properties: - addonCharts: - x-kubernetes-preserve-unknown-fields: true - chart: - type: string - dependencyCharts: - x-kubernetes-preserve-unknown-fields: true - prettyName: - type: string - releaseName: - type: string - repository: - type: string - values: - x-kubernetes-preserve-unknown-fields: true - version: - type: string - required: - - chart - - prettyName - - releaseName - - version - type: object - type: array - required: - - helm - type: object - required: - - kubernetes - - operatingSystem - - workloads - type: object - releaseVersion: - type: string - required: - - releaseVersion - type: object - status: - description: ReleaseManifestStatus defines the observed state of ReleaseManifest - type: object - type: object - served: true - storage: true - subresources: - status: {} + type: array + required: + - helm + type: object + required: + - kubernetes + - operatingSystem + - workloads + type: object + releaseVersion: + type: string + required: + - releaseVersion + type: object + status: + description: ReleaseManifestStatus defines the observed state of ReleaseManifest + type: object + type: object + served: true + storage: true + subresources: + status: {} From 70dab5b60bb95703bb24bbaa3ca0110b8e5df836 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 15:25:44 +0200 Subject: [PATCH 03/14] Introduce new release manifest CRD changes --- .../lifecycle.suse.com_releasemanifests.yaml | 58 +++++++++++++++++++ .../templates/release-manifest-crd.yaml | 58 +++++++++++++++++++ 2 files changed, 116 insertions(+) diff --git a/config/crd/bases/lifecycle.suse.com_releasemanifests.yaml b/config/crd/bases/lifecycle.suse.com_releasemanifests.yaml index 2f80867..68f0e1e 100644 --- a/config/crd/bases/lifecycle.suse.com_releasemanifests.yaml +++ b/config/crd/bases/lifecycle.suse.com_releasemanifests.yaml @@ -45,6 +45,35 @@ spec: properties: k3s: properties: + coreComponents: + items: + properties: + containers: + items: + properties: + image: + type: string + name: + type: string + required: + - image + - name + type: object + type: array + name: + type: string + type: + enum: + - HelmChart + - Deployment + type: string + version: + type: string + required: + - name + - type + type: object + type: array version: type: string required: @@ -52,6 +81,35 @@ spec: type: object rke2: properties: + coreComponents: + items: + properties: + containers: + items: + properties: + image: + type: string + name: + type: string + required: + - image + - name + type: object + type: array + name: + type: string + type: + enum: + - HelmChart + - Deployment + type: string + version: + type: string + required: + - name + - type + type: object + type: array version: type: string required: diff --git a/helm/upgrade-controller/charts/lifecycle-crds/templates/release-manifest-crd.yaml b/helm/upgrade-controller/charts/lifecycle-crds/templates/release-manifest-crd.yaml index 118a380..61fa507 100644 --- a/helm/upgrade-controller/charts/lifecycle-crds/templates/release-manifest-crd.yaml +++ b/helm/upgrade-controller/charts/lifecycle-crds/templates/release-manifest-crd.yaml @@ -43,6 +43,35 @@ spec: properties: k3s: properties: + coreComponents: + items: + properties: + containers: + items: + properties: + image: + type: string + name: + type: string + required: + - image + - name + type: object + type: array + name: + type: string + type: + enum: + - HelmChart + - Deployment + type: string + version: + type: string + required: + - name + - type + type: object + type: array version: type: string required: @@ -50,6 +79,35 @@ spec: type: object rke2: properties: + coreComponents: + items: + properties: + containers: + items: + properties: + image: + type: string + name: + type: string + required: + - image + - name + type: object + type: array + name: + type: string + type: + enum: + - HelmChart + - Deployment + type: string + version: + type: string + required: + - name + - type + type: object + type: array version: type: string required: From 8e550cd30ff0ff4874a22e531899b6e079f5e261 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 15:26:00 +0200 Subject: [PATCH 04/14] make generate --- api/v1alpha1/zz_generated.deepcopy.go | 48 +++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 61f3746..104d6ca 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -29,7 +29,7 @@ import ( // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Components) DeepCopyInto(out *Components) { *out = *in - out.Kubernetes = in.Kubernetes + in.Kubernetes.DeepCopyInto(&out.Kubernetes) in.OperatingSystem.DeepCopyInto(&out.OperatingSystem) in.Workloads.DeepCopyInto(&out.Workloads) } @@ -44,6 +44,41 @@ func (in *Components) DeepCopy() *Components { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *CoreComponent) DeepCopyInto(out *CoreComponent) { + *out = *in + if in.Containers != nil { + in, out := &in.Containers, &out.Containers + *out = make([]CoreComponentContainer, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CoreComponent. +func (in *CoreComponent) DeepCopy() *CoreComponent { + if in == nil { + return nil + } + out := new(CoreComponent) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *CoreComponentContainer) DeepCopyInto(out *CoreComponentContainer) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CoreComponentContainer. +func (in *CoreComponentContainer) DeepCopy() *CoreComponentContainer { + if in == nil { + return nil + } + out := new(CoreComponentContainer) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DisableDrain) DeepCopyInto(out *DisableDrain) { *out = *in @@ -116,8 +151,8 @@ func (in *HelmValues) DeepCopy() *HelmValues { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Kubernetes) DeepCopyInto(out *Kubernetes) { *out = *in - out.K3S = in.K3S - out.RKE2 = in.RKE2 + in.K3S.DeepCopyInto(&out.K3S) + in.RKE2.DeepCopyInto(&out.RKE2) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Kubernetes. @@ -133,6 +168,13 @@ func (in *Kubernetes) DeepCopy() *Kubernetes { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *KubernetesDistribution) DeepCopyInto(out *KubernetesDistribution) { *out = *in + if in.CoreComponents != nil { + in, out := &in.CoreComponents, &out.CoreComponents + *out = make([]CoreComponent, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new KubernetesDistribution. From a705566fe9da18aee6bf7d20207c8e25def7e836 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 16:32:10 +0200 Subject: [PATCH 05/14] Introduce container comparison logic --- internal/upgrade/container.go | 41 +++++++++ internal/upgrade/container_test.go | 132 +++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+) create mode 100644 internal/upgrade/container.go create mode 100644 internal/upgrade/container_test.go diff --git a/internal/upgrade/container.go b/internal/upgrade/container.go new file mode 100644 index 0000000..5ac6f9e --- /dev/null +++ b/internal/upgrade/container.go @@ -0,0 +1,41 @@ +package upgrade + +import ( + "strings" + + corev1 "k8s.io/api/core/v1" +) + +// ContainsContainerImages validates that a given map of "container: image" refereces +// exist in a slice of corev1.Containers. +// +// Returns 'true' only if all the "container: image" references from the 'contains' map +// are present in the corev1.Containers slice. +// +// If 'strict' is true, will require for the corev1.Container.Image to be exactly the same +// as the image string defined in the 'contains' map. +// +// If 'strict' is false, will require for the corev1.Container.Image to contain the image string defined +// in the 'contains' map. Useful for use-cases where the image registry may change +// based on the environment (e.g. private regisrty). +func ContainsContainerImages(containers []corev1.Container, contains map[string]string, strict bool) bool { + foundContainers := 0 + for _, container := range containers { + image, ok := contains[container.Name] + if !ok { + // Skip containers that are not in the 'contains' map. + continue + } + foundContainers++ + + if strict && container.Image != image { + return false + } + + if !strict && !strings.Contains(container.Image, image) { + return false + } + } + + return foundContainers == len(contains) +} diff --git a/internal/upgrade/container_test.go b/internal/upgrade/container_test.go new file mode 100644 index 0000000..837875d --- /dev/null +++ b/internal/upgrade/container_test.go @@ -0,0 +1,132 @@ +package upgrade + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" +) + +func TestContainsContainerImagesStrict(t *testing.T) { + additionalStrictTest := ContainersImageTest{ + Name: "Container image registry mismatch", + Containers: []corev1.Container{ + { + Name: "coredns", + Image: "foo.bar:8080/rancher/mirrored-coredns-coredns:1.11.3", + }, + }, + ContainerImages: map[string]string{ + "coredns": "rancher/mirrored-coredns-coredns:1.11.3", + }, + ExpectedResult: false, + } + + allTests := append(getDefaultContainerImageTests(), additionalStrictTest) + + for _, test := range allTests { + t.Run(test.Name, func(t *testing.T) { + if test.ExpectedResult { + assert.True(t, ContainsContainerImages(test.Containers, test.ContainerImages, true)) + } else { + assert.False(t, ContainsContainerImages(test.Containers, test.ContainerImages, true)) + } + }) + } +} + +func TestContainsContainerImagesLenient(t *testing.T) { + additionalLenientTest := ContainersImageTest{ + Name: "Container image registry mismatch", + Containers: []corev1.Container{ + { + Name: "coredns", + Image: "foo.bar:8080/rancher/mirrored-coredns-coredns:1.11.3", + }, + }, + ContainerImages: map[string]string{ + "coredns": "rancher/mirrored-coredns-coredns:1.11.3", + }, + ExpectedResult: true, + } + + allTests := append(getDefaultContainerImageTests(), additionalLenientTest) + + for _, test := range allTests { + t.Run(test.Name, func(t *testing.T) { + if test.ExpectedResult { + assert.True(t, ContainsContainerImages(test.Containers, test.ContainerImages, false)) + } else { + assert.False(t, ContainsContainerImages(test.Containers, test.ContainerImages, false)) + } + }) + } +} + +type ContainersImageTest struct { + Name string + Containers []corev1.Container + ContainerImages map[string]string + ExpectedResult bool +} + +func getDefaultContainerImageTests() []ContainersImageTest { + return []ContainersImageTest{ + { + Name: "Missing container image for comparison", + Containers: []corev1.Container{ + { + Name: "foo", + Image: "bar/baz:0.0.0", + }, + }, + ContainerImages: map[string]string{ + "coredns": "rancher/mirrored-coredns-coredns:1.11.3", + }, + ExpectedResult: false, + }, + { + Name: "Container image version mismatch", + Containers: []corev1.Container{ + { + Name: "coredns", + Image: "rancher/mirrored-coredns-coredns:1.11.2", + }, + }, + ContainerImages: map[string]string{ + "coredns": "rancher/mirrored-coredns-coredns:1.11.3", + }, + ExpectedResult: false, + }, + { + Name: "Matching container image without sidecar injection", + Containers: []corev1.Container{ + { + Name: "coredns", + Image: "rancher/mirrored-coredns-coredns:1.11.3", + }, + }, + ContainerImages: map[string]string{ + "coredns": "rancher/mirrored-coredns-coredns:1.11.3", + }, + ExpectedResult: true, + }, + { + Name: "Matching container image with sidecar injection", + Containers: []corev1.Container{ + { + Name: "coredns", + Image: "rancher/mirrored-coredns-coredns:1.11.3", + }, + { + Name: "sidecar", + Image: "foo/bar:0.0.0", + }, + }, + ContainerImages: map[string]string{ + "coredns": "rancher/mirrored-coredns-coredns:1.11.3", + }, + ExpectedResult: true, + }, + } +} From e01e322f7f7a2fc8848923dcf11ff4eda22bc3f3 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 17:01:46 +0200 Subject: [PATCH 06/14] Update variable name to improve reusability --- cmd/main.go | 6 +++--- internal/controller/helm.go | 4 ++-- internal/upgrade/base.go | 4 ++-- internal/upgrade/helm.go | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 0b135a0..bf47530 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -128,9 +128,9 @@ func main() { var watchNamespaces map[string]cache.Config if watchNamespace != "" { watchNamespaces = map[string]cache.Config{ - watchNamespace: {}, - upgrade.HelmChartNamespace: {}, - upgrade.SUCNamespace: {}, + watchNamespace: {}, + upgrade.KubeSystemNamespace: {}, + upgrade.SUCNamespace: {}, } } diff --git a/internal/controller/helm.go b/internal/controller/helm.go index aee0cc0..9987c1f 100644 --- a/internal/controller/helm.go +++ b/internal/controller/helm.go @@ -137,7 +137,7 @@ func (r *UpgradePlanReconciler) createHelmChart(ctx context.Context, upgradePlan }, ObjectMeta: metav1.ObjectMeta{ Name: installedChart.Name, - Namespace: upgrade.HelmChartNamespace, + Namespace: upgrade.KubeSystemNamespace, Labels: labels, Annotations: annotations, }, @@ -256,7 +256,7 @@ func (r *UpgradePlanReconciler) upgradeHelmChart(ctx context.Context, upgradePla } job := &batchv1.Job{} - if err = r.Get(ctx, types.NamespacedName{Name: chart.Status.JobName, Namespace: upgrade.HelmChartNamespace}, job); err != nil { + if err = r.Get(ctx, types.NamespacedName{Name: chart.Status.JobName, Namespace: upgrade.KubeSystemNamespace}, job); err != nil { return upgrade.ChartStateUnknown, client.IgnoreNotFound(err) } diff --git a/internal/upgrade/base.go b/internal/upgrade/base.go index 1266998..74c028d 100644 --- a/internal/upgrade/base.go +++ b/internal/upgrade/base.go @@ -18,8 +18,8 @@ const ( ControlPlaneLabel = "node-role.kubernetes.io/control-plane" - HelmChartNamespace = "kube-system" - SUCNamespace = "cattle-system" + KubeSystemNamespace = "kube-system" + SUCNamespace = "cattle-system" controlPlaneKey = "control-plane" workersKey = "workers" diff --git a/internal/upgrade/helm.go b/internal/upgrade/helm.go index 3df7fc7..4b37c47 100644 --- a/internal/upgrade/helm.go +++ b/internal/upgrade/helm.go @@ -9,7 +9,7 @@ import ( func ChartNamespacedName(chart string) types.NamespacedName { return types.NamespacedName{ Name: chart, - Namespace: HelmChartNamespace, + Namespace: KubeSystemNamespace, } } From df5de5b7e4c126a6396cbbb3abfa60bdd4f812fb Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 17:03:18 +0200 Subject: [PATCH 07/14] Add deployment monitoring permissions to reconciler --- config/rbac/role.yaml | 8 ++++++++ internal/controller/upgradeplan_controller.go | 1 + 2 files changed, 9 insertions(+) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 8012153..d9b2e4a 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -27,6 +27,14 @@ rules: - customresourcedefinitions verbs: - get +- apiGroups: + - apps + resources: + - deployments + verbs: + - get + - list + - watch - apiGroups: - batch resources: diff --git a/internal/controller/upgradeplan_controller.go b/internal/controller/upgradeplan_controller.go index ffb74b3..ea04380 100644 --- a/internal/controller/upgradeplan_controller.go +++ b/internal/controller/upgradeplan_controller.go @@ -64,6 +64,7 @@ type UpgradePlanReconciler struct { // +kubebuilder:rbac:groups=upgrade.cattle.io,resources=plans,verbs=create;list;get;watch;delete // +kubebuilder:rbac:groups="",resources=nodes,verbs=watch;list // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;delete;create;watch +// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch // +kubebuilder:rbac:groups=core,resources=events,verbs=create;patch // +kubebuilder:rbac:groups=batch,resources=jobs,verbs=get;list;watch;create // +kubebuilder:rbac:groups=batch,resources=jobs/status,verbs=get From d65bfd7d4c9f829519ec284272a736f5d54990d7 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 17:03:45 +0200 Subject: [PATCH 08/14] Introduce helm release comparison function --- internal/controller/helm.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/controller/helm.go b/internal/controller/helm.go index 9987c1f..6114697 100644 --- a/internal/controller/helm.go +++ b/internal/controller/helm.go @@ -69,6 +69,15 @@ func retrieveHelmRelease(name string) (*helmrelease.Release, error) { return helmRelease, nil } +func compareChartReleaseWithVersion(releaseName string, version string) (bool, error) { + helmRelease, err := retrieveHelmRelease(releaseName) + if err != nil { + return false, fmt.Errorf("retrieving helm release: %w", err) + } + + return helmRelease.Chart.Metadata.Version == version, nil +} + // Updates an existing HelmChart resource in order to trigger an upgrade. func (r *UpgradePlanReconciler) updateHelmChart(ctx context.Context, upgradePlan *lifecyclev1alpha1.UpgradePlan, chart *helmcattlev1.HelmChart, releaseChart *lifecyclev1alpha1.HelmChart) error { backoffLimit := int32(6) From b60cab2e130bde50dcfd363985b65bd8ceb9d90c Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 17:07:05 +0200 Subject: [PATCH 09/14] Update function to parse K8s distribution --- internal/controller/reconcile_kubernetes.go | 22 +++++++------- .../controller/reconcile_kubernetes_test.go | 30 +++++++++---------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/internal/controller/reconcile_kubernetes.go b/internal/controller/reconcile_kubernetes.go index 65eaab1..4476514 100644 --- a/internal/controller/reconcile_kubernetes.go +++ b/internal/controller/reconcile_kubernetes.go @@ -24,16 +24,16 @@ func (r *UpgradePlanReconciler) reconcileKubernetes( ) (ctrl.Result, error) { nameSuffix := upgradePlan.Status.SUCNameSuffix - kubernetesVersion, err := targetKubernetesVersion(nodeList, kubernetes) + k8sDistro, err := targetKubernetesDistribution(nodeList, kubernetes) if err != nil { - return ctrl.Result{}, fmt.Errorf("identifying target kubernetes version: %w", err) + return ctrl.Result{}, fmt.Errorf("identifying target kubernetes distribution: %w", err) } conditionType := lifecyclev1alpha1.KubernetesUpgradedCondition identifierLabels := upgrade.PlanIdentifierLabels(upgradePlan.Name, upgradePlan.Namespace) drainControlPlane, drainWorker := parseDrainOptions(nodeList, upgradePlan) - controlPlanePlan := upgrade.KubernetesControlPlanePlan(nameSuffix, kubernetesVersion, drainControlPlane, identifierLabels) + controlPlanePlan := upgrade.KubernetesControlPlanePlan(nameSuffix, k8sDistro.Version, drainControlPlane, identifierLabels) if err = r.Get(ctx, client.ObjectKeyFromObject(controlPlanePlan), controlPlanePlan); err != nil { if !errors.IsNotFound(err) { return ctrl.Result{}, err @@ -48,7 +48,7 @@ func (r *UpgradePlanReconciler) reconcileKubernetes( return ctrl.Result{}, err } - if !isKubernetesUpgraded(nodes, kubernetesVersion) { + if !isKubernetesUpgraded(nodes, k8sDistro.Version) { setInProgressCondition(upgradePlan, conditionType, "Control plane nodes are being upgraded") return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil } else if controlPlaneOnlyCluster(nodeList) { @@ -56,7 +56,7 @@ func (r *UpgradePlanReconciler) reconcileKubernetes( return ctrl.Result{Requeue: true}, nil } - workerPlan := upgrade.KubernetesWorkerPlan(nameSuffix, kubernetesVersion, drainWorker, identifierLabels) + workerPlan := upgrade.KubernetesWorkerPlan(nameSuffix, k8sDistro.Version, drainWorker, identifierLabels) if err = r.Get(ctx, client.ObjectKeyFromObject(workerPlan), workerPlan); err != nil { if !errors.IsNotFound(err) { return ctrl.Result{}, err @@ -71,7 +71,7 @@ func (r *UpgradePlanReconciler) reconcileKubernetes( return ctrl.Result{}, err } - if !isKubernetesUpgraded(nodes, kubernetesVersion) { + if !isKubernetesUpgraded(nodes, k8sDistro.Version) { setInProgressCondition(upgradePlan, conditionType, "Worker nodes are being upgraded") return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil } @@ -80,20 +80,20 @@ func (r *UpgradePlanReconciler) reconcileKubernetes( return ctrl.Result{Requeue: true}, nil } -func targetKubernetesVersion(nodeList *corev1.NodeList, kubernetes *lifecyclev1alpha1.Kubernetes) (string, error) { +func targetKubernetesDistribution(nodeList *corev1.NodeList, kubernetes *lifecyclev1alpha1.Kubernetes) (*lifecyclev1alpha1.KubernetesDistribution, error) { if len(nodeList.Items) == 0 { - return "", fmt.Errorf("unable to determine current kubernetes version due to empty node list") + return nil, fmt.Errorf("unable to determine current kubernetes distribution due to empty node list") } kubeletVersion := nodeList.Items[0].Status.NodeInfo.KubeletVersion switch { case strings.Contains(kubeletVersion, "k3s"): - return kubernetes.K3S.Version, nil + return &kubernetes.K3S, nil case strings.Contains(kubeletVersion, "rke2"): - return kubernetes.RKE2.Version, nil + return &kubernetes.RKE2, nil default: - return "", fmt.Errorf("upgrading from kubernetes version %s is not supported", kubeletVersion) + return nil, fmt.Errorf("unsupported kubernetes distribution detected in version %s", kubeletVersion) } } diff --git a/internal/controller/reconcile_kubernetes_test.go b/internal/controller/reconcile_kubernetes_test.go index 518ec76..74b3488 100644 --- a/internal/controller/reconcile_kubernetes_test.go +++ b/internal/controller/reconcile_kubernetes_test.go @@ -212,7 +212,7 @@ func TestControlPlaneOnlyCluster(t *testing.T) { })) } -func TestTargetKubernetesVersion(t *testing.T) { +func TestTargetKubernetesDistribution(t *testing.T) { kubernetes := &lifecyclev1alpha1.Kubernetes{ K3S: lifecyclev1alpha1.KubernetesDistribution{ Version: "v1.30.3+k3s1", @@ -223,49 +223,49 @@ func TestTargetKubernetesVersion(t *testing.T) { } tests := []struct { - name string - nodes *corev1.NodeList - expectedVersion string - expectedError string + name string + nodes *corev1.NodeList + expectedDistribution *lifecyclev1alpha1.KubernetesDistribution + expectedError string }{ { name: "Empty node list", nodes: &corev1.NodeList{}, - expectedError: "unable to determine current kubernetes version due to empty node list", + expectedError: "unable to determine current kubernetes distribution due to empty node list", }, { - name: "Unsupported Kubernetes version", + name: "Unsupported Kubernetes distribution", nodes: &corev1.NodeList{ Items: []corev1.Node{{Status: corev1.NodeStatus{NodeInfo: corev1.NodeSystemInfo{KubeletVersion: "v1.30.3"}}}}, }, - expectedError: "upgrading from kubernetes version v1.30.3 is not supported", + expectedError: "unsupported kubernetes distribution detected in version v1.30.3", }, { - name: "Target k3s version", + name: "Target k3s distribution", nodes: &corev1.NodeList{ Items: []corev1.Node{{Status: corev1.NodeStatus{NodeInfo: corev1.NodeSystemInfo{KubeletVersion: "v1.28.12+k3s1"}}}}, }, - expectedVersion: "v1.30.3+k3s1", + expectedDistribution: &kubernetes.K3S, }, { - name: "Target RKE2 version", + name: "Target RKE2 distribution", nodes: &corev1.NodeList{ Items: []corev1.Node{{Status: corev1.NodeStatus{NodeInfo: corev1.NodeSystemInfo{KubeletVersion: "v1.28.12+rke2r1"}}}}, }, - expectedVersion: "v1.30.3+rke2r1", + expectedDistribution: &kubernetes.RKE2, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - version, err := targetKubernetesVersion(test.nodes, kubernetes) + k8sDistro, err := targetKubernetesDistribution(test.nodes, kubernetes) if test.expectedError != "" { require.Error(t, err) assert.EqualError(t, err, test.expectedError) - assert.Empty(t, version) + assert.Nil(t, k8sDistro) } else { require.NoError(t, err) - assert.Equal(t, test.expectedVersion, version) + assert.Equal(t, test.expectedDistribution, k8sDistro) } }) } From b9c326b53def6e3e64bcc59138e750659e36f167 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 17:10:43 +0200 Subject: [PATCH 10/14] Introduce wait mechanism for K8s core components --- internal/controller/reconcile_kubernetes.go | 87 +++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/internal/controller/reconcile_kubernetes.go b/internal/controller/reconcile_kubernetes.go index 4476514..244cced 100644 --- a/internal/controller/reconcile_kubernetes.go +++ b/internal/controller/reconcile_kubernetes.go @@ -3,15 +3,20 @@ package controller import ( "context" "fmt" + "slices" "strings" "time" + helmcattlev1 "github.com/k3s-io/helm-controller/pkg/apis/helm.cattle.io/v1" lifecyclev1alpha1 "github.com/suse-edge/upgrade-controller/api/v1alpha1" "github.com/suse-edge/upgrade-controller/internal/upgrade" + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -52,6 +57,17 @@ func (r *UpgradePlanReconciler) reconcileKubernetes( setInProgressCondition(upgradePlan, conditionType, "Control plane nodes are being upgraded") return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil } else if controlPlaneOnlyCluster(nodeList) { + allUpgraded, waitingFor, err := r.getK8sCoreComponentsUpgradeStatus(ctx, k8sDistro.CoreComponents) + if err != nil { + return ctrl.Result{}, err + } + + if !allUpgraded { + msg := fmt.Sprintf("Waiting for %s core component to be upgraded", waitingFor) + setInProgressCondition(upgradePlan, conditionType, msg) + return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil + } + setSuccessfulCondition(upgradePlan, conditionType, "All cluster nodes are upgraded") return ctrl.Result{Requeue: true}, nil } @@ -76,10 +92,81 @@ func (r *UpgradePlanReconciler) reconcileKubernetes( return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil } + allUpgraded, waitingFor, err := r.getK8sCoreComponentsUpgradeStatus(ctx, k8sDistro.CoreComponents) + if err != nil { + return ctrl.Result{}, err + } + + if !allUpgraded { + msg := fmt.Sprintf("Waiting for %s core component to be upgraded", waitingFor) + setInProgressCondition(upgradePlan, conditionType, msg) + return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil + } + setSuccessfulCondition(upgradePlan, conditionType, "All cluster nodes are upgraded") return ctrl.Result{Requeue: true}, nil } +func (r *UpgradePlanReconciler) getK8sCoreComponentsUpgradeStatus(ctx context.Context, core []lifecyclev1alpha1.CoreComponent) (allUpgraded bool, waitingFor string, err error) { + for _, component := range core { + upgraded, err := r.isK8sCoreComponentUpgraded(ctx, &component) + if err != nil { + if errors.IsNotFound(err) { + // Not every component from the core list might be + // on every cluster. + continue + } + return false, "", fmt.Errorf("validating upgrade for component %s: %w", component.Name, err) + } + + if !upgraded { + return false, component.Name, nil + } + } + + return true, "", nil +} + +func (r *UpgradePlanReconciler) isK8sCoreComponentUpgraded(ctx context.Context, component *lifecyclev1alpha1.CoreComponent) (bool, error) { + switch component.Type { + case lifecyclev1alpha1.HelmChartType: + chart := &helmcattlev1.HelmChart{} + if err := r.Get(ctx, upgrade.ChartNamespacedName(component.Name), chart); err != nil { + return false, fmt.Errorf("getting %s helm chart: %w", component.Name, err) + } + + chartJob := &batchv1.Job{} + if err := r.Get(ctx, types.NamespacedName{Name: chart.Status.JobName, Namespace: chart.Namespace}, chartJob); err != nil { + return false, fmt.Errorf("getting %s helm chart job: %w", chart.Name, err) + } + + isJobComplete := func(conditions []batchv1.JobCondition) bool { + return slices.ContainsFunc(conditions, func(condition batchv1.JobCondition) bool { + return condition.Status == corev1.ConditionTrue && condition.Type == batchv1.JobComplete + }) + } + + if !isJobComplete(chartJob.Status.Conditions) { + return false, nil + } + + return compareChartReleaseWithVersion(chart.Name, component.Version) + case lifecyclev1alpha1.DeploymentType: + dep := &appsv1.Deployment{} + if err := r.Get(ctx, types.NamespacedName{Name: component.Name, Namespace: upgrade.KubeSystemNamespace}, dep); err != nil { + return false, fmt.Errorf("getting %s deployment: %w", component.Name, err) + } + + if dep.Status.Replicas != dep.Status.ReadyReplicas { + return false, nil + } + + return upgrade.ContainsContainerImages(dep.Spec.Template.Spec.Containers, component.ConvertContainerSliceToMap(), false), nil + default: + return false, fmt.Errorf("unsupported component type: %s", component.Type) + } +} + func targetKubernetesDistribution(nodeList *corev1.NodeList, kubernetes *lifecyclev1alpha1.Kubernetes) (*lifecyclev1alpha1.KubernetesDistribution, error) { if len(nodeList.Items) == 0 { return nil, fmt.Errorf("unable to determine current kubernetes distribution due to empty node list") From 55a433a11325fbc3f84effe3260032c9f2d0e983 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Thu, 5 Dec 2024 17:12:19 +0200 Subject: [PATCH 11/14] Fix typos --- internal/upgrade/container.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/upgrade/container.go b/internal/upgrade/container.go index 5ac6f9e..6060a30 100644 --- a/internal/upgrade/container.go +++ b/internal/upgrade/container.go @@ -6,7 +6,7 @@ import ( corev1 "k8s.io/api/core/v1" ) -// ContainsContainerImages validates that a given map of "container: image" refereces +// ContainsContainerImages validates that a given map of "container: image" references // exist in a slice of corev1.Containers. // // Returns 'true' only if all the "container: image" references from the 'contains' map @@ -17,7 +17,7 @@ import ( // // If 'strict' is false, will require for the corev1.Container.Image to contain the image string defined // in the 'contains' map. Useful for use-cases where the image registry may change -// based on the environment (e.g. private regisrty). +// based on the environment (e.g. private registry). func ContainsContainerImages(containers []corev1.Container, contains map[string]string, strict bool) bool { foundContainers := 0 for _, container := range containers { From 801fec33001c3e226c6f9695d188fe46492f273d Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Mon, 9 Dec 2024 15:33:22 +0200 Subject: [PATCH 12/14] Don't fail on job not found errors --- internal/controller/reconcile_kubernetes.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/controller/reconcile_kubernetes.go b/internal/controller/reconcile_kubernetes.go index 244cced..cef1f6d 100644 --- a/internal/controller/reconcile_kubernetes.go +++ b/internal/controller/reconcile_kubernetes.go @@ -137,7 +137,10 @@ func (r *UpgradePlanReconciler) isK8sCoreComponentUpgraded(ctx context.Context, chartJob := &batchv1.Job{} if err := r.Get(ctx, types.NamespacedName{Name: chart.Status.JobName, Namespace: chart.Namespace}, chartJob); err != nil { - return false, fmt.Errorf("getting %s helm chart job: %w", chart.Name, err) + // If the HelmChart exists it must have a Job attatched to it. + // If the Job is missing, this indicates a recreate of the Job has been issued + // which can take some time on slower machines. + return false, client.IgnoreNotFound(err) } isJobComplete := func(conditions []batchv1.JobCondition) bool { From 2de5dbb07137fdf62027ac939f6f51d89b7fe865 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Wed, 11 Dec 2024 14:20:22 +0200 Subject: [PATCH 13/14] Check deployment staus conditions --- internal/controller/reconcile_kubernetes.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/controller/reconcile_kubernetes.go b/internal/controller/reconcile_kubernetes.go index cef1f6d..16412b9 100644 --- a/internal/controller/reconcile_kubernetes.go +++ b/internal/controller/reconcile_kubernetes.go @@ -160,7 +160,13 @@ func (r *UpgradePlanReconciler) isK8sCoreComponentUpgraded(ctx context.Context, return false, fmt.Errorf("getting %s deployment: %w", component.Name, err) } - if dep.Status.Replicas != dep.Status.ReadyReplicas { + isDeploymentAvailable := func(conditions []appsv1.DeploymentCondition) bool { + return slices.ContainsFunc(conditions, func(condition appsv1.DeploymentCondition) bool { + return condition.Status == corev1.ConditionTrue && condition.Type == appsv1.DeploymentAvailable + }) + } + + if !isDeploymentAvailable(dep.Status.Conditions) { return false, nil } From 73b14414ddd639c64a61221ad3dfe8a3b0f0c9b1 Mon Sep 17 00:00:00 2001 From: Ivo Petrov Date: Wed, 11 Dec 2024 14:22:19 +0200 Subject: [PATCH 14/14] Fix typos --- internal/controller/reconcile_kubernetes.go | 2 +- internal/upgrade/container_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/controller/reconcile_kubernetes.go b/internal/controller/reconcile_kubernetes.go index 16412b9..ec8f05a 100644 --- a/internal/controller/reconcile_kubernetes.go +++ b/internal/controller/reconcile_kubernetes.go @@ -137,7 +137,7 @@ func (r *UpgradePlanReconciler) isK8sCoreComponentUpgraded(ctx context.Context, chartJob := &batchv1.Job{} if err := r.Get(ctx, types.NamespacedName{Name: chart.Status.JobName, Namespace: chart.Namespace}, chartJob); err != nil { - // If the HelmChart exists it must have a Job attatched to it. + // If the HelmChart exists it must have a Job attached to it. // If the Job is missing, this indicates a recreate of the Job has been issued // which can take some time on slower machines. return false, client.IgnoreNotFound(err) diff --git a/internal/upgrade/container_test.go b/internal/upgrade/container_test.go index 837875d..982110d 100644 --- a/internal/upgrade/container_test.go +++ b/internal/upgrade/container_test.go @@ -99,7 +99,7 @@ func getDefaultContainerImageTests() []ContainersImageTest { ExpectedResult: false, }, { - Name: "Matching container image without sidecar injection", + Name: "Matching container image without additional images", Containers: []corev1.Container{ { Name: "coredns", @@ -112,7 +112,7 @@ func getDefaultContainerImageTests() []ContainersImageTest { ExpectedResult: true, }, { - Name: "Matching container image with sidecar injection", + Name: "Matching container image with additional images", Containers: []corev1.Container{ { Name: "coredns",