From c4e359a95af436161644d63b7edf53158ba0e313 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Mon, 13 Dec 2021 09:23:14 -0500 Subject: [PATCH 1/5] Update to latest baremetal-operator --- go.mod | 6 +- go.sum | 12 +- .../v1alpha1/firmwareschema_types.go | 5 + .../v1alpha1/hostfirmwaresettings_types.go | 13 +- .../v1alpha1/zz_generated.deepcopy.go | 4 + .../controllers/metal3.io/action_result.go | 3 +- .../metal3.io/baremetalhost_controller.go | 48 +++- .../hostfirmwaresettings_controller.go | 235 +++++++----------- .../preprovisioningimage_controller.go | 6 + .../pkg/hardwareutils/bmc/irmc.go | 4 +- .../pkg/imageprovider/default.go | 2 +- .../pkg/imageprovider/invalid.go | 19 ++ vendor/modules.txt | 12 +- 13 files changed, 199 insertions(+), 170 deletions(-) create mode 100644 vendor/github.com/metal3-io/baremetal-operator/pkg/imageprovider/invalid.go diff --git a/go.mod b/go.mod index f7a0c5c4..7f2d49f3 100644 --- a/go.mod +++ b/go.mod @@ -24,7 +24,7 @@ require ( ) replace ( - github.com/metal3-io/baremetal-operator => github.com/openshift/baremetal-operator v0.0.0-20211202144449-e5fb40679cf0 - github.com/metal3-io/baremetal-operator/apis => github.com/openshift/baremetal-operator/apis v0.0.0-20211202144449-e5fb40679cf0 - github.com/metal3-io/baremetal-operator/pkg/hardwareutils => github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211202144449-e5fb40679cf0 + github.com/metal3-io/baremetal-operator => github.com/openshift/baremetal-operator v0.0.0-20211212164328-3070daca9ae3 + github.com/metal3-io/baremetal-operator/apis => github.com/openshift/baremetal-operator/apis v0.0.0-20211212164328-3070daca9ae3 + github.com/metal3-io/baremetal-operator/pkg/hardwareutils => github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211212164328-3070daca9ae3 ) diff --git a/go.sum b/go.sum index b3e62d4a..e6590388 100644 --- a/go.sum +++ b/go.sum @@ -513,12 +513,12 @@ github.com/onsi/gomega v1.15.0 h1:WjP/FQ/sk43MRmnEcT+MlDw2TFvkrXlprrPST/IudjU= github.com/onsi/gomega v1.15.0/go.mod h1:cIuvLEne0aoVhAgh/O6ac0Op8WWw9H6eYCriF+tEHG0= github.com/openshift/assisted-image-service v0.0.0-20211122133112-1552361c0458 h1:TDYrQaxV3PObc4pmcft15BUCbuDe0UNhzXTDZGnvzZ0= github.com/openshift/assisted-image-service v0.0.0-20211122133112-1552361c0458/go.mod h1:DSFZEsQZpIHqnV9saugs7meqdYmGKlI5FKKkDe421/g= -github.com/openshift/baremetal-operator v0.0.0-20211202144449-e5fb40679cf0 h1:ZYUtfAnTsU9EntVmflF1y6i8y05h3DI2XmkRIDy9IWg= -github.com/openshift/baremetal-operator v0.0.0-20211202144449-e5fb40679cf0/go.mod h1:p32F1DBUxfgd0JjM4rCuhJomFJokEoWR1Z/LZNL2LM8= -github.com/openshift/baremetal-operator/apis v0.0.0-20211202144449-e5fb40679cf0 h1:V1jCAv73CBOhuM/MGOoj08S7uVpdQnY0JMTqkiZMoTU= -github.com/openshift/baremetal-operator/apis v0.0.0-20211202144449-e5fb40679cf0/go.mod h1:CVSU+wS3oYrFJooMeiyDtTpatoXoKyXPE2YS5vT26vE= -github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211202144449-e5fb40679cf0 h1:9J4ZFKJvsQYg1StHmMl1M0kITHwnl2tQTLC+PeN/Wg4= -github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211202144449-e5fb40679cf0/go.mod h1:Q+r+xTc1jDcx/y61bVspJ9ANiAjJlsx/j+sL44mCB8w= +github.com/openshift/baremetal-operator v0.0.0-20211212164328-3070daca9ae3 h1:rZNvtqFsyOnxLAHcYnPM7/BB1gRWHxJ2PHsRv0HZOeI= +github.com/openshift/baremetal-operator v0.0.0-20211212164328-3070daca9ae3/go.mod h1:Nm29FDSH26opS7wK6ykg4pAjNFNlbJGUUUcB1rsUSLg= +github.com/openshift/baremetal-operator/apis v0.0.0-20211212164328-3070daca9ae3 h1:yqTZ+2P0+tslKx+KRt7eiH9XDQ5DP1GgSaV8A/Cyjq8= +github.com/openshift/baremetal-operator/apis v0.0.0-20211212164328-3070daca9ae3/go.mod h1:+60ass2P7bP6t6f1WUojQ38hi7cKHuJ3tphaYzimPlA= +github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211212164328-3070daca9ae3 h1:8TnG8Q2LZXKpoIvOg938PC68k/VpLNyhTQYV+ftrmhA= +github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211212164328-3070daca9ae3/go.mod h1:/PSTQInIZmfuOmAp/pSgZAs4txs6T49woC0MYIa4QzE= github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k= github.com/pelletier/go-toml v1.2.0 h1:T5zMGML61Wp+FlcbWjRDT7yAxhJNAiPPLOFECq181zc= diff --git a/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/firmwareschema_types.go b/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/firmwareschema_types.go index df76ff5d..67620b88 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/firmwareschema_types.go +++ b/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/firmwareschema_types.go @@ -18,6 +18,7 @@ package v1alpha1 import ( "fmt" + "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -71,6 +72,10 @@ func (schema *SettingSchema) Validate(name string, value intstr.IntOrString) err return SchemaSettingError{name: name, message: "it is ReadOnly"} } + if strings.Contains(name, "Password") { + return SchemaSettingError{name: name, message: "Password fields can't be set"} + } + // Check if valid based on type switch schema.AttributeType { case "Enumeration": diff --git a/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwaresettings_types.go b/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwaresettings_types.go index 8bca68d1..c170e967 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwaresettings_types.go +++ b/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwaresettings_types.go @@ -37,19 +37,16 @@ type SettingsConditionType string const ( // Indicates that the settings in the Spec are different than Status - UpdateRequested SettingsConditionType = "UpdateRequested" + FirmwareSettingsChangeDetected SettingsConditionType = "ChangeDetected" // Indicates if the settings are valid and can be configured on the host - SettingsValid SettingsConditionType = "Valid" + FirmwareSettingsValid SettingsConditionType = "Valid" ) // HostFirmwareSettingsSpec defines the desired state of HostFirmwareSettings type HostFirmwareSettingsSpec struct { // Settings are the desired firmware settings stored as name/value pairs. - // This will be populated with the actual firmware settings and only - // contain the settings that can be modified (i.e. not ReadOnly), to - // facilitate making changes. // +patchStrategy=merge Settings DesiredSettingsMap `json:"settings" required:"true"` } @@ -61,9 +58,13 @@ type HostFirmwareSettingsStatus struct { // Namespace as the settings but it can be overwritten in the Spec FirmwareSchema *SchemaReference `json:"schema,omitempty"` - // Settings are the actual firmware settings stored as name/value pairs + // Settings are the firmware settings stored as name/value pairs Settings SettingsMap `json:"settings" required:"true"` + // Time that the status was last updated + // +optional + LastUpdated *metav1.Time `json:"lastUpdated,omitempty"` + // Track whether settings stored in the spec are valid based on the schema // +patchMergeKey=type // +patchStrategy=merge diff --git a/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go b/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go index a352fdbb..e62c6071 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go +++ b/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go @@ -585,6 +585,10 @@ func (in *HostFirmwareSettingsStatus) DeepCopyInto(out *HostFirmwareSettingsStat (*out)[key] = val } } + if in.LastUpdated != nil { + in, out := &in.LastUpdated, &out.LastUpdated + *out = (*in).DeepCopy() + } if in.Conditions != nil { in, out := &in.Conditions, &out.Conditions *out = make([]metav1.Condition, len(*in)) diff --git a/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/action_result.go b/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/action_result.go index 94a3fbd2..33df2e5f 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/action_result.go +++ b/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/action_result.go @@ -141,8 +141,7 @@ func calculateBackoff(errorCount int) time.Duration { } base := math.Exp2(float64(errorCount)) - /* #nosec */ - backOff := base - (rand.Float64() * base * 0.5) + backOff := base - (rand.Float64() * base * 0.5) // #nosec backOffDuration := time.Duration(float64(time.Minute) * backOff) return backOffDuration } diff --git a/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/baremetalhost_controller.go b/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/baremetalhost_controller.go index a33e0734..8eb5b399 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/baremetalhost_controller.go +++ b/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/baremetalhost_controller.go @@ -55,6 +55,7 @@ const ( unmanagedRetryDelay = time.Minute * 10 preprovImageRetryDelay = time.Minute * 5 provisionerNotReadyRetryDelay = time.Second * 30 + subResourceNotReadyRetryDelay = time.Second * 60 rebootAnnotationPrefix = "reboot.metal3.io" inspectAnnotationPrefix = "inspect.metal3.io" hardwareDetailsAnnotation = inspectAnnotationPrefix + "/hardwaredetails" @@ -824,6 +825,14 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf return result } + // Create the hostFirmwareSettings resource with same host name/namespace if it doesn't exist + if info.host.Name != "" { + if err = r.createHostFirmwareSettings(info); err != nil { + info.log.Info("failed creating hostfirmwaresettings") + return actionError{errors.Wrap(err, "failed creating hostFirmwareSettings")} + } + } + // Reaching this point means the credentials are valid and worked, // so clear any previous error and record the success in the // status block. @@ -960,7 +969,8 @@ func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner, // Use settings in hostFirmwareSettings if available hfs, err := r.getHostFirmwareSettings(info) if err != nil { - info.log.Info("hostFirmwareSettings not available for cleaning") + // wait until hostFirmwareSettings are ready + return actionContinue{subResourceNotReadyRetryDelay} } if hfs != nil { prepareData.ActualFirmwareSettings = hfs.Status.Settings.DeepCopy() @@ -1334,6 +1344,38 @@ func saveHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty boo return } +func (r *BareMetalHostReconciler) createHostFirmwareSettings(info *reconcileInfo) error { + + // Check if HostFirmwareSettings already exists + hfs := &metal3v1alpha1.HostFirmwareSettings{} + if err := r.Get(context.TODO(), info.request.NamespacedName, hfs); err != nil { + if k8serrors.IsNotFound(err) { + // A resource doesn't exist, create one + hfs.ObjectMeta = metav1.ObjectMeta{ + Name: info.host.Name, + Namespace: info.host.Namespace} + hfs.Status.Settings = make(metal3v1alpha1.SettingsMap) + hfs.Spec.Settings = make(metal3v1alpha1.DesiredSettingsMap) + + // Set bmh as owner, this makes sure the resource is deleted when bmh is deleted + if err = controllerutil.SetControllerReference(info.host, hfs, r.Scheme()); err != nil { + return errors.Wrap(err, "could not set bmh as controller") + } + if err = r.Create(context.TODO(), hfs); err != nil { + return errors.Wrap(err, "failure creating hostFirmwareSettings resource") + } + + info.log.Info("created new hostFirmwareSettings resource") + + } else { + // Error reading the object + return errors.Wrap(err, "could not load hostFirmwareSettings resource") + } + } + + return nil +} + // Get the stored firmware settings if there are valid changes func (r *BareMetalHostReconciler) getHostFirmwareSettings(info *reconcileInfo) (hfs *metal3v1alpha1.HostFirmwareSettings, err error) { @@ -1351,9 +1393,9 @@ func (r *BareMetalHostReconciler) getHostFirmwareSettings(info *reconcileInfo) ( } // Check if there are settings in the Spec that are different than the Status - if meta.IsStatusConditionTrue(hfs.Status.Conditions, string(metal3v1alpha1.UpdateRequested)) { + if meta.IsStatusConditionTrue(hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsChangeDetected)) { - if meta.IsStatusConditionTrue(hfs.Status.Conditions, string(metal3v1alpha1.SettingsValid)) { + if meta.IsStatusConditionTrue(hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsValid)) { return hfs, nil } diff --git a/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/hostfirmwaresettings_controller.go b/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/hostfirmwaresettings_controller.go index 566480e7..a3370a48 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/hostfirmwaresettings_controller.go +++ b/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/hostfirmwaresettings_controller.go @@ -21,7 +21,9 @@ import ( "crypto/sha256" "fmt" "sort" + "strconv" "strings" + "time" "github.com/go-logr/logr" "github.com/pkg/errors" @@ -30,21 +32,21 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/predicate" - "sigs.k8s.io/controller-runtime/pkg/source" metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" "github.com/metal3-io/baremetal-operator/pkg/provisioner" ) +const ( + provisionerRetryDelay = time.Second * 30 + resourceNotAvailableRetryDelay = time.Second * 30 + reconcilerRequeueDelay = time.Minute * 5 +) + // HostFirmwareSettingsReconciler reconciles a HostFirmwareSettings object type HostFirmwareSettingsReconciler struct { client.Client @@ -115,24 +117,20 @@ func (r *HostFirmwareSettingsReconciler) Reconcile(ctx context.Context, req ctrl bmh := &metal3v1alpha1.BareMetalHost{} if err = r.Get(context.TODO(), req.NamespacedName, bmh); err != nil { reqLogger.Info("could not get baremetalhost, not running reconciler") - // only run again if created - return ctrl.Result{}, nil + return ctrl.Result{Requeue: true, RequeueAfter: resourceNotAvailableRetryDelay}, nil } // Fetch the HostFirmwareSettings hfs := &metal3v1alpha1.HostFirmwareSettings{} info := &rInfo{log: reqLogger, hfs: hfs, bmh: bmh} if err = r.Get(ctx, req.NamespacedName, hfs); err != nil { + // The HFS resource may have been deleted if k8serrors.IsNotFound(err) { - // A resource doesn't exist, create one - if err = r.newHostFirmwareSettings(info, req.NamespacedName); err != nil { - - return ctrl.Result{}, errors.Wrap(err, "could not create firmware settings") - } - } else { - // Error reading the object - requeue the request. - return ctrl.Result{}, errors.Wrap(err, "could not load host firmware settings") + reqLogger.Info("hostFirmwareSettings not found") + return ctrl.Result{Requeue: true, RequeueAfter: resourceNotAvailableRetryDelay}, nil } + // Error reading the object - requeue the request. + return ctrl.Result{}, errors.Wrap(err, "could not load hostFirmwareSettings") } // Create a provisioner that can access Ironic API @@ -143,13 +141,20 @@ func (r *HostFirmwareSettingsReconciler) Reconcile(ctx context.Context, req ctrl ready, err := prov.IsReady() if err != nil || !ready { - reqLogger.Info("provisioner is not ready", "RequeueAfter:", provisionerNotReadyRetryDelay) - return ctrl.Result{Requeue: true, RequeueAfter: provisionerNotReadyRetryDelay}, nil + reqLogger.Info("provisioner is not ready", "RequeueAfter:", provisionerRetryDelay) + return ctrl.Result{Requeue: true, RequeueAfter: provisionerRetryDelay}, nil + } + + info.log.Info("retrieving firmware settings and saving to resource", "node", bmh.Status.Provisioning.ID) + + // Get the current settings and schema, retry if provisioner returns error + currentSettings, schema, err := prov.GetFirmwareSettings(true) + if err != nil { + reqLogger.Info("provisioner returns error", "RequeueAfter:", provisionerRetryDelay) + return ctrl.Result{Requeue: true, RequeueAfter: provisionerRetryDelay}, nil } - // Get the data from Ironic and update HFS, the call to provisioner may fail if settings can't - // be retrieved for the bios_interface. - if err = r.updateHostFirmwareSettings(prov, info); err != nil { + if err = r.updateHostFirmwareSettings(currentSettings, schema, info); err != nil { return ctrl.Result{}, errors.Wrap(err, "Could not update hostFirmwareSettings") } @@ -157,20 +162,12 @@ func (r *HostFirmwareSettingsReconciler) Reconcile(ctx context.Context, req ctrl r.publishEvent(req, e) } - // only run again on changes - return ctrl.Result{}, nil + // requeue to run again after delay + return ctrl.Result{Requeue: true, RequeueAfter: reconcilerRequeueDelay}, nil } // Get the firmware settings from the provisioner and update hostFirmwareSettings -func (r *HostFirmwareSettingsReconciler) updateHostFirmwareSettings(prov provisioner.Provisioner, info *rInfo) (err error) { - - info.log.Info("retrieving firmware settings and saving to resource", "node", info.bmh.Status.Provisioning.ID) - - // Get the current settings and schema - currentSettings, schema, err := prov.GetFirmwareSettings(true) - if err != nil { - return errors.Wrap(err, "could not get firmware settings from provisioner") - } +func (r *HostFirmwareSettingsReconciler) updateHostFirmwareSettings(currentSettings metal3v1alpha1.SettingsMap, schema map[string]metal3v1alpha1.SettingSchema, info *rInfo) (err error) { // get or create a firmwareSchema to hold schema firmwareSchema, err := r.getOrCreateFirmwareSchema(info, schema) @@ -197,12 +194,13 @@ func (r *HostFirmwareSettingsReconciler) updateStatus(info *rInfo, settings meta info.hfs.Status.Settings = make(metal3v1alpha1.SettingsMap) } - // Update Status + // Update Status on changes for k, v := range settings { // Some vendors include encrypted password fields, don't add these if strings.Contains(k, "Password") { continue } + info.hfs.Status.Settings[k] = v } @@ -214,6 +212,10 @@ func (r *HostFirmwareSettingsReconciler) updateStatus(info *rInfo, settings meta specMismatch = true break } + } else { + // Spec setting is not in Status, this will be handled by validateHostFirmwareSettings + specMismatch = true + break } } @@ -222,26 +224,33 @@ func (r *HostFirmwareSettingsReconciler) updateStatus(info *rInfo, settings meta generation := info.hfs.GetGeneration() if specMismatch { - setCondition(generation, &info.hfs.Status, metal3v1alpha1.UpdateRequested, metav1.ConditionTrue, reason, "") + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsChangeDetected, metav1.ConditionTrue, reason, "") - } else { - setCondition(generation, &info.hfs.Status, metal3v1alpha1.UpdateRequested, metav1.ConditionFalse, reason, "") - } + // Run validation on the Spec to detect invalid values entered by user, including Spec settings not in Status + // Eventually this will be handled by a webhook + errors := r.validateHostFirmwareSettings(info, schema) + if len(errors) == 0 { + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsValid, metav1.ConditionTrue, reason, "") + } else { + for _, error := range errors { + info.publishEvent("ValidationFailed", fmt.Sprintf("Invalid BIOS setting: %v", error)) + } - // Run validation on the Spec to detect invalid values entered by user, including Spec settings not in Status - // Eventually this will be handled by a webhook - errors := r.validateHostFirmwareSettings(info, schema) - if len(errors) == 0 { - setCondition(generation, &info.hfs.Status, metal3v1alpha1.SettingsValid, metav1.ConditionTrue, reason, "") + reason = reasonConfigurationError + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsValid, metav1.ConditionFalse, reason, "Invalid BIOS setting") + } } else { - for _, error := range errors { - info.publishEvent("ValidationFailed", fmt.Sprintf("Invalid BIOS setting: %v", error)) + // Reset conditions + if meta.IsStatusConditionTrue(info.hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsChangeDetected)) { + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsChangeDetected, metav1.ConditionFalse, reason, "") + } + if !meta.IsStatusConditionTrue(info.hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsValid)) { + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsValid, metav1.ConditionTrue, reason, "") } - - reason = reasonConfigurationError - setCondition(generation, &info.hfs.Status, metal3v1alpha1.SettingsValid, metav1.ConditionFalse, reason, "Invalid BIOS setting") } + t := metav1.Now() + info.hfs.Status.LastUpdated = &t return r.Status().Update(context.TODO(), info.hfs) } @@ -259,12 +268,19 @@ func (r *HostFirmwareSettingsReconciler) getOrCreateFirmwareSchema(info *rInfo, info.log.Info("found existing firmwareSchema resource") + // Add hfs as owner so can be garbage collected on delete, if already an owner it will just be overwritten + if err = controllerutil.SetOwnerReference(info.hfs, firmwareSchema, r.Scheme()); err != nil { + return nil, errors.Wrap(err, "could not set owner of existing firmwareSchema") + } + if err = r.Update(context.TODO(), firmwareSchema); err != nil { + return nil, err + } + return firmwareSchema, nil } if !k8serrors.IsNotFound(err) { // Error reading the object return nil, err - } firmwareSchema = &metal3v1alpha1.FirmwareSchema{ @@ -285,15 +301,12 @@ func (r *HostFirmwareSettingsReconciler) getOrCreateFirmwareSchema(info *rInfo, for k, v := range schema { firmwareSchema.Spec.Schema[k] = v } - - if err = r.Create(context.TODO(), firmwareSchema); err != nil { - return nil, err - } - // Set hfs as owner after the create - if controllerutil.SetOwnerReference(info.hfs, firmwareSchema, r.Scheme()); err != nil { + // Set hfs as owner + if err = controllerutil.SetOwnerReference(info.hfs, firmwareSchema, r.Scheme()); err != nil { return nil, errors.Wrap(err, "could not set owner of firmwareSchema") } - if err = r.Update(context.TODO(), firmwareSchema); err != nil { + + if err = r.Create(context.TODO(), firmwareSchema); err != nil { return nil, err } @@ -302,91 +315,11 @@ func (r *HostFirmwareSettingsReconciler) getOrCreateFirmwareSchema(info *rInfo, return firmwareSchema, nil } -// Create a hostFirmwareSettings -func (r *HostFirmwareSettingsReconciler) newHostFirmwareSettings(info *rInfo, namespacedName types.NamespacedName) (err error) { - - info.hfs.ObjectMeta = metav1.ObjectMeta{ - Name: namespacedName.Name, Namespace: namespacedName.Namespace} - info.hfs.Status.Settings = make(metal3v1alpha1.SettingsMap) - info.hfs.Spec.Settings = make(metal3v1alpha1.DesiredSettingsMap) - - if err = r.Create(context.TODO(), info.hfs); err != nil { - return errors.Wrap(err, "failure creating hostFirmwareSettings resource") - } - - // Set bmh as owner, this makes sure the resource is deleted when bmh is deleted - if controllerutil.SetControllerReference(info.bmh, info.hfs, r.Scheme()); err != nil { - return errors.Wrap(err, "could not set bmh as controller") - } - if err = r.Update(context.TODO(), info.hfs); err != nil { - return errors.Wrap(err, "could not update hostfirmwaresettings") - } - - info.log.Info("created new hostFirmwareSettings resource") - - return nil -} - -// EventHandler for updates to both the baremetalhost and hostfirmwarettings -func (r *HostFirmwareSettingsReconciler) updateEventHandler(e event.UpdateEvent) bool { - - r.Log.Info("hostfirmwaresettings in event handler") - - // If this is a baremetalhost, only return true on certain state transitions - var oldState metal3v1alpha1.ProvisioningState = metal3v1alpha1.StateNone - newState := oldState - - oldHost, oldHostExists := e.ObjectOld.(*metal3v1alpha1.BareMetalHost) - if oldHostExists { - oldState = oldHost.Status.Provisioning.State - } - newHost, newHostExists := e.ObjectNew.(*metal3v1alpha1.BareMetalHost) - if newHostExists { - newState = newHost.Status.Provisioning.State - } - - if newHostExists && !oldHostExists { - // The baremetalhost has been created - return true - } - if oldHostExists || newHostExists { - // Data needs to be retrieved from Ironic when node is moved to Ready e.g. after cleaning and when - // provisioned, its not necessary on every state change as data from Ironic will be the same. - if (newState == metal3v1alpha1.StateReady || newState == metal3v1alpha1.StateProvisioned) && oldState != newState { - r.Log.Info("baremetalhost event state update", "oldstate", oldState, "newstate", newState) - return true - } - - return false - } - - _, oldHFS := e.ObjectOld.(*metal3v1alpha1.HostFirmwareSettings) - _, newHFS := e.ObjectNew.(*metal3v1alpha1.HostFirmwareSettings) - if !(oldHFS || newHFS) { - // Don't process if not a hostFirmwareSetting - return false - } - - // This is a hostfirmwaresettings resource, if the update increased the resource generation then process it - // changes to LastUpdated will not increase the resource generation - if e.ObjectNew.GetGeneration() != e.ObjectOld.GetGeneration() { - r.Log.Info("hostfirmwaresettings resource generation changed, updating") - return true - } - - return false -} - // SetupWithManager sets up the controller with the Manager. func (r *HostFirmwareSettingsReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&metal3v1alpha1.HostFirmwareSettings{}). - WithEventFilter( - predicate.Funcs{ - UpdateFunc: r.updateEventHandler, - }). - Watches(&source.Kind{Type: &metal3v1alpha1.BareMetalHost{}}, &handler.EnqueueRequestForObject{}, builder.Predicates{}). Complete(r) } @@ -447,19 +380,37 @@ func setCondition(generation int64, status *metal3v1alpha1.HostFirmwareSettingsS meta.SetStatusCondition(&status.Conditions, newCondition) } -// Generate a name based on the schema keys which should be the same for similar hardware +// Generate a name based on the schema key and values which should be the same for similar hardware func GetSchemaName(schema map[string]metal3v1alpha1.SettingSchema) string { // Schemas from the same vendor and model should be identical for both keys and values. - // Hash the keys of the map to get a identifier unique to this schema. - keys := make([]string, 0, len(schema)) - for k := range schema { - keys = append(keys, k) + hashkeys := make([]string, 0, len(schema)) + for k, v := range schema { + hashkeys = append(hashkeys, k) + hashkeys = append(hashkeys, v.AttributeType) + for _, av := range v.AllowableValues { + hashkeys = append(hashkeys, av) + } + if v.LowerBound != nil { + hashkeys = append(hashkeys, strconv.Itoa(*v.LowerBound)) + } + if v.UpperBound != nil { + hashkeys = append(hashkeys, strconv.Itoa(*v.UpperBound)) + } + if v.MinLength != nil { + hashkeys = append(hashkeys, strconv.Itoa(*v.MinLength)) + } + if v.MaxLength != nil { + hashkeys = append(hashkeys, strconv.Itoa(*v.MaxLength)) + } + if v.ReadOnly != nil { + hashkeys = append(hashkeys, strconv.FormatBool(*v.ReadOnly)) + } } - sort.Strings(keys) + sort.Strings(hashkeys) h := sha256.New() - h.Write([]byte(fmt.Sprintf("%v", keys))) + h.Write([]byte(fmt.Sprintf("%v", hashkeys))) hash := fmt.Sprintf("%x", h.Sum(nil))[:8] return "schema-" + hash diff --git a/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/preprovisioningimage_controller.go b/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/preprovisioningimage_controller.go index 1f9f83e2..5d7b210e 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/preprovisioningimage_controller.go +++ b/vendor/github.com/metal3-io/baremetal-operator/controllers/metal3.io/preprovisioningimage_controller.go @@ -57,6 +57,7 @@ const ( reasonImageSuccess imageConditionReason = "ImageSuccess" reasonImageConfigurationError imageConditionReason = "ConfigurationError" reasonImageMissingNetworkData imageConditionReason = "MissingNetworkData" + reasonImageBuildInvalid imageConditionReason = "ImageBuildInvalid" ) // +kubebuilder:rbac:groups=metal3.io,resources=preprovisioningimages,verbs=get;list;watch;update;patch @@ -177,6 +178,11 @@ func (r *PreprovisioningImageReconciler) update(img *metal3.PreprovisioningImage NetworkDataStatus: secretStatus, }, networkDataContent, log) if err != nil { + failure := imageprovider.ImageBuildInvalid{} + if errors.As(err, &failure) { + log.Info("image build failed", "error", "err") + return setError(generation, &img.Status, reasonImageBuildInvalid, failure.Error()), nil + } return false, err } log.Info("image URL available", "url", url, "format", format) diff --git a/vendor/github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc/irmc.go b/vendor/github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc/irmc.go index c6b12f83..1bfa03f9 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc/irmc.go +++ b/vendor/github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc/irmc.go @@ -31,7 +31,9 @@ func (a *iRMCAccessDetails) Type() string { // NeedsMAC returns true when the host is going to need a separate // port created rather than having it discovered. func (a *iRMCAccessDetails) NeedsMAC() bool { - return false + // For the inspection to work, we need a MAC address + // https://github.com/metal3-io/baremetal-operator/pull/284#discussion_r317579040 + return true } func (a *iRMCAccessDetails) Driver() string { diff --git a/vendor/github.com/metal3-io/baremetal-operator/pkg/imageprovider/default.go b/vendor/github.com/metal3-io/baremetal-operator/pkg/imageprovider/default.go index c6de4f70..37127ff2 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/pkg/imageprovider/default.go +++ b/vendor/github.com/metal3-io/baremetal-operator/pkg/imageprovider/default.go @@ -49,7 +49,7 @@ func (eip envImageProvider) BuildImage(data ImageData, networkData NetworkData, url = eip.initrdURL } if url == "" { - err = fmt.Errorf("Unsupported image format \"%s\"", data.Format) + err = BuildInvalidError(fmt.Errorf("Unsupported image format \"%s\"", data.Format)) } return } diff --git a/vendor/github.com/metal3-io/baremetal-operator/pkg/imageprovider/invalid.go b/vendor/github.com/metal3-io/baremetal-operator/pkg/imageprovider/invalid.go new file mode 100644 index 00000000..c5006564 --- /dev/null +++ b/vendor/github.com/metal3-io/baremetal-operator/pkg/imageprovider/invalid.go @@ -0,0 +1,19 @@ +package imageprovider + +import "fmt" + +type ImageBuildInvalid struct { + err error +} + +func (ibf ImageBuildInvalid) Error() string { + return fmt.Sprintf("Cannot generate image: %s", ibf.err.Error()) +} + +func (ibf ImageBuildInvalid) Unwrap() error { + return ibf.err +} + +func BuildInvalidError(err error) ImageBuildInvalid { + return ImageBuildInvalid{err: err} +} diff --git a/vendor/modules.txt b/vendor/modules.txt index fd335f1c..98ec7505 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -236,7 +236,7 @@ github.com/mattn/go-isatty github.com/matttproud/golang_protobuf_extensions/pbutil # github.com/mbilski/exhaustivestruct v1.0.1 github.com/mbilski/exhaustivestruct/pkg/analyzer -# github.com/metal3-io/baremetal-operator v0.0.0 => github.com/openshift/baremetal-operator v0.0.0-20211202144449-e5fb40679cf0 +# github.com/metal3-io/baremetal-operator v0.0.0 => github.com/openshift/baremetal-operator v0.0.0-20211212164328-3070daca9ae3 ## explicit github.com/metal3-io/baremetal-operator/controllers/metal3.io github.com/metal3-io/baremetal-operator/pkg/hardware @@ -244,10 +244,10 @@ github.com/metal3-io/baremetal-operator/pkg/imageprovider github.com/metal3-io/baremetal-operator/pkg/provisioner github.com/metal3-io/baremetal-operator/pkg/secretutils github.com/metal3-io/baremetal-operator/pkg/utils -# github.com/metal3-io/baremetal-operator/apis v0.0.0 => github.com/openshift/baremetal-operator/apis v0.0.0-20211202144449-e5fb40679cf0 +# github.com/metal3-io/baremetal-operator/apis v0.0.0 => github.com/openshift/baremetal-operator/apis v0.0.0-20211212164328-3070daca9ae3 ## explicit github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1 -# github.com/metal3-io/baremetal-operator/pkg/hardwareutils v0.0.0 => github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211202144449-e5fb40679cf0 +# github.com/metal3-io/baremetal-operator/pkg/hardwareutils v0.0.0 => github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211212164328-3070daca9ae3 github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc # github.com/mitchellh/go-homedir v1.1.0 github.com/mitchellh/go-homedir @@ -869,6 +869,6 @@ sigs.k8s.io/structured-merge-diff/v4/value # sigs.k8s.io/yaml v1.2.0 ## explicit sigs.k8s.io/yaml -# github.com/metal3-io/baremetal-operator => github.com/openshift/baremetal-operator v0.0.0-20211202144449-e5fb40679cf0 -# github.com/metal3-io/baremetal-operator/apis => github.com/openshift/baremetal-operator/apis v0.0.0-20211202144449-e5fb40679cf0 -# github.com/metal3-io/baremetal-operator/pkg/hardwareutils => github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211202144449-e5fb40679cf0 +# github.com/metal3-io/baremetal-operator => github.com/openshift/baremetal-operator v0.0.0-20211212164328-3070daca9ae3 +# github.com/metal3-io/baremetal-operator/apis => github.com/openshift/baremetal-operator/apis v0.0.0-20211212164328-3070daca9ae3 +# github.com/metal3-io/baremetal-operator/pkg/hardwareutils => github.com/openshift/baremetal-operator/pkg/hardwareutils v0.0.0-20211212164328-3070daca9ae3 From 049532ae8f20f62030c22a5f0c1f2e4e5e4e2750 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Thu, 9 Dec 2021 16:58:20 -0500 Subject: [PATCH 2/5] Make Ironic URL optional If the address for Ironic is not available (e.g. if the metal3 Pod is not running) then the CBO will start the controller with an empty IRONIC_BASE_URL. In this case we don't want it to crashloop, but rather start and update the CRDs to indicate that the images are not available. --- cmd/static-server/main.go | 5 ++++- pkg/env/env.go | 2 +- pkg/ignition/builder.go | 17 +++++++++-------- pkg/ignition/builder_test.go | 5 ++++- pkg/imageprovider/rhcos.go | 9 +++++++-- 5 files changed, 25 insertions(+), 13 deletions(-) diff --git a/cmd/static-server/main.go b/cmd/static-server/main.go index a241797d..f3fc9279 100644 --- a/cmd/static-server/main.go +++ b/cmd/static-server/main.go @@ -58,12 +58,15 @@ func loadStaticNMState(env *env.EnvInputs, nmstateDir string, imageServer imageh if err != nil { return errors.WithMessagef(err, "problem reading %s", path.Join(nmstateDir, f.Name())) } - igBuilder := ignition.New(b, registries, + igBuilder, err := ignition.New(b, registries, env.IronicBaseURL, env.IronicAgentImage, env.IronicAgentPullSecret, env.IronicRAMDiskSSHKey, ) + if err != nil { + return errors.WithMessage(err, "failed to configure ignition") + } ign, err := igBuilder.Generate() if err != nil { return errors.WithMessagef(err, "problem generating ignition %s", f.Name()) diff --git a/pkg/env/env.go b/pkg/env/env.go index 2812179d..c5cdc4bc 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -10,7 +10,7 @@ import ( type EnvInputs struct { DeployISO string `envconfig:"DEPLOY_ISO" required:"true"` DeployInitrd string `envconfig:"DEPLOY_INITRD" required:"true"` - IronicBaseURL string `envconfig:"IRONIC_BASE_URL" required:"true"` + IronicBaseURL string `envconfig:"IRONIC_BASE_URL"` IronicAgentImage string `envconfig:"IRONIC_AGENT_IMAGE" required:"true"` IronicAgentPullSecret string `envconfig:"IRONIC_AGENT_PULL_SECRET"` IronicRAMDiskSSHKey string `envconfig:"IRONIC_RAMDISK_SSH_KEY"` diff --git a/pkg/ignition/builder.go b/pkg/ignition/builder.go index 57d61c07..4e344d08 100644 --- a/pkg/ignition/builder.go +++ b/pkg/ignition/builder.go @@ -27,7 +27,14 @@ type ignitionBuilder struct { ironicRAMDiskSSHKey string } -func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ironicAgentPullSecret, ironicRAMDiskSSHKey string) *ignitionBuilder { +func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ironicAgentPullSecret, ironicRAMDiskSSHKey string) (*ignitionBuilder, error) { + if ironicBaseURL == "" { + return nil, errors.New("ironicBaseURL is required") + } + if ironicAgentImage == "" { + return nil, errors.New("ironicAgentImage is required") + } + return &ignitionBuilder{ nmStateData: nmStateData, registriesConf: registriesConf, @@ -35,16 +42,10 @@ func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ir ironicAgentImage: ironicAgentImage, ironicAgentPullSecret: ironicAgentPullSecret, ironicRAMDiskSSHKey: ironicRAMDiskSSHKey, - } + }, nil } func (b *ignitionBuilder) Generate() ([]byte, error) { - if b.ironicAgentImage == "" { - return nil, errors.New("ironicAgentImage is required") - } - if b.ironicBaseURL == "" { - return nil, errors.New("ironicBaseURL is required") - } config := ignition_config_types_32.Config{ Ignition: ignition_config_types_32.Ignition{ Version: "3.2.0", diff --git a/pkg/ignition/builder_test.go b/pkg/ignition/builder_test.go index 096206eb..03f02a12 100644 --- a/pkg/ignition/builder_test.go +++ b/pkg/ignition/builder_test.go @@ -15,10 +15,13 @@ func TestGenerateRegistries(t *testing.T) { [[registry.mirror]] location = "virthost.ostest.test.metalkube.org:5000/localimages/local-release-image" ` - builder := New([]byte{}, []byte(registries), + builder, err := New([]byte{}, []byte(registries), "http://ironic.example.com", "quay.io/openshift-release-dev/ironic-ipa-image", "", "") + if err != nil { + t.Fatalf("Unexpected error %v", err) + } ignition, err := builder.Generate() if err != nil { diff --git a/pkg/imageprovider/rhcos.go b/pkg/imageprovider/rhcos.go index 68b3768d..01eaac36 100644 --- a/pkg/imageprovider/rhcos.go +++ b/pkg/imageprovider/rhcos.go @@ -47,12 +47,17 @@ func (ip *rhcosImageProvider) SupportsFormat(format metal3.ImageFormat) bool { func (ip *rhcosImageProvider) buildIgnitionConfig(networkData imageprovider.NetworkData) ([]byte, error) { nmstateData := networkData["nmstate"] - return ignition.New(nmstateData, ip.RegistriesConf, + builder, err := ignition.New(nmstateData, ip.RegistriesConf, ip.EnvInputs.IronicBaseURL, ip.EnvInputs.IronicAgentImage, ip.EnvInputs.IronicAgentPullSecret, ip.EnvInputs.IronicRAMDiskSSHKey, - ).Generate() + ) + if err != nil { + return nil, imageprovider.BuildInvalidError(err) + } + + return builder.Generate() } func imageKey(data imageprovider.ImageData) string { From 6acf73077cb709bd7bb9d36f7f7f90c6bab1cd2c Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Thu, 9 Dec 2021 16:30:14 -0500 Subject: [PATCH 3/5] Avoid extra parsing of baseURL Enforce the early checking for a valid publish address base URL by requiring a URL object as the input to NewImageHandler(). --- cmd/controller/main.go | 4 ++-- cmd/static-server/main.go | 4 ++-- pkg/imagehandler/imagehandler.go | 10 +++++----- pkg/imagehandler/imagehandler_test.go | 17 ++++++++++++++--- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 3c337d84..e70435bd 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -127,13 +127,13 @@ func main() { os.Exit(1) } - _, err = url.Parse(imagesPublishAddr) + publishURL, err := url.Parse(imagesPublishAddr) if err != nil { setupLog.Error(err, "imagesPublishAddr is not parsable") os.Exit(1) } - imageServer := imagehandler.NewImageHandler(ctrl.Log.WithName("ImageHandler"), envInputs.DeployISO, envInputs.DeployInitrd, imagesPublishAddr) + imageServer := imagehandler.NewImageHandler(ctrl.Log.WithName("ImageHandler"), envInputs.DeployISO, envInputs.DeployInitrd, publishURL) http.Handle("/", http.FileServer(imageServer.FileSystem())) go func() { diff --git a/cmd/static-server/main.go b/cmd/static-server/main.go index f3fc9279..462beb35 100644 --- a/cmd/static-server/main.go +++ b/cmd/static-server/main.go @@ -110,7 +110,7 @@ func main() { os.Exit(1) } - _, err = url.Parse(imagesPublishAddr) + publishURL, err := url.Parse(imagesPublishAddr) if err != nil { log.Error(err, "imagesPublishAddr is not parsable") os.Exit(1) @@ -121,7 +121,7 @@ func main() { os.Exit(1) } - imageServer := imagehandler.NewImageHandler(ctrl.Log.WithName("ImageHandler"), env.DeployISO, env.DeployInitrd, imagesPublishAddr) + imageServer := imagehandler.NewImageHandler(ctrl.Log.WithName("ImageHandler"), env.DeployISO, env.DeployInitrd, publishURL) http.Handle("/", http.FileServer(imageServer.FileSystem())) if err := loadStaticNMState(env, nmstateDir, imageServer); err != nil { diff --git a/pkg/imagehandler/imagehandler.go b/pkg/imagehandler/imagehandler.go index 0d7eae08..91f35eaa 100644 --- a/pkg/imagehandler/imagehandler.go +++ b/pkg/imagehandler/imagehandler.go @@ -14,6 +14,7 @@ limitations under the License. package imagehandler import ( + "fmt" "net/http" "net/url" "sync" @@ -27,7 +28,7 @@ import ( type imageFileSystem struct { isoFile *baseIso initramfsFile *baseInitramfs - baseURL string + baseURL *url.URL keys map[string]string images map[string]*imageFile mu *sync.Mutex @@ -43,7 +44,7 @@ type ImageHandler interface { RemoveImage(key string) } -func NewImageHandler(logger logr.Logger, isoFile, initramfsFile, baseURL string) ImageHandler { +func NewImageHandler(logger logr.Logger, isoFile, initramfsFile string, baseURL *url.URL) ImageHandler { return &imageFileSystem{ log: logger, isoFile: newBaseIso(isoFile), @@ -105,12 +106,11 @@ func (f *imageFileSystem) ServeImage(key string, ignitionContent []byte, initram } } - u, err := url.Parse(f.baseURL) + p, err := url.Parse(fmt.Sprintf("/%s", name)) if err != nil { return "", err } - u.Path = name - return u.String(), nil + return f.baseURL.ResolveReference(p).String(), nil } func (f *imageFileSystem) imageFileByName(name string) *imageFile { diff --git a/pkg/imagehandler/imagehandler_test.go b/pkg/imagehandler/imagehandler_test.go index 3432e77f..a345a1d7 100644 --- a/pkg/imagehandler/imagehandler_test.go +++ b/pkg/imagehandler/imagehandler_test.go @@ -17,6 +17,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "strings" "sync" "testing" @@ -42,11 +43,13 @@ func TestImageHandler(t *testing.T) { t.Fatal(err) } + baseURL, _ := url.Parse("http://localhost:8080") + rr := httptest.NewRecorder() imageServer := &imageFileSystem{ log: zap.New(zap.UseDevMode(true)), isoFile: &baseIso{baseFileData{filename: "dummyfile.iso", size: 12345}}, - baseURL: "http://localhost:8080", + baseURL: baseURL, keys: map[string]string{ "host-xyz-45-uuid": "host-xyz-45.iso", }, @@ -79,10 +82,14 @@ func TestImageHandler(t *testing.T) { } func TestNewImageHandler(t *testing.T) { + baseUrl, err := url.Parse("http://base.test:1234") + if err != nil { + t.Fatalf("unexpected error %v", err) + } handler := NewImageHandler(zap.New(zap.UseDevMode(true)), "dummyfile.iso", "dummyfile.initramfs", - "http://base.test:1234") + baseUrl) ifs := handler.(*imageFileSystem) ifs.isoFile.size = 12345 @@ -122,10 +129,14 @@ func TestNewImageHandler(t *testing.T) { } func TestNewImageHandlerStatic(t *testing.T) { + baseUrl, err := url.Parse("http://base.test:1234") + if err != nil { + t.Fatalf("unexpected error %v", err) + } handler := NewImageHandler(zap.New(zap.UseDevMode(true)), "dummyfile.iso", "dummyfile.initramfs", - "http://base.test:1234") + baseUrl) ifs := handler.(*imageFileSystem) ifs.isoFile.size = 12345 From 0cbc3a05ebd4b0921c13f54e46538f793f4c5544 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Thu, 9 Dec 2021 17:44:13 -0500 Subject: [PATCH 4/5] Report errors from nmstatectl If the network data is in an invalid format, report the error from nmstatectl and flag an error in the image CR. --- cmd/static-server/main.go | 3 +++ pkg/ignition/builder.go | 28 +++++++++++++++++++--------- pkg/imageprovider/rhcos.go | 9 +++++++++ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/cmd/static-server/main.go b/cmd/static-server/main.go index 462beb35..f102c9e0 100644 --- a/cmd/static-server/main.go +++ b/cmd/static-server/main.go @@ -67,6 +67,9 @@ func loadStaticNMState(env *env.EnvInputs, nmstateDir string, imageServer imageh if err != nil { return errors.WithMessage(err, "failed to configure ignition") } + if err, _ := igBuilder.ProcessNetworkState(); err != nil { + return errors.WithMessage(err, "failed to convert nmstate data") + } ign, err := igBuilder.Generate() if err != nil { return errors.WithMessagef(err, "problem generating ignition %s", f.Name()) diff --git a/pkg/ignition/builder.go b/pkg/ignition/builder.go index 4e344d08..6e6b5c52 100644 --- a/pkg/ignition/builder.go +++ b/pkg/ignition/builder.go @@ -25,6 +25,7 @@ type ignitionBuilder struct { ironicAgentImage string ironicAgentPullSecret string ironicRAMDiskSSHKey string + networkKeyFiles []byte } func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ironicAgentPullSecret, ironicRAMDiskSSHKey string) (*ignitionBuilder, error) { @@ -45,6 +46,22 @@ func New(nmStateData, registriesConf []byte, ironicBaseURL, ironicAgentImage, ir }, nil } +func (b *ignitionBuilder) ProcessNetworkState() (error, string) { + if len(b.nmStateData) > 0 { + nmstatectl := exec.Command("nmstatectl", "gc", "-") + nmstatectl.Stdin = strings.NewReader(string(b.nmStateData)) + out, err := nmstatectl.Output() + if err != nil { + if ee, ok := err.(*exec.ExitError); ok { + return err, string(ee.Stderr) + } + return err, "" + } + b.networkKeyFiles = out + } + return nil, "" +} + func (b *ignitionBuilder) Generate() ([]byte, error) { config := ignition_config_types_32.Config{ Ignition: ignition_config_types_32.Ignition{ @@ -87,15 +104,8 @@ func (b *ignitionBuilder) Generate() ([]byte, error) { config.Storage.Files = append(config.Storage.Files, registriesFile) } - if len(b.nmStateData) > 0 { - nmstatectl := exec.Command("nmstatectl", "gc", "-") - nmstatectl.Stdin = strings.NewReader(string(b.nmStateData)) - out, err := nmstatectl.Output() - if err != nil { - return nil, err - } - - files, err := nmstateOutputToFiles(out) + if len(b.networkKeyFiles) > 0 { + files, err := nmstateOutputToFiles(b.networkKeyFiles) if err != nil { return nil, err } diff --git a/pkg/imageprovider/rhcos.go b/pkg/imageprovider/rhcos.go index 01eaac36..0cc701e1 100644 --- a/pkg/imageprovider/rhcos.go +++ b/pkg/imageprovider/rhcos.go @@ -1,6 +1,7 @@ package imageprovider import ( + "errors" "fmt" "github.com/go-logr/logr" @@ -57,6 +58,14 @@ func (ip *rhcosImageProvider) buildIgnitionConfig(networkData imageprovider.Netw return nil, imageprovider.BuildInvalidError(err) } + err, message := builder.ProcessNetworkState() + if message != "" { + return nil, imageprovider.BuildInvalidError(errors.New(message)) + } + if err != nil { + return nil, err + } + return builder.Generate() } From 2add8b1e6df4b8cf209293de0cacf0ff58f2ba33 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Thu, 9 Dec 2021 19:16:22 -0500 Subject: [PATCH 5/5] Record failures to read base image If the base image cannot be read, this is not a transient error but a reason to mark the image as in error. --- pkg/imagehandler/imagehandler.go | 22 +++++++++++++++++----- pkg/imageprovider/rhcos.go | 6 +++++- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/pkg/imagehandler/imagehandler.go b/pkg/imagehandler/imagehandler.go index 91f35eaa..1ff32714 100644 --- a/pkg/imagehandler/imagehandler.go +++ b/pkg/imagehandler/imagehandler.go @@ -23,6 +23,18 @@ import ( "github.com/google/uuid" ) +type InvalidBaseImageError struct { + cause error +} + +func (ie InvalidBaseImageError) Error() string { + return "Base Image not available" +} + +func (ie InvalidBaseImageError) Unwrap() error { + return ie.cause +} + // imageFileSystem is an http.FileSystem that creates a virtual filesystem of // host images. type imageFileSystem struct { @@ -82,7 +94,7 @@ func (f *imageFileSystem) getNameForKey(key string) (name string, err error) { func (f *imageFileSystem) ServeImage(key string, ignitionContent []byte, initramfs, static bool) (string, error) { size, err := f.getBaseImage(initramfs).Size() if err != nil { - return "", err + return "", InvalidBaseImageError{cause: err} } f.mu.Lock() @@ -95,6 +107,10 @@ func (f *imageFileSystem) ServeImage(key string, ignitionContent []byte, initram return "", err } } + p, err := url.Parse(fmt.Sprintf("/%s", name)) + if err != nil { + return "", err + } if _, exists := f.images[key]; !exists { f.keys[name] = key @@ -106,10 +122,6 @@ func (f *imageFileSystem) ServeImage(key string, ignitionContent []byte, initram } } - p, err := url.Parse(fmt.Sprintf("/%s", name)) - if err != nil { - return "", err - } return f.baseURL.ResolveReference(p).String(), nil } diff --git a/pkg/imageprovider/rhcos.go b/pkg/imageprovider/rhcos.go index 0cc701e1..11a0e2da 100644 --- a/pkg/imageprovider/rhcos.go +++ b/pkg/imageprovider/rhcos.go @@ -85,8 +85,12 @@ func (ip *rhcosImageProvider) BuildImage(data imageprovider.ImageData, networkDa return "", err } - return ip.ImageHandler.ServeImage(imageKey(data), ignitionConfig, + url, err := ip.ImageHandler.ServeImage(imageKey(data), ignitionConfig, data.Format == metal3.ImageFormatInitRD, false) + if errors.As(err, &imagehandler.InvalidBaseImageError{}) { + return "", imageprovider.BuildInvalidError(err) + } + return url, err } func (ip *rhcosImageProvider) DiscardImage(data imageprovider.ImageData) error {