release-controller-api: render RPM diff

cmd/release-controller-api/http.go

@@ -726,7 +726,7 @@ func (c *Controller) changeLogWorker(result *renderResult, tagInfo *releaseTagIn
 	ch := make(chan renderResult)
 
 	// run the changelog in a goroutine because it may take significant time
-	go c.getChangeLog(ch, tagInfo.PreviousTagPullSpec, tagInfo.Info.Previous.Name, tagInfo.TagPullSpec, tagInfo.Info.Tag.Name, format)
+	go c.getChangeLog(ch, nil, tagInfo.PreviousTagPullSpec, tagInfo.Info.Previous.Name, tagInfo.TagPullSpec, tagInfo.Info.Tag.Name, format)
 
 	select {
 	case *result = <-ch:

cmd/release-controller-api/http_changelog.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/http"
 	"regexp"
+	"strings"
 	"time"
 
 	"github.com/openshift/release-controller/pkg/rhcos"
@@ -23,7 +24,7 @@ type renderResult struct {
 	err error
 }
 
-func (c *Controller) getChangeLog(ch chan renderResult, fromPull string, fromTag string, toPull string, toTag string, format string) {
+func (c *Controller) getChangeLog(ch chan renderResult, chRpmDiff chan renderResult, fromPull string, fromTag string, toPull string, toTag string, format string) {
 	fromImage, err := releasecontroller.GetImageInfo(c.releaseInfo, c.architecture, fromPull)
 	if err != nil {
 		ch <- renderResult{err: err}
@@ -72,12 +73,31 @@ func (c *Controller) getChangeLog(ch chan renderResult, fromPull string, fromTag
 		ch <- renderResult{out: out}
 	}
 
-	out, err = rhcos.TransformMarkDownOutput(out, fromPull, fromTag, toPull, toTag, architecture, archExtension)
+	out, err = rhcos.TransformMarkDownOutput(out, fromTag, toTag, architecture, archExtension)
 	if err != nil {
 		ch <- renderResult{err: err}
 		return
 	}
 	ch <- renderResult{out: out}
+
+	// We returned a result already, so we're no longer blocking rendering. So now also fetch the CoreOS RPM diff if requested.
+	if chRpmDiff == nil {
+		return
+	}
+
+	// Only request a CoreOS diff if it'll be rendered. Use the exact
+	// check that renderChangelog does to know if to consume from us.
+	if !strings.Contains(out, "#coreos-package-diff") {
+		chRpmDiff <- renderResult{}
+		return
+	}
+
+	rpmdiff, err := c.releaseInfo.RpmDiff(fromImage.GenerateDigestPullSpec(), toImage.GenerateDigestPullSpec())
+	if err != nil {
+		chRpmDiff <- renderResult{err: err}
+	}
+
+	chRpmDiff <- renderResult{out: rhcos.RenderRpmDiff(out, rpmdiff)}
 }
 
 func (c *Controller) renderChangeLog(w http.ResponseWriter, fromPull string, fromTag string, toPull string, toTag string, format string) {
@@ -89,9 +109,10 @@ func (c *Controller) renderChangeLog(w http.ResponseWriter, fromPull string, fro
 	flusher.Flush()
 
 	ch := make(chan renderResult)
+	chRpmDiff := make(chan renderResult)
 
 	// run the changelog in a goroutine because it may take significant time
-	go c.getChangeLog(ch, fromPull, fromTag, toPull, toTag, format)
+	go c.getChangeLog(ch, chRpmDiff, fromPull, fromTag, toPull, toTag, format)
 
 	var render renderResult
 	select {
@@ -141,4 +162,27 @@ func (c *Controller) renderChangeLog(w http.ResponseWriter, fromPull string, fro
 		// if we don't get a valid result within limits, just show the simpler informational view
 		fmt.Fprintf(w, `<p class="alert alert-danger">%s</p>`, fmt.Sprintf("Unable to show full changelog: %s", render.err))
 	}
+
+	// only render a CoreOS diff if we need to; we can know this by
+	// checking if it links to the diff section we create here
+	if !strings.Contains(render.out, "#coreos-package-diff") {
+		return
+	}
+
+	fmt.Fprintf(w, "<h3 id=\"coreos-package-diff\">CoreOS Package Diff</h3>")
+
+	// only render the RPM diff if it's cached; judged by it taking less than 500ms
+	select {
+	case <-time.After(500 * time.Millisecond):
+		fmt.Fprintf(w, `<p class="alert alert-danger">Package diff still loading; check again later...</p>`)
+	case render = <-chRpmDiff:
+		if render.err != nil {
+			fmt.Fprintf(w, `<p class="alert alert-danger">%s</p>`, fmt.Sprintf("Unable to show package diff: %s", render.err))
+		} else {
+			result := blackfriday.Run([]byte(render.out))
+			if _, err := w.Write(result); err != nil {
+				http.Error(w, err.Error(), http.StatusInternalServerError)
+			}
+		}
+	}
 }

pkg/release-controller/release_info.go

@@ -70,6 +70,22 @@ func NewCachingReleaseInfo(info ReleaseInfo, size int64, architecture string) Re
 					}
 				}
 			}
+		case "rpmdiff":
+			if strings.Contains(parts[1], "\x00") || strings.Contains(parts[2], "\x00") {
+				s, err = "", fmt.Errorf("invalid from/to")
+			} else {
+				var rpmdiff RpmDiff
+				rpmdiff, err = info.RpmDiff(parts[1], parts[2])
+				if err == nil {
+					var rpmdiffByte []byte
+					rpmdiffByte, err = json.Marshal(rpmdiff)
+					if err != nil {
+						klog.V(4).Infof("Failed to Marshal Rpm Diff; from: %s to: %s; %s", parts[1], parts[2], err)
+					} else {
+						s = string(rpmdiffByte)
+					}
+				}
+			}
 		case "changelog":
 			if strings.Contains(parts[1], "\x00") || strings.Contains(parts[2], "\x00") || strings.Contains(parts[3], "\x00") {
 				s, err = "", fmt.Errorf("invalid from/to")
@@ -109,6 +125,22 @@ func (c *CachingReleaseInfo) Bugs(from, to string) ([]BugDetails, error) {
 	return bugList(s)
 }
 
+func (c *CachingReleaseInfo) RpmDiff(from, to string) (RpmDiff, error) {
+	var s string
+	err := c.cache.Get(context.TODO(), strings.Join([]string{"rpmdiff", from, to}, "\x00"), groupcache.StringSink(&s))
+	if err != nil {
+		return RpmDiff{}, err
+	}
+	if s == "" {
+		return RpmDiff{}, nil
+	}
+	var rpmdiff RpmDiff
+	if err = json.Unmarshal([]byte(s), &rpmdiff); err != nil {
+		return RpmDiff{}, err
+	}
+	return rpmdiff, nil
+}
+
 func (c *CachingReleaseInfo) ChangeLog(from, to string, json bool) (string, error) {
 	var s string
 	err := c.cache.Get(context.TODO(), strings.Join([]string{"changelog", from, to, strconv.FormatBool(json)}, "\x00"), groupcache.StringSink(&s))
@@ -156,6 +188,7 @@ type ReleaseInfo interface {
 	// Bugs returns a list of jira bug IDs for bugs fixed between the provided release tags
 	Bugs(from, to string) ([]BugDetails, error)
 	ChangeLog(from, to string, json bool) (string, error)
+	RpmDiff(from, to string) (RpmDiff, error)
 	ReleaseInfo(image string) (string, error)
 	UpgradeInfo(image string) (ReleaseUpgradeInfo, error)
 	ImageInfo(image, architecture string) (string, error)
@@ -308,6 +341,56 @@ type BugDetails struct {
 	Source int    `json:"source"`
 }
 
+func (r *ExecReleaseInfo) RpmDiff(from, to string) (RpmDiff, error) {
+	if _, err := imagereference.Parse(from); err != nil {
+		return RpmDiff{}, fmt.Errorf("%s is not an image reference: %v", from, err)
+	}
+	if _, err := imagereference.Parse(to); err != nil {
+		return RpmDiff{}, fmt.Errorf("%s is not an image reference: %v", to, err)
+	}
+	if strings.HasPrefix(from, "-") || strings.HasPrefix(to, "-") {
+		return RpmDiff{}, fmt.Errorf("not a valid reference")
+	}
+
+	out, errOut := &bytes.Buffer{}, &bytes.Buffer{}
+	oc, err := exec.LookPath("oc")
+	if err != nil {
+		return RpmDiff{}, fmt.Errorf("Failed to find `oc` executable: %w", err)
+	}
+	cmd := exec.Command(oc, "adm", "release", "info", "--rpmdb-cache=/tmp/rpmdb/", "--output=json", "--rpmdb-diff", from, to)
+	klog.V(4).Infof("Running RPM diff command: %s", cmd.String())
+	cmd.Stdout = out
+	cmd.Stdin = nil
+	cmd.Stderr = errOut
+	if err := cmd.Run(); err != nil {
+		klog.V(4).Infof("Failed to generate RPM diff: %v\n$ %s\n%s\n%s", err, cmd.String(), errOut.String(), out.String())
+		msg := errOut.String()
+		if len(msg) == 0 {
+			msg = err.Error()
+		}
+		return RpmDiff{}, fmt.Errorf("could not generate RPM diff: %v", msg)
+	}
+	klog.V(4).Infof("Finished running RPM diff command: %s", cmd.String())
+
+	var rpmdiff RpmDiff
+	if err = json.Unmarshal(out.Bytes(), &rpmdiff); err != nil {
+		return RpmDiff{}, fmt.Errorf("unmarshaling RPM diff: %s", err)
+	}
+
+	return rpmdiff, nil
+}
+
+type RpmDiff struct {
+	Changed map[string]RpmChangedDiff `json:"changed,omitempty"`
+	Added   map[string]string         `json:"added,omitempty"`
+	Removed map[string]string         `json:"removed,omitempty"`
+}
+
+type RpmChangedDiff struct {
+	Old string `json:"old,omitempty"`
+	New string `json:"new,omitempty"`
+}
+
 type ReleaseUpgradeInfo struct {
 	Metadata *ReleaseUpgradeMetadata `json:"metadata"`
 }

pkg/rhcos/rhcos.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/url"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -31,14 +32,17 @@ var (
 	reMdCentOSCoSDiff    = regexp.MustCompile(`\* CentOS Stream CoreOS upgraded from ((\d)(\d+)\.[\w\.\-]+) to ((\d)(\d+)\.[\w\.\-]+)\n`)
 	reMdCentOSCoSVersion = regexp.MustCompile(`\* CentOS Stream CoreOS ((\d)(\d+)\.[\w\.\-]+)\n`)
 
+	// the postprocessed markdown diff line
+	reMdCoSDiffPost = regexp.MustCompile(`\* (.*? CoreOS upgraded from .*?) \(\[diff\]\(.*?\)\)`)
+
 	// regex for RHCOS in < 4.19, where the version string was e.g. 418.94.202410090804-0
 	reOcpCoreOsVersion = regexp.MustCompile(`((\d)(\d+))\.(\d+)\.(\d+)-(\d+)`)
 
 	// regex for RHCOS in >= 4.19, where the version string was e.g. 9.6.20250121-0
 	reRhelCoreOsVersion = regexp.MustCompile(`(\d+)\.(\d+)\.(\d+)-(\d+)`)
 )
 
-func TransformMarkDownOutput(markdown, fromPull, fromTag, toPull, toTag, architecture, architectureExtension string) (string, error) {
+func TransformMarkDownOutput(markdown, fromTag, toTag, architecture, architectureExtension string) (string, error) {
 	// replace references to the previous version with links
 	rePrevious, err := regexp.Compile(fmt.Sprintf(`([^\w:])%s(\W)`, regexp.QuoteMeta(fromTag)))
 	if err != nil {
@@ -58,9 +62,9 @@ func TransformMarkDownOutput(markdown, fromPull, fromTag, toPull, toTag, archite
 	// TODO: As we get more comfortable with these sorts of transformations, we could make them more generic.
 	//       For now, this will have to do.
 	if m := reMdRHCoSDiff.FindStringSubmatch(markdown); m != nil {
-		markdown = transformCoreOSUpgradeLinks(rhelCoreOs, fromPull, toPull, architecture, architectureExtension, markdown, m)
+		markdown = transformCoreOSUpgradeLinks(rhelCoreOs, architecture, architectureExtension, markdown, m)
 	} else if m = reMdCentOSCoSDiff.FindStringSubmatch(markdown); m != nil {
-		markdown = transformCoreOSUpgradeLinks(centosStreamCoreOs, fromPull, toPull, architecture, architectureExtension, markdown, m)
+		markdown = transformCoreOSUpgradeLinks(centosStreamCoreOs, architecture, architectureExtension, markdown, m)
 	}
 	if m := reMdRHCoSVersion.FindStringSubmatch(markdown); m != nil {
 		markdown = transformCoreOSLinks(rhelCoreOs, architecture, architectureExtension, markdown, m)
@@ -179,7 +183,7 @@ func getRHCoSReleaseStream(version, architectureExtension string) (string, bool)
 	return "", false
 }
 
-func transformCoreOSUpgradeLinks(name, fromPull, toPull, architecture, architectureExtension, input string, matches []string) string {
+func transformCoreOSUpgradeLinks(name, architecture, architectureExtension, input string, matches []string) string {
 	var ok bool
 	var fromURL, toURL url.URL
 	var fromStream, toStream string
@@ -214,41 +218,40 @@ func transformCoreOSUpgradeLinks(name, fromPull, toPull, architecture, architect
 		}
 	}
 
-	// if either from or to release is of the new style, add an infobox
-	alertInfo := ""
-	if !strings.HasPrefix(fromRelease, "4") || !strings.HasPrefix(toRelease, "4") {
-		alertInfo = fmt.Sprintf(`
-<div class="alert alert-info">
-	<p>The RHCOS diff above only contains RHEL packages (e.g. kernel, systemd, ignition, coreos-installer).
-	For a full diff which includes OCP packages (e.g. openshift-clients, openshift-kubelet), run the
-	following command:</p>
-
-<code style="white-space: pre-wrap">img_from=$(oc adm release info --image-for=rhel-coreos %s)
-img_to=$(oc adm release info --image-for=rhel-coreos %s)
-git diff --no-index <(podman run --rm $img_from rpm -qa | sort) <(podman run --rm $img_to rpm -qa | sort)</code>
-</div>`, fromPull, toPull)
-	}
+	// anything not starting with "4" means it's the new node images so we will
+	// render a package diff ourselves, see also:
+	// https://github.com/openshift/enhancements/blob/master/enhancements/rhcos/split-rhcos-into-layers.md#etcos-release
+	hasCoreosLayered := !strings.HasPrefix(fromRelease, "4") || !strings.HasPrefix(toRelease, "4")
+
+	// even for the few 4.19 releases we've had, we render the package diff so that we can test it out
+	hasCoreos419 := strings.HasPrefix(fromRelease, "419") || strings.HasPrefix(toRelease, "419")
 
-	diffURL := url.URL{
-		Scheme: serviceScheme,
-		Host:   serviceUrl,
-		Path:   "/diff.html",
-		RawQuery: (url.Values{
-			"first_stream":   []string{fromStream},
-			"first_release":  []string{fromRelease},
-			"second_stream":  []string{toStream},
-			"second_release": []string{toRelease},
-			"arch":           []string{architecture},
-		}).Encode(),
+	var diffURL string
+	if hasCoreos419 || hasCoreosLayered {
+		diffURL = "#coreos-package-diff"
+	} else {
+		diffURL = (&url.URL{
+			Scheme: serviceScheme,
+			Host:   serviceUrl,
+			Path:   "/diff.html",
+			RawQuery: (url.Values{
+				"first_stream":   []string{fromStream},
+				"first_release":  []string{fromRelease},
+				"second_stream":  []string{toStream},
+				"second_release": []string{toRelease},
+				"arch":           []string{architecture},
+			}).Encode(),
+		}).String()
 	}
+
 	replace := fmt.Sprintf(
-		`* %s upgraded from [%s](%s) to [%s](%s) ([diff](%s))`+"\n"+alertInfo,
+		`* %s upgraded from [%s](%s) to [%s](%s) ([diff](%s))`,
 		name,
 		fromRelease,
 		fromURL.String(),
 		toRelease,
 		toURL.String(),
-		diffURL.String(),
+		diffURL,
 	)
 	return strings.ReplaceAll(input, matches[0], replace)
 }
@@ -280,3 +283,48 @@ func transformCoreOSLinks(name, architecture, architectureExtension, input strin
 	)
 	return strings.ReplaceAll(input, matches[0], replace)
 }
+
+func RenderRpmDiff(markdown string, rpmDiff releasecontroller.RpmDiff) string {
+	output := new(strings.Builder)
+
+	// Reprint the upgrade line, with the browser links for the build itself.
+	if m := reMdCoSDiffPost.FindStringSubmatch(markdown); m != nil {
+		fmt.Fprintf(output, "%s\n\n", m[1])
+	}
+
+	writeList := func(header string, elements []string) {
+		fmt.Fprintf(output, "#### %s:\n\n", header)
+		sort.Strings(elements)
+		for _, elem := range elements {
+			fmt.Fprintf(output, "* %s\n", elem)
+		}
+	}
+
+	if len(rpmDiff.Changed) > 0 {
+		elements := []string{}
+		for pkg, v := range rpmDiff.Changed {
+			elements = append(elements, fmt.Sprintf("%s %s → %s", pkg, v.Old, v.New))
+		}
+		writeList("Changed", elements)
+	}
+	if len(rpmDiff.Removed) > 0 {
+		elements := []string{}
+		for pkg, v := range rpmDiff.Removed {
+			elements = append(elements, fmt.Sprintf("%s %s", pkg, v))
+		}
+		writeList("Removed", elements)
+	}
+	if len(rpmDiff.Added) > 0 {
+		elements := []string{}
+		for pkg, v := range rpmDiff.Added {
+			elements = append(elements, fmt.Sprintf("%s %s", pkg, v))
+		}
+		writeList("Added", elements)
+	}
+
+	if output.Len() == 0 {
+		return "No package diff"
+	}
+
+	return output.String()
+}