diff --git a/cmd/release-controller-api/http.go b/cmd/release-controller-api/http.go index d357e14c0..a813dec54 100644 --- a/cmd/release-controller-api/http.go +++ b/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: diff --git a/cmd/release-controller-api/http_changelog.go b/cmd/release-controller-api/http_changelog.go index ba8c790ad..95aad3657 100644 --- a/cmd/release-controller-api/http_changelog.go +++ b/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, `

%s

`, 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, "

CoreOS Package Diff

") + + // 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, `

Package diff still loading; check again later...

`) + case render = <-chRpmDiff: + if render.err != nil { + fmt.Fprintf(w, `

%s

`, 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) + } + } + } } diff --git a/pkg/release-controller/release_info.go b/pkg/release-controller/release_info.go index a847d9f53..ac3a03c1c 100644 --- a/pkg/release-controller/release_info.go +++ b/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"` } diff --git a/pkg/rhcos/rhcos.go b/pkg/rhcos/rhcos.go index ee8a82d66..07b6c7d97 100644 --- a/pkg/rhcos/rhcos.go +++ b/pkg/rhcos/rhcos.go @@ -5,6 +5,7 @@ import ( "fmt" "net/url" "regexp" + "sort" "strconv" "strings" @@ -31,6 +32,9 @@ 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+)`) @@ -38,7 +42,7 @@ var ( 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(` -
-

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:

- -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) -
`, 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() +}