rpm: consult dnf database for repository information #869

hdonnay · 2023-03-13T20:42:36Z

These changes make the rhel machinery consult both the rpm and dnf databases when possible.

See-also: #809

codecov · 2024-01-12T17:19:14Z

Codecov Report

Attention: Patch coverage is 29.52381% with 74 lines in your changes missing coverage. Please review.

Project coverage is 55.23%. Comparing base (c2fc225) to head (57bb054).

Files	Patch %	Lines
rpm/dnf.go	25.71%	52 Missing ⚠️
rpm/packagescanner.go	14.28%	18 Missing ⚠️
rhel/coalescer.go	57.14%	1 Missing and 2 partials ⚠️
rpm/native_db.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #869      +/-   ##
==========================================
- Coverage   55.39%   55.23%   -0.17%     
==========================================
  Files         282      283       +1     
  Lines       17836    17923      +87     
==========================================
+ Hits         9881     9899      +18     
- Misses       6923     6990      +67     
- Partials     1032     1034       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hdonnay · 2024-12-19T23:12:57Z

rpm/dnf.go

+	if fi, err := fs.Stat(sys, `root/buildinfo/content_manifests`); errors.Is(err, nil) && fi.IsDir() {
+		// This is a RHEL layer, skip.
+		return nil, nil
+	}


Update this to check the files for a marker value and continue if found.

Should look for from_dnf_hint key in the yaml files.
See also: containerbuildsystem/atomic-reactor#2140

Punting on this unless we determine it's needed.

cgwalters · 2025-01-06T13:42:12Z

I created rpm-software-management/rpm#3505 related to this

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

Using these should give us a nice "todo" list when the time comes. Signed-off-by: Hank Donnay <hdonnay@redhat.com>

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

This adds two helpers: one to discover repoids, and one to annotate a stream of Packages. Signed-off-by: Hank Donnay <hdonnay@redhat.com>

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

RTann

made it through most of the files, but I'll need to come back to this another time. Would you mind making more commits and then squashing things at the end so it's easier to see the diffs after each round? Thanks

RTann · 2025-01-22T22:14:53Z

internal/dnf/dnf.go

+// outside of the Red Hat build system's builder's context. See [CLAIRDEV-45]
+// for more information.
+//
+// [CLAIRDEV-45]: https://issues.redhat.com/browse/CLAIRDEV-45


This ticket specifically calls out OSBS. What about Konflux? Should we update the ticket to mention Konflux, or is this not true for Konflux?

RTann · 2025-01-22T22:16:29Z

internal/dnf/dnf.go

+	"github.com/quay/claircore/internal/rpm"
+	"github.com/quay/zlog"
+	_ "modernc.org/sqlite" // register the sqlite driver


just sanity checking here

"github.com/quay/claircore/internal/rpm" is in the same module, so I expected to see this in the last block

RTann · 2025-01-22T22:17:45Z

internal/dnf/dnf.go

+	Wrap(context.Context, iter.Seq[claircore.Package]) (iter.Seq[claircore.Package], func() error)
+}
+
+// Identity is an [Annotator] who's [Annotator.Wrap] method does nothing.


RTann · 2025-01-22T22:18:25Z

internal/dnf/dnf.go

+var Identity Annotator
+
+func init() {
+	Identity = ident{}
+}


curious why not just var Identity = ident{}?

I think the idea here is the variable can't be (accidentally or deliberately) reassigned

RTann · 2025-01-22T22:19:54Z

internal/dnf/dnf.go

+	return nil
+}
+
+// NewAnnotator holds book-keeping for producing multiple independent mapping


This is where the convention of doing something like NewAnnotator instead of newAnnotator for the godoc confuses me because now we have two of these

RTann · 2025-01-23T00:03:00Z

internal/dnf/sql/repoid_for_package.sql

looks good, but I don't know the tables well enough to verify this is definitely the right query

RTann · 2025-01-23T00:05:04Z

internal/dnf/dnf.go

+					continue
+				default:
+					final = fmt.Errorf("internal/dnf: database error: %w", err)
+					return


I'm still getting used to iterators in Go. It's ok to have a path where we don't yield?

