From 7fe0c42f7841b9a236b775994ac0ae04b144c878 Mon Sep 17 00:00:00 2001 From: Hardik Dodiya Date: Mon, 24 Jun 2024 13:43:33 +0000 Subject: [PATCH 1/4] Update IgnitionSecretRef to ObjectRef --- api/v1alpha1/httpbootconfig_types.go | 8 +-- api/v1alpha1/zz_generated.deepcopy.go | 2 +- .../boot.ironcore.dev_httpbootconfigs.yaml | 59 ++++++++++++++++--- go.mod | 2 +- go.sum | 4 +- ...achinebootconfiguration_http_controller.go | 2 +- ...serverbootconfiguration_http_controller.go | 2 +- server/bootserver.go | 2 +- 8 files changed, 62 insertions(+), 19 deletions(-) diff --git a/api/v1alpha1/httpbootconfig_types.go b/api/v1alpha1/httpbootconfig_types.go index d77c2086..b87f4e2d 100644 --- a/api/v1alpha1/httpbootconfig_types.go +++ b/api/v1alpha1/httpbootconfig_types.go @@ -10,10 +10,10 @@ import ( // HTTPBootConfigSpec defines the desired state of HTTPBootConfig type HTTPBootConfigSpec struct { - SystemUUID string `json:"systemUUID,omitempty"` - IgnitionSecretRef *corev1.LocalObjectReference `json:"ignitionSecretRef,omitempty"` - SystemIPs []string `json:"systemIPs,omitempty"` - UKIURL string `json:"ukiURL,omitempty"` + SystemUUID string `json:"systemUUID,omitempty"` + IgnitionSecretRef *corev1.ObjectReference `json:"ignitionSecretRef,omitempty"` + SystemIPs []string `json:"systemIPs,omitempty"` + UKIURL string `json:"ukiURL,omitempty"` } // HTTPBootConfigStatus defines the observed state of HTTPBootConfig diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 2a926ef2..fde51394 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -76,7 +76,7 @@ func (in *HTTPBootConfigSpec) DeepCopyInto(out *HTTPBootConfigSpec) { *out = *in if in.IgnitionSecretRef != nil { in, out := &in.IgnitionSecretRef, &out.IgnitionSecretRef - *out = new(v1.LocalObjectReference) + *out = new(v1.ObjectReference) **out = **in } if in.SystemIPs != nil { diff --git a/config/crd/bases/boot.ironcore.dev_httpbootconfigs.yaml b/config/crd/bases/boot.ironcore.dev_httpbootconfigs.yaml index d9f007bd..3c1480df 100644 --- a/config/crd/bases/boot.ironcore.dev_httpbootconfigs.yaml +++ b/config/crd/bases/boot.ironcore.dev_httpbootconfigs.yaml @@ -48,19 +48,62 @@ spec: properties: ignitionSecretRef: description: |- - LocalObjectReference contains enough information to let you locate the - referenced object inside the same namespace. + ObjectReference contains enough information to let you inspect or modify the referred object. + --- + New uses of this type are discouraged because of difficulty describing its usage when embedded in APIs. + 1. Ignored fields. It includes many fields which are not generally honored. For instance, ResourceVersion and FieldPath are both very rarely valid in actual usage. + 2. Invalid usage help. It is impossible to add specific help for individual usage. In most embedded usages, there are particular + restrictions like, "must refer only to types A and B" or "UID not honored" or "name must be restricted". + Those cannot be well described when embedded. + 3. Inconsistent validation. Because the usages are different, the validation rules are different by usage, which makes it hard for users to predict what will happen. + 4. The fields are both imprecise and overly precise. Kind is not a precise mapping to a URL. This can produce ambiguity + during interpretation and require a REST mapping. In most cases, the dependency is on the group,resource tuple + and the version of the actual struct is irrelevant. + 5. We cannot easily change it. Because this type is embedded in many locations, updates to this type + will affect numerous schemas. Don't make new APIs embed an underspecified API type they do not control. + + + Instead of using this type, create a locally provided and used type that is well-focused on your reference. + For example, ServiceReferences for admission registration: https://github.com/kubernetes/api/blob/release-1.17/admissionregistration/v1/types.go#L533 . properties: + apiVersion: + description: API version of the referent. + type: string + fieldPath: + description: |- + If referring to a piece of an object instead of an entire object, this string + should contain a valid JSON/Go field access statement, such as desiredState.manifest.containers[2]. + For example, if the object reference is to a container within a pod, this would take on a value like: + "spec.containers{name}" (where "name" refers to the name of the container that triggered + the event) or if no container name is specified "spec.containers[2]" (container with + index 2 in this pod). This syntax is chosen only to have some well-defined way of + referencing a part of an object. + TODO: this design is not final and this field is subject to change in the future. + type: string + kind: + description: |- + Kind of the referent. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string name: - default: "" description: |- Name of the referent. - This field is effectively required, but due to backwards compatibility is - allowed to be empty. Instances of this type with an empty value here are - almost certainly wrong. - TODO: Add other useful fields. apiVersion, kind, uid? More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names - TODO: Drop `kubebuilder:default` when controller-gen doesn't need it https://github.com/kubernetes-sigs/kubebuilder/issues/3896. + type: string + namespace: + description: |- + Namespace of the referent. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/ + type: string + resourceVersion: + description: |- + Specific resourceVersion to which this reference is made, if any. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#concurrency-control-and-consistency + type: string + uid: + description: |- + UID of the referent. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids type: string type: object x-kubernetes-map-type: atomic diff --git a/go.mod b/go.mod index d57a4117..4436833b 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/coreos/butane v0.21.0 github.com/go-logr/logr v1.4.2 github.com/ironcore-dev/controller-utils v0.9.3 - github.com/ironcore-dev/metal v0.0.0-20240624081522-bfe4e65f0385 + github.com/ironcore-dev/metal v0.0.0-20240624131301-18385f342755 github.com/onsi/ginkgo/v2 v2.19.0 github.com/onsi/gomega v1.33.1 k8s.io/api v0.30.2 diff --git a/go.sum b/go.sum index 9a9a7dab..3ae4c4ca 100644 --- a/go.sum +++ b/go.sum @@ -67,8 +67,8 @@ github.com/imdario/mergo v0.3.16 h1:wwQJbIsHYGMUyLSPrEq1CT16AhnhNJQ51+4fdHUnCl4= github.com/imdario/mergo v0.3.16/go.mod h1:WBLT9ZmE3lPoWsEzCh9LPo3TiwVN+ZKEjmz+hD27ysY= github.com/ironcore-dev/controller-utils v0.9.3 h1:sTrnxSzX5RrLf4B8KrAH2axSC+gxfJXphkV6df2GSsw= github.com/ironcore-dev/controller-utils v0.9.3/go.mod h1:djKnxDs0Hwxhhc0VmVY8tZnrOrElvrRV2jov/LiCZ2Y= -github.com/ironcore-dev/metal v0.0.0-20240624081522-bfe4e65f0385 h1:I7lMRc6HSOUa2YCYP926uogP13U/FBLx5LOrYoa47iQ= -github.com/ironcore-dev/metal v0.0.0-20240624081522-bfe4e65f0385/go.mod h1:+/bmkghOE7acqXDT/LDH57RemaUzlVwnQjttsOjdoyg= +github.com/ironcore-dev/metal v0.0.0-20240624131301-18385f342755 h1:EmR3Ngg2wmOXJkxgsdYVuPXLRfwWmO2Fi+htjih6QGY= +github.com/ironcore-dev/metal v0.0.0-20240624131301-18385f342755/go.mod h1:+/bmkghOE7acqXDT/LDH57RemaUzlVwnQjttsOjdoyg= github.com/josharian/intern v1.0.0 h1:vlS4z54oSdjm0bgjRigI+G1HpF+tI+9rE5LLzOg8HmY= github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y= github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= diff --git a/internal/controller/machinebootconfiguration_http_controller.go b/internal/controller/machinebootconfiguration_http_controller.go index 1feb2b79..d587e70a 100644 --- a/internal/controller/machinebootconfiguration_http_controller.go +++ b/internal/controller/machinebootconfiguration_http_controller.go @@ -90,7 +90,7 @@ func (r *MachineBootConfigurationHTTPReconciler) reconcile(ctx context.Context, SystemUUID: systemUUID, SystemIPs: systemIPs, UKIURL: ukiURL, - IgnitionSecretRef: &corev1.LocalObjectReference{Name: config.Spec.IgnitionSecretRef.Name}, + IgnitionSecretRef: &corev1.ObjectReference{Name: config.Spec.IgnitionSecretRef.Name, Namespace: config.Spec.IgnitionSecretRef.Namespace}, }, } diff --git a/internal/controller/serverbootconfiguration_http_controller.go b/internal/controller/serverbootconfiguration_http_controller.go index 0cd4e8ac..6a7ad479 100644 --- a/internal/controller/serverbootconfiguration_http_controller.go +++ b/internal/controller/serverbootconfiguration_http_controller.go @@ -89,7 +89,7 @@ func (r *ServerBootConfigurationHTTPReconciler) reconcile(ctx context.Context, l SystemUUID: systemUUID, SystemIPs: systemIPs, UKIURL: ukiURL, - IgnitionSecretRef: &corev1.LocalObjectReference{Name: config.Spec.IgnitionSecretRef.Name}, + IgnitionSecretRef: &corev1.ObjectReference{Name: config.Spec.IgnitionSecretRef.Name, Namespace: config.Namespace}, }, } diff --git a/server/bootserver.go b/server/bootserver.go index 8e10f217..f57e63e8 100644 --- a/server/bootserver.go +++ b/server/bootserver.go @@ -62,7 +62,7 @@ func handleIgnitionHTTPBoot(w http.ResponseWriter, r *http.Request, k8sClient cl HTTPBootConfig := HTTPBootConfigList.Items[0] ignitionSecret := &corev1.Secret{} - if err := k8sClient.Get(ctx, client.ObjectKey{Name: HTTPBootConfig.Spec.IgnitionSecretRef.Name, Namespace: HTTPBootConfig.Namespace}, ignitionSecret); err != nil { + if err := k8sClient.Get(ctx, client.ObjectKey{Name: HTTPBootConfig.Spec.IgnitionSecretRef.Name, Namespace: HTTPBootConfig.Spec.IgnitionSecretRef.Namespace}, ignitionSecret); err != nil { http.Error(w, "Resource Not Found", http.StatusNotFound) log.Info("Error: Failed to get Ignition secret", "error", err.Error()) return From 62a4eae8a08404e5ad8b235f0c2aa3364a574dcc Mon Sep 17 00:00:00 2001 From: Hardik Dodiya Date: Mon, 24 Jun 2024 13:57:55 +0000 Subject: [PATCH 2/4] Ensure Ignition with Ref's Namespace --- internal/controller/httpbootconfig_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/httpbootconfig_controller.go b/internal/controller/httpbootconfig_controller.go index dc238b4d..b4524a51 100644 --- a/internal/controller/httpbootconfig_controller.go +++ b/internal/controller/httpbootconfig_controller.go @@ -75,7 +75,7 @@ func (r *HTTPBootConfigReconciler) ensureIgnition(ctx context.Context, _ logr.Lo // Verify if the IgnitionRef is set, and it has the intended data key. if HTTPBootConfig.Spec.IgnitionSecretRef != nil { IgnitionSecret := &corev1.Secret{} - if err := r.Get(ctx, client.ObjectKey{Name: HTTPBootConfig.Spec.IgnitionSecretRef.Name, Namespace: HTTPBootConfig.Namespace}, IgnitionSecret); err != nil { + if err := r.Get(ctx, client.ObjectKey{Name: HTTPBootConfig.Spec.IgnitionSecretRef.Name, Namespace: HTTPBootConfig.Spec.IgnitionSecretRef.Namespace}, IgnitionSecret); err != nil { return bootv1alpha1.HTTPBootConfigStateError, err // TODO: Add some validation steps to ensure that the IgntionData is compliant, if necessary. // Assume for now, that it's going to json format. From 7f119e60f78cc7826fff887315dfa688eb78de4c Mon Sep 17 00:00:00 2001 From: Hardik Dodiya Date: Mon, 24 Jun 2024 15:09:16 +0000 Subject: [PATCH 3/4] Fix the Enqueue method for IgnitionSecrets --- internal/controller/httpbootconfig_controller.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/controller/httpbootconfig_controller.go b/internal/controller/httpbootconfig_controller.go index b4524a51..9d8fb1a8 100644 --- a/internal/controller/httpbootconfig_controller.go +++ b/internal/controller/httpbootconfig_controller.go @@ -130,14 +130,16 @@ func (r *HTTPBootConfigReconciler) enqueueHTTPBootConfigReferencingIgnitionSecre } list := &bootv1alpha1.HTTPBootConfigList{} - if err := r.Client.List(ctx, list, client.InNamespace(secretObj.Namespace)); err != nil { + if err := r.Client.List(ctx, list, client.InNamespace("")); err != nil { log.Error(err, "failed to list HTTPBootConfig for secret", secret) return nil } var requests []reconcile.Request for _, HTTPBootConfig := range list.Items { - if HTTPBootConfig.Spec.IgnitionSecretRef != nil && HTTPBootConfig.Spec.IgnitionSecretRef.Name == secretObj.Name { + if HTTPBootConfig.Spec.IgnitionSecretRef != nil && + HTTPBootConfig.Spec.IgnitionSecretRef.Name == secretObj.Name && + HTTPBootConfig.Spec.IgnitionSecretRef.Namespace == secretObj.Namespace { requests = append(requests, reconcile.Request{ NamespacedName: types.NamespacedName{ Name: HTTPBootConfig.Name, From d1c6dc60894245fd1bda625419129b15f724fc70 Mon Sep 17 00:00:00 2001 From: Hardik Dodiya Date: Tue, 25 Jun 2024 11:20:08 +0000 Subject: [PATCH 4/4] Add Eventfilter for ResourceVersion changes --- internal/controller/httpbootconfig_controller.go | 4 ++++ .../controller/machinebootconfiguration_http_controller.go | 2 ++ 2 files changed, 6 insertions(+) diff --git a/internal/controller/httpbootconfig_controller.go b/internal/controller/httpbootconfig_controller.go index 9d8fb1a8..ceca43b3 100644 --- a/internal/controller/httpbootconfig_controller.go +++ b/internal/controller/httpbootconfig_controller.go @@ -112,6 +112,10 @@ func (r *HTTPBootConfigReconciler) patchStatus( HTTPBootConfig *bootv1alpha1.HTTPBootConfig, state bootv1alpha1.HTTPBootConfigState, ) error { + if HTTPBootConfig.Status.State == state { + return nil + } + base := HTTPBootConfig.DeepCopy() HTTPBootConfig.Status.State = state diff --git a/internal/controller/machinebootconfiguration_http_controller.go b/internal/controller/machinebootconfiguration_http_controller.go index d587e70a..ef79d20c 100644 --- a/internal/controller/machinebootconfiguration_http_controller.go +++ b/internal/controller/machinebootconfiguration_http_controller.go @@ -16,6 +16,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/predicate" metalv1alpha1 "github.com/ironcore-dev/metal/api/v1alpha1" ) @@ -179,5 +180,6 @@ func (r *MachineBootConfigurationHTTPReconciler) SetupWithManager(mgr ctrl.Manag return ctrl.NewControllerManagedBy(mgr). For(&metalv1alpha1.BootConfiguration{}). Owns(&bootv1alpha1.HTTPBootConfig{}). + WithEventFilter(predicate.ResourceVersionChangedPredicate{}). Complete(r) }