diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 761dcec40c..219fd3eef2 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -26,6 +26,7 @@ import ( mcfgclientv1 "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/typed/machineconfiguration.openshift.io/v1" "github.com/pkg/errors" "github.com/vincent-petithory/dataurl" + imgref "github.com/containers/image/docker/reference" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" @@ -580,8 +581,13 @@ func (dn *Daemon) CheckStateOnBoot() error { } if state.bootstrapping { - if !dn.checkOS(state.currentConfig.Spec.OSImageURL) { - glog.Info("Bootstrap pivot required") + targetOSImageURL := state.currentConfig.Spec.OSImageURL + osMatch, err := dn.checkOS(targetOSImageURL) + if err != nil { + return err + } + if !osMatch { + glog.Infof("Bootstrap pivot required to: %s", targetOSImageURL) // This only returns on error return dn.updateOSAndReboot(state.currentConfig) } @@ -825,7 +831,12 @@ func (dn *Daemon) triggerUpdateWithMachineConfig(currentConfig *mcfgv1.MachineCo // degraded. func (dn *Daemon) validateOnDiskState(currentConfig *mcfgv1.MachineConfig) bool { // Be sure we're booted into the OS we expect - if !dn.checkOS(currentConfig.Spec.OSImageURL) { + osMatch, err := dn.checkOS(currentConfig.Spec.OSImageURL) + if err != nil { + glog.Errorf("%s", err); + return false + } + if !osMatch { glog.Errorf("Expected target osImageURL %s", currentConfig.Spec.OSImageURL) return false } @@ -839,32 +850,69 @@ func (dn *Daemon) validateOnDiskState(currentConfig *mcfgv1.MachineConfig) bool return true } -// isUnspecifiedOS says whether an osImageURL is "unspecified", -// i.e. we should not try to change the current state. -func (dn *Daemon) isUnspecifiedOS(osImageURL string) bool { + +// getRefDigest parses a Docker/OCI image reference and returns +// its digest, or an error if the string fails to parse as +// a "canonical" image reference with a digest. +func getRefDigest(ref string) (string, error) { + refParsed, err := imgref.ParseNamed(ref) + if err != nil { + return "", err + } + canon, ok := refParsed.(imgref.Canonical) + if !ok { + return "", fmt.Errorf("Not canonical form: %s", ref) + } + + return canon.Digest().String(), nil +} + +// compareOSImageURL is the backend for checkOS. +func compareOSImageURL(current, desired string) (bool, error) { + // Since https://github.com/openshift/machine-config-operator/pull/426 landed + // we don't use the "unspecified" osImageURL anymore, but let's keep supporting + // it for now. // The ://dummy syntax is legacy - return osImageURL == "" || osImageURL == "://dummy" + if desired == "" || desired == "://dummy" { + glog.Info(`No target osImageURL provided`) + return true, nil + } + + if current == desired { + return true, nil + } + + bootedDigest, err := getRefDigest(current) + if err != nil { + return false, errors.Wrap(err, "parsing booted osImageURL") + } + desiredDigest, err := getRefDigest(desired) + if err != nil { + return false, errors.Wrap(err, "parsing desired osImageURL") + } + + if bootedDigest == desiredDigest { + glog.Infof("Current and target osImageURL have matching digest %s", bootedDigest) + return true, nil + } + + return false, nil } // checkOS determines whether the booted system matches the target // osImageURL and if not whether we need to take action. This function // returns `true` if no action is required, which is the case if we're -// not running RHCOS, or if the target osImageURL is "" (unspecified). +// not running RHCOS, or if the target osImageURL is "" (unspecified), +// or if the digests match. // Otherwise if `false` is returned, then we need to perform an update. -func (dn *Daemon) checkOS(osImageURL string) bool { +func (dn *Daemon) checkOS(osImageURL string) (bool, error) { // Nothing to do if we're not on RHCOS if dn.OperatingSystem != machineConfigDaemonOSRHCOS { glog.Infof(`Not booted into Red Hat CoreOS, ignoring target OSImageURL %s`, osImageURL) - return true + return true, nil } - // XXX: The installer doesn't pivot yet so for now, just make "" - // mean "unset, don't pivot". See also: https://github.com/openshift/installer/issues/281 - if dn.isUnspecifiedOS(osImageURL) { - glog.Info(`No target osImageURL provided`) - return true - } - return dn.bootedOSImageURL == osImageURL + return compareOSImageURL(dn.bootedOSImageURL, osImageURL) } // checkUnits validates the contents of all the units in the diff --git a/pkg/daemon/daemon_test.go b/pkg/daemon/daemon_test.go index cec3d2e8e2..24fc4092aa 100644 --- a/pkg/daemon/daemon_test.go +++ b/pkg/daemon/daemon_test.go @@ -86,3 +86,25 @@ func TestOverwrittenFile(t *testing.T) { t.Errorf("Validating an overwritten file failed") } } + +func TestCompareOSImageURL(t *testing.T) { + refA := "registry.example.com/foo/bar@sha256:0743a3cc3bcf3b4aabb814500c2739f84cb085ff4e7ec7996aef7977c4c19c7f" + refB := "registry.example.com/foo/baz@sha256:0743a3cc3bcf3b4aabb814500c2739f84cb085ff4e7ec7996aef7977c4c19c7f" + refC := "registry.example.com/foo/bar@sha256:2a76681fd15bfc06fa4aa0ff6913ba17527e075417fc92ea29f6bcc2afca24ff" + m, err := compareOSImageURL(refA, refA) + if !m { + t.Fatalf("Expected refA ident") + } + m, err = compareOSImageURL(refA, refB) + if !m { + t.Fatalf("Expected refA = refB") + } + m, err = compareOSImageURL(refA, refC) + if m { + t.Fatalf("Expected refA != refC") + } + m, err = compareOSImageURL(refA, "registry.example.com/foo/bar") + if m || err == nil { + t.Fatalf("Expected err") + } +} diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index 3f42e9e047..3a2864713c 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -636,19 +636,15 @@ func (dn *Daemon) updateOS(config *mcfgv1.MachineConfig) error { } newURL := config.Spec.OSImageURL - - // see similar logic in checkOS() - if dn.isUnspecifiedOS(newURL) { - glog.Info("No target osImageURL provided") - return nil + osMatch, err := compareOSImageURL(dn.bootedOSImageURL, newURL) + if err != nil { + return err } - - if newURL == dn.bootedOSImageURL { + if osMatch { return nil } glog.Infof("Updating OS to %s", newURL) - if err := dn.NodeUpdaterClient.RunPivot(newURL); err != nil { return fmt.Errorf("Failed to run pivot: %v", err) }