Introduce storage module #114

jcrossley3 · 2024-03-25T20:08:52Z

This includes the commits from #113 and resolves the conflicts incurred from #116

I modified the StorageBackend a tad, breaking out the "faux sync" methods into a helper class.

I don't love having `retrieve_sync` and `retrieve_buf` as part of the primary Storage trait, since it seems they only exist because serde can't deserialize async streams. If there's no more elegant way to overcome that, I'd prefer we break them out into some sort of "helper class". Ideally, the storage trait would have only retrieve and store. Signed-off-by: Jim Crossley <jim@crossleys.org>

Signed-off-by: Jim Crossley <jim@crossleys.org>

Remove advisory format for the time being Signed-off-by: Jim Crossley <jim@crossleys.org>

Includes the introduction of a Format enum with tests for both OSV and CSAF formats. Signed-off-by: Jim Crossley <jim@crossleys.org>

bobmcwhirter

Everything I've commented on is just nit-picky.

I defer to your discretion as to what you want to address in this PR or in follow-ups.

I also will assume the burden of rebasing my subsequent PR so you can get this one landed.

bobmcwhirter · 2024-03-28T18:46:34Z

modules/importer/src/server/csaf/storage.rs

+        self.ingestor
+            .ingest(
+                &location,
+                Format::CSAF,


Typing while I read, this may be answered below but...

Can we link the Format::CSAF/OSV etc to the actual path param in the REST API, instead of being stringly-typed as I left it?

I assume this question will be answered by #122 ?

bobmcwhirter · 2024-03-28T18:47:09Z

modules/importer/src/server/mod.rs


 /// run the importer loop
-pub async fn importer(db: Database) -> anyhow::Result<()> {
-    Server { db }.run().await
+pub async fn importer(db: Database, storage: DispatchBackend) -> anyhow::Result<()> {


Why is a variable named storage of a type named DispatchBackend?

I dunno, honestly. There is a comment left by @ctron in dispatch.rs but I didn't fully grok it.

bobmcwhirter · 2024-03-28T18:50:07Z

modules/ingestor/src/endpoints/advisory.rs

    payload: web::Payload,
+    web::Query(UploadAdvisoryQuery { location, format }): web::Query<UploadAdvisoryQuery>,


Moving to query params instead of path param? I'm fine with that. We should probably start a Tenets document or discussion to ensure we keep consistent and how we decide which option to choose.

Cool. I don't have a strong opinion either way, I was just reusing the Query struct.

bobmcwhirter · 2024-03-28T18:50:09Z

modules/ingestor/src/endpoints/advisory.rs

-    }
+    let fmt = format
+        .map(|f| Format::from_str(&f))
+        .unwrap_or(Ok(Format::CSAF))?;


Default to CSAF format? If someone POSTs in a RANDOFORMAT, which is not actually CSAF, then we'll end up with a bunch of "this is invalid CSAF" errors instead of "We don't support RANDOFORMAT" yeah?

My logic may be flawed. How should it work?

Maybe .ok_or(Error::InvalidFormat)? which will turn a None into a throw error, or a Some into the underlying value without unwrap. Sorta the err-variant of unwrap_or(...)

I think my logic is correct: I'm mapping and unwrapping an Option containing a Result. So the unwrap_or is returning an Err if the format param is invalid. The Ok(Format::CSAF) is only returned if format is None.

See #126

bobmcwhirter · 2024-03-28T18:50:41Z

modules/ingestor/src/endpoints/advisory.rs

+    }
+
+    #[test_log::test(actix_web::test)]
+    async fn upload_osv() -> Result<(), anyhow::Error> {


bobmcwhirter · 2024-03-28T18:51:34Z

modules/ingestor/src/service/advisory/csaf/mod.rs

+            .retrieve(sha256.clone())
+            .await
+            .map_err(Error::Storage)?
+            .ok_or_else(|| Error::Storage(anyhow!("file went missing during upload")))?;


For bonus points, do we want to re-hash to also ensure file contents didn't change during the shuffling?

I think being able to successfully retrieve it ensures that, right?

I dunno tbh. I think it means we managed to find some file at the hash-named location. Belt-and-suspenders would then verify, I guess, if the file retrieved by hash, when hashed, hashes to the hash of the hash-based file.

Yo dawg, I heard you liked hash, so I put some hash in your hash so you can hash while you hash.

I see what you mean, verify we got what we put.

bobmcwhirter · 2024-03-28T18:52:30Z

modules/ingestor/src/service/advisory/csaf/mod.rs

+            .ok_or_else(|| Error::Storage(anyhow!("file went missing during upload")))?;
+
+        let result = match fmt {
+            Format::CSAF => {


Not for this PR, but when we add a 3rd format, probably smart to shuffle into a fn on Format itself to handle the delegation to the appropriate loader?

bobmcwhirter · 2024-03-28T18:53:49Z

modules/storage/src/service/dispatch.rs

+    Filesystem(FileSystemBackend),
+}
+
+impl StorageBackend for DispatchBackend {


Ahhh, DispatchBackend is-a form of storage. Ignore previous.

I reply to these in order. 😄

bobmcwhirter · 2024-03-28T18:54:45Z

modules/storage/src/service/fs.rs

+const NUM_LEVELS: usize = 2;
+
+impl FileSystemBackend {
+    pub async fn new(base: impl Into<PathBuf>) -> anyhow::Result<Self> {


See my pending PR for trustd. We should maybe default to .trustify/...whatever for the Simple Happy PM Use-Case?

(not this PR though)

Should this go in #121 ?

bobmcwhirter · 2024-03-28T18:55:40Z

modules/storage/src/service/fs.rs

+
+        let hash = hex::encode(digest);
+        let target = level_dir(&self.content, &hash, NUM_LEVELS);
+        create_dir_all(&target).await.map_err(StoreError::Backend)?;


Should we sprinkle some from derives or impl From<..> to avoid having to map_err(...)?

Probably. I expect much of the storage module to evolve.

bobmcwhirter

I meant to tick the [approve] box.

jcrossley3 mentioned this pull request Mar 25, 2024

feat: add some filesystem storage #113

Closed

jcrossley3 force-pushed the feature/storage_1 branch from dafaa20 to 0356a79 Compare March 27, 2024 20:37

ctron and others added 6 commits March 27, 2024 17:31

fix: add missing drop for "at"

9af01df

feat: add some filesystem storage

0cd7ebb

build: deny using expect in non-test code

d0e571a

chore: basic storage retrieval

de07183

Break out sync retrieve behavior into a helper struct

167d954

Signed-off-by: Jim Crossley <jim@crossleys.org>

jcrossley3 force-pushed the feature/storage_1 branch 2 times, most recently from 0ac80b3 to 84e867f Compare March 28, 2024 01:25

Resolve conflicts from rebase

9dfa13e

Remove advisory format for the time being Signed-off-by: Jim Crossley <jim@crossleys.org>

jcrossley3 force-pushed the feature/storage_1 branch from 84e867f to 9dfa13e Compare March 28, 2024 13:15

Restore advisory format logic

4aa3d26

Includes the introduction of a Format enum with tests for both OSV and CSAF formats. Signed-off-by: Jim Crossley <jim@crossleys.org>

jcrossley3 changed the title ~~Manifest review of #113~~ Introduce storage module Mar 28, 2024

jcrossley3 marked this pull request as ready for review March 28, 2024 15:52

jcrossley3 requested a review from bobmcwhirter March 28, 2024 15:52

bobmcwhirter reviewed Mar 28, 2024

View reviewed changes

bobmcwhirter approved these changes Mar 28, 2024

View reviewed changes

jcrossley3 added this pull request to the merge queue Mar 28, 2024

This was referenced Mar 28, 2024

Verify behavior when invalidly formatted advisory is uploaded #123

Closed

Verify what's retrieved is what was stored #124

Closed

Hang the loader delegation on the Format enum #125

Closed

Merged via the queue into trustification:main with commit 70c8baa Mar 28, 2024
3 checks passed

jcrossley3 deleted the feature/storage_1 branch March 28, 2024 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce storage module #114

Introduce storage module #114

jcrossley3 commented Mar 25, 2024 •

edited

Loading

bobmcwhirter left a comment

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter Mar 28, 2024

jcrossley3 Mar 28, 2024

bobmcwhirter left a comment

		payload: web::Payload,
		web::Query(UploadAdvisoryQuery { location, format }): web::Query<UploadAdvisoryQuery>,

Introduce storage module #114

Introduce storage module #114

Conversation

jcrossley3 commented Mar 25, 2024 • edited Loading

bobmcwhirter 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobmcwhirter left a comment

Choose a reason for hiding this comment

jcrossley3 commented Mar 25, 2024 •

edited

Loading