Yes the consumer will just see that at the end of the iteration (in this case there's also a consumable error so the reason for it should be known.

RTann · 2025-01-23T01:19:57Z

internal/rpm/find.go

+		switch n {
+		case `Packages`:
+			db.kind = kindBDB
+			if ok, err := testpath(p, bdb.CheckMagic); !ok {


confirming we want to ignore true, err != nil?

Yeah, testpath could return that if fs.File.Close() fails, I think that case should be accounted for.

RTann · 2025-01-23T01:40:15Z

internal/rpm/packages.go

+		blobs, dbErr := db.All(ctx)
+		seq, parseErr := parseBlob(ctx, blobs)
+		defer func() {
+			final = errors.Join(err, parseErr(), dbErr(), db.Close())


just confirming we want to close the DB here?

RTann · 2025-01-23T01:46:02Z

internal/rpm/info.go

+	//
+	// [yara]: https://github.com/VirusTotal/yara
+	pat := []string{
+		`^.*/[^/]+\.jar$`, // Jar files


I know this is copied over, but I'm wondering if we should extend this to the other file extensions we support like WAR, EAR, etc?

Yes, but maybe let's address that in #1473

RTann · 2025-01-24T20:13:19Z

rpm/packagescanner.go

-	for _, db := range found {
-		ctx := zlog.ContextWithValues(ctx, "db", db.String())
-		zlog.Debug(ctx).Msg("examining database")
-		if _, ok := done[db.Path]; ok {


confirming this is no longer needed because of the slices.CompactFunc in rpm.FindDBs?

That's what I read it as too, the slices.SortFunc is putting the found in order (so dbs with the same path are adjacent), then slices.CompactFunc is compacting them together, so by the time FindDBs returns the dupes should be taken care of

RTann · 2025-01-24T21:36:14Z

internal/rpm/find.go

+			switch {
+			case a.kind < b.kind:
+				cmp = -1
+			case a.kind > b.kind:
+				cmp = +1
+			}


why bother checking this if we only care about the path later? Also not sure if it's possible for paths to be the same but different kind at the moment anyway

RTann · 2025-01-24T22:14:51Z

rhel/packagescanner.go

+
+			seq, checkPkgs := db.Packages(ctx)
+			seq, checkDNF := a.Wrap(ctx, seq)
+			wart.AsPointer(seq)(yield)


This was kind of a 🤯 for me, and I had to play with it in Go Playground to understand this: https://go.dev/play/p/j2-vMOOmrGw

RTann · 2025-01-24T22:18:55Z

rhel/packagescanner_test.go

+		/*
+			for _, pkg := range got {
+				t.Logf("%s: %#q", pkg.Name, pkg.RepositoryHint)
+			}
+		*/


still desired or to delete?

RTann · 2025-01-24T22:25:29Z

rpm/packagescanner.go

 func (*Scanner) Name() string { return pkgName }

-// Version implements scanner.VersionedScanner.
+// Version implements [indexer.VersionedScanner].
 func (*Scanner) Version() string { return pkgVersion }


Just confirming: even though the code is very different, we aren't expecting different results, right?

RTann · 2025-01-24T22:34:56Z

rhel/repositoryscanner.go

+			ctx, done := context.WithTimeout(ctx, r.cfg.Timeout)
+			defer done()
+
+			// !!! This ends up being a weird nonlocal return. !!!


what does this mean?

I think it's saying that any error returned from the mapContainerAPI is not evaluated or returned until outside of this function body L#259

RTann · 2025-01-24T22:38:05Z

rhel/repositoryscanner.go

+			}
+		}
+
+		if r.apiFetcher != nil {


let's say this is enabled. we seem to go here, no matter what, right? Before, we only got here if we were unable to read the content-sets files or the prod sec data is faulty.

Yes, it seems like if len(repoids) == 0 && r.apiFetcher != nil would be preferred, I can't see a situation where we'd want to reach out to the container API if we had content-sets either from DNF or the embedded file.

crozzy · 2025-02-13T16:37:19Z

internal/rpm/testdata/Info.Files.SQLite.txtar

These txter test files are not net-new, I think they should be removed from rpm/testdata

crozzy · 2025-02-13T17:49:21Z

rhel/repositoryscanner.go

-		for _, cpe := range cpes.CPEs {
-			s[cpe] = struct{}{}
-		}
+func (m *mappingFile) GetOne(ctx context.Context, repoid string) (cpes []string, ok bool) {


This needs a comment

crozzy · 2025-02-13T21:11:34Z

internal/dnf/dnf.go

+	db    *sql.DB
+
+	// Concurrent maps for memoizing database lookups.
+	absent sync.Map


Is there a specific reason for using an absent map here and not saving an empty struct in repo that can be checked to signal if it's absent, readability?

crozzy · 2025-02-14T16:48:15Z

rhel/coalescer.go

-					environment.RepositoryIDs[i] = layerArtifacts.Repos[i].ID
+				v, _ := url.ParseQuery(pkg.RepositoryHint)
+				if id := v.Get("repoid"); id != "" {
+					environment.RepositoryIDs = v["repoid"]


This will leave inconsistencies in how the repo IDs are reported in index report. Packages with no specific repos will report their repo ids as numerical with the ability to be referenced back to repository objects (like current behaviour):

"repository_ids": [ "1", "2", ...

But packages that have a specific repoid specified by the DNF DB will have the repo slug that can not be directly referenced back to a repository object:

"repository_ids": [ "rhel-9-for-x86_64-appstream-rpms" ]

We should think of a way to make these consistent to keep the expectation that "repository_ids" will refer to keys in the "repository" section and not a mix of the claircore.Repository.ID and (what will be persisted as) the claircore.Repository.Name.

Could do something like this:

func lookupRepositoryID(repos []*claircore.Repository, slug string) string { for _, r := range repos { if r.Name == slug { return r.ID } } return "" }

and then

if slug := v.Get("repoid"); slug != "" { if id := lookupRepositoryID(layerArtifacts.Repos, slug); id != "" { environment.RepositoryIDs = []string{id} }

This should always find a repo as repository scanner should be finding the same repos as the package scanner.

This suggestion is incomplete in that we need to append all repositories who's Name == repoid, not just the first one as the example code does

Rename to NERVA to NEVRA

hdonnay force-pushed the hack/dnf branch from 4b2aa2d to 9284a22 Compare April 5, 2023 17:55

hdonnay force-pushed the hack/dnf branch 2 times, most recently from 5acce04 to 5a9d76e Compare April 13, 2023 15:03

hdonnay mentioned this pull request Jan 12, 2024

rpm et. al.: lints #1205

Merged

hdonnay force-pushed the hack/dnf branch from 5a9d76e to cb74edd Compare January 12, 2024 17:10

hdonnay force-pushed the hack/dnf branch from cb74edd to 9e040ff Compare January 12, 2024 19:32

hdonnay force-pushed the hack/dnf branch from 9e040ff to 57bb054 Compare August 27, 2024 16:41

hdonnay force-pushed the hack/dnf branch from 57bb054 to f86dd34 Compare December 19, 2024 22:59

hdonnay changed the title ~~wip: rpm: consult dnf database for repository information~~ rpm: consult dnf database for repository information Dec 19, 2024

hdonnay force-pushed the hack/dnf branch from f86dd34 to 18e9818 Compare December 19, 2024 23:04

hdonnay commented Dec 19, 2024

View reviewed changes

hdonnay force-pushed the hack/dnf branch from 18e9818 to 69ba836 Compare December 20, 2024 22:17

hdonnay force-pushed the hack/dnf branch 3 times, most recently from 10b07ff to e59f165 Compare January 13, 2025 15:57

hdonnay force-pushed the hack/dnf branch from e59f165 to 4862693 Compare January 22, 2025 19:23

hdonnay added 6 commits January 22, 2025 13:52

internal/rpm: new iterator-based rpm package

58c5c61

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

internal/rpm/sqlite: better error return for Validate

787c3e0

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

wart: add helpers for iter.Seq[T] implementations

78c26b0

Using these should give us a nice "todo" list when the time comes. Signed-off-by: Hank Donnay <hdonnay@redhat.com>

rpm: move to use new internal/rpm package

cc4677a

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

internal/rpm: change "RepositoryHint" format

79f59a7

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

internal/dnf: add dnf-handling package

1eb75f1

This adds two helpers: one to discover repoids, and one to annotate a stream of Packages. Signed-off-by: Hank Donnay <hdonnay@redhat.com>

hdonnay force-pushed the hack/dnf branch from 4862693 to 8c86d88 Compare January 22, 2025 19:54

hdonnay added 5 commits January 22, 2025 13:58

rpmtest: allow specifying repository hint

77b8b68

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

rhel: add package scanner

ff75397

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

rhel: fix testcase names

9f75bb4

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

rhel: have repository scanner consult dnf

26642a6

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

rhel: use repoids in coalescer

9ea862e

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

containerapi: add deprecation notices

7a459ba

Signed-off-by: Hank Donnay <hdonnay@redhat.com>

hdonnay force-pushed the hack/dnf branch from 8c86d88 to 7a459ba Compare January 22, 2025 20:08

hdonnay marked this pull request as ready for review January 22, 2025 20:22

hdonnay requested a review from a team as a code owner January 22, 2025 20:22

hdonnay requested review from RTann and removed request for a team January 22, 2025 20:22

RTann requested a review from crozzy January 22, 2025 22:11

RTann reviewed Jan 23, 2025

View reviewed changes

RTann reviewed Jan 24, 2025

View reviewed changes

crozzy reviewed Feb 13, 2025

View reviewed changes

crozzy reviewed Feb 14, 2025

View reviewed changes

crozzy and others added 10 commits February 24, 2025 00:07

fixup! internal/dnf: add dnf-handling package

0797287

fixup! internal/dnf: add dnf-handling package

e8469e9

fixup! internal/dnf: add dnf-handling package

b6c5b3b

fixup! internal/dnf: add dnf-handling package

7a9803c

fixup! internal/dnf: add dnf-handling package

8a75493

fixup! internal/dnf: add dnf-handling package

f492c89

fixup! internal/dnf: add dnf-handling package

de0886e

fixup! internal/dnf: add dnf-handling package

0123676

fixup! internal/dnf: add dnf-handling package

d9d5f87

fixup! internal/rpm: new iterator-based rpm package

c1346ba

Rename to NERVA to NEVRA

BradLugo force-pushed the hack/dnf branch from 2b58b35 to c1346ba Compare February 24, 2025 08:08

rpm: consult dnf database for repository information #869

Are you sure you want to change the base?

rpm: consult dnf database for repository information #869

Conversation

hdonnay commented Mar 13, 2023 • edited Loading

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Jan 6, 2025

RTann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crozzy Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdonnay commented Mar 13, 2023 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

crozzy Feb 19, 2025 •

edited

Loading