Skip to content

Commit

Permalink
Make verification pass if commit hashes match
Browse files Browse the repository at this point in the history
rpm-ostree deploy-from-self does get us to the right image, but the MCO
doesn't know that because the OSImageURL/Container Name/Custom
Deployment name doesn't get set when it rebases, it just uses the
commit hash.

This tells the MCO to:
- inspect the current image specified by OSImageURL,
- grab its commit hash from the labels
- compare it to the base commit of what we have on-disk.
If they match, we know we are on the correct image, even if we
did not get there the usual way.
  • Loading branch information
jkyros committed Sep 27, 2022
1 parent b63b319 commit 9c6b021
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 19 deletions.
25 changes: 22 additions & 3 deletions pkg/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ type Daemon struct {
// bootedOSImageURL is the currently booted URL of the operating system
bootedOSImageURL string

// bootedOScommit is the commit hash of the currently booted operating system
bootedOSCommit string

// kubeClient allows interaction with Kubernetes, including the node we are running on.
kubeClient kubernetes.Interface

Expand Down Expand Up @@ -213,6 +216,7 @@ func New(
var (
osImageURL string
osVersion string
osCommit string
err error
)

Expand All @@ -231,11 +235,11 @@ func New(
if err != nil {
return nil, fmt.Errorf("error initializing rpm-ostree: %w", err)
}
osImageURL, osVersion, err = nodeUpdaterClient.GetBootedOSImageURL()
osImageURL, osVersion, osCommit, err = nodeUpdaterClient.GetBootedOSImageURL()
if err != nil {
return nil, fmt.Errorf("error reading osImageURL from rpm-ostree: %w", err)
}
glog.Infof("Booted osImageURL: %s (%s)", osImageURL, osVersion)
glog.Infof("Booted osImageURL: %s (%s) %s", osImageURL, osVersion, osCommit)
}

bootID := ""
Expand Down Expand Up @@ -267,6 +271,7 @@ func New(
os: hostos,
NodeUpdaterClient: nodeUpdaterClient,
bootedOSImageURL: osImageURL,
bootedOSCommit: osCommit,
bootID: bootID,
exitCh: exitCh,
currentConfigPath: currentConfigPath,
Expand Down Expand Up @@ -1872,7 +1877,7 @@ func (dn *Daemon) validateOnDiskState(currentConfig *mcfgv1.MachineConfig) error
// Be sure we're booted into the OS we expect
osMatch := dn.checkOS(currentConfig.Spec.OSImageURL)
if !osMatch {
return fmt.Errorf("expected target osImageURL %q, have %q", currentConfig.Spec.OSImageURL, dn.bootedOSImageURL)
return fmt.Errorf("expected target osImageURL %q, have %q (%q)", currentConfig.Spec.OSImageURL, dn.bootedOSImageURL, dn.bootedOSCommit)
}

if dn.os.IsCoreOSVariant() {
Expand All @@ -1898,6 +1903,20 @@ func (dn *Daemon) checkOS(osImageURL string) bool {
return true
}

// TODO(jkyros): the header for this functions says "if the digests match"
// so I'm wondering if at one point this used to work this way....
// TODO(jkyros): and pulling it just to check is so expensive, skopeo works now
// if you use the --no-tags argument (doesn't pull tags) so we should probably just do that
inspection, err := imageInspect(osImageURL)
if err != nil {
glog.Warningf("Unable to check manifest for matching hash: %s", err)
} else if ostreeCommit, ok := inspection.Labels["ostree.commit"]; ok {
if ostreeCommit == dn.bootedOSCommit {
glog.Infof("We are technically in the right image even if the URL doesn't match (%s == %s)", ostreeCommit, osImageURL)
return true
}
}

return dn.bootedOSImageURL == osImageURL
}

Expand Down
24 changes: 19 additions & 5 deletions pkg/daemon/rpm-ostree.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type RpmOstreeDeployment struct {
OSName string `json:"osname"`
Serial int32 `json:"serial"`
Checksum string `json:"checksum"`
BaseChecksum string `json:"base-checksum,omitempty"`
Version string `json:"version"`
Timestamp uint64 `json:"timestamp"`
Booted bool `json:"booted"`
Expand Down Expand Up @@ -68,7 +69,7 @@ type imageInspection struct {
type NodeUpdaterClient interface {
Initialize() error
GetStatus() (string, error)
GetBootedOSImageURL() (string, string, error)
GetBootedOSImageURL() (string, string, string, error)
Rebase(string, string) (bool, error)
RebaseLayered(string) error
IsBootableImage(string) (bool, error)
Expand Down Expand Up @@ -169,16 +170,21 @@ func (r *RpmOstreeClient) GetStatus() (string, error) {
return string(output), nil
}

// GetBootedOSImageURL returns the image URL as well as the OSTree version (for logging)
// GetBootedOSImageURL returns the image URL as well as the OSTree version(for logging) and the ostree commit (for comparisons)
// Returns the empty string if the host doesn't have a custom origin that matches pivot://
// (This could be the case for e.g. FCOS, or a future RHCOS which comes not-pivoted by default)
func (r *RpmOstreeClient) GetBootedOSImageURL() (string, string, error) {
func (r *RpmOstreeClient) GetBootedOSImageURL() (string, string, string, error) {
bootedDeployment, _, err := r.GetBootedAndStagedDeployment()
if err != nil {
return "", "", err
return "", "", "", err
}

// TODO(jkyros): take this out, I just want to see when/why it's empty?
j, _ := json.MarshalIndent(bootedDeployment, "", " ")
glog.Infof("%s", j)

// the canonical image URL is stored in the custom origin field.

osImageURL := ""
if len(bootedDeployment.CustomOrigin) > 0 {
if strings.HasPrefix(bootedDeployment.CustomOrigin[0], "pivot://") {
Expand All @@ -194,7 +200,15 @@ func (r *RpmOstreeClient) GetBootedOSImageURL() (string, string, error) {
osImageURL = tokens[1]
}
}
return osImageURL, bootedDeployment.Version, nil

// BaseChecksum is populated if the system has been modified in a way that changes the base checksum
// (like via an RPM install) so prefer BaseCheksum that if it's present, otherwise just use Checksum.
baseChecksum := bootedDeployment.Checksum
if bootedDeployment.BaseChecksum != "" {
baseChecksum = bootedDeployment.BaseChecksum
}

return osImageURL, bootedDeployment.Version, baseChecksum, nil
}

func podmanInspect(imgURL string) (imgdata *imageInspection, err error) {
Expand Down
16 changes: 5 additions & 11 deletions pkg/daemon/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -1922,17 +1922,11 @@ func (dn *Daemon) updateLayeredOS(config *mcfgv1.MachineConfig) error {
if err != nil {
return err
}
dn.needSecondaryReboot = true
// We'll need to do a second reconciliation pass i.e. an extra reboot today, but that won't
// be necessary after we ship the newer rpm-ostree into older releases.
// See also https://github.com/coreos/rpm-ostree/issues/4018
if err := os.WriteFile(constants.MachineConfigDaemonPersistentForceOnceFile, []byte(""), 0o644); err != nil {
return err
}
} else {
if err := dn.NodeUpdaterClient.RebaseLayered(newURL); err != nil {
return fmt.Errorf("failed to update OS to %s : %w", newURL, err)
}

// TODO(jkyros): we don't need to do anything special here if I teach rpm-ostree this is the same container

} else if err := dn.NodeUpdaterClient.RebaseLayered(newURL); err != nil {
return fmt.Errorf("failed to update OS to %s : %w", newURL, err)
}

return nil
Expand Down

0 comments on commit 9c6b021

Please sign in to comment.