[Vulnerabilites]Add IVulnerabilityWriter #9712

ryuyu · 2023-10-17T23:55:59Z

Update the Github Vulnerability interaction library to support IVulnerabilityWriters for writing vulnerabilities.
This is in preparation for use by the new v3 vulnerability resource updater.

Spec for Vulnerabilities in V3: https://github.com/NuGet/Engineering/pull/4940

zhhyu · 2023-10-18T19:03:40Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Extensions.Logging;
+using NuGet.Services.Entities;


nit: order of dependencies.

Moved System above to be consistent with other files.

zhhyu · 2023-10-18T19:05:18Z

src/NuGet.Services.GitHub/Ingest/IVulnerabilityWriter.cs

+{
+    public interface IVulnerabilityWriter
+    {
+        Task<bool> WriteVulnerabilityAsync(PackageVulnerability vulnerability, bool wasWithdrawn = false);


nit: add the description of an interface.

zhhyu · 2023-10-18T19:57:02Z

src/NuGet.Services.GitHub/Entities/Advisory.cs

+    public class Advisory
+    {
+
+        [JsonProperty(PropertyName = "url")]


nit: empty line above.

zhhyu · 2023-10-18T19:59:25Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+        public async void FlushAsync(string outputFileName = null)
+        {
+            // This method is unused for this implementation.
+            throw new NotImplementedException();


What will this method be used for?

This method will be for when we aren't directly writing to the store location, which isn't the case for DB (DB implementation writes to DB on every call of WriteVulnerability).
The implementation planned for the v3 resource will internerally track all added vulnerabilities, then call Flush to write it out the final location (blob storage).
In short, IVulnerabilityWriter is treated sort of as a local cache of expected changes, and we call Flush to commit them. I didn't want to change DB writer behaviour, so this paradigm is ignored for DB implementation.

Note: As mentioned in #9712 (comment) I have updated this to no-op instead.

zhhyu · 2023-10-18T21:09:16Z

src/GitHubVulnerabilities2Db/Job.cs

@@ -66,6 +66,10 @@ protected void ConfigureIngestionServices(ContainerBuilder containerBuilder)
                .RegisterType<PackageVulnerabilitiesManagementService>()
                .As<IPackageVulnerabilitiesManagementService>();

+            containerBuilder.RegisterType<GalleryDbVulnerabilityWriter>()
+                .As<IVulnerabilityWriter>()
+                .SingleInstance();


I am a little concerned about whether it is safe to use singleton here or whether there is a need to do so. Is there any consideration to use singleton?

The GalleryDbVulnerabilityWriter is a very simple passthrough implementation.
Since the PacakgeVulnerabilitiesManagementService above it that it passes through to is not single instance, we can probably drop it here. Good catch.

zhhyu · 2023-10-18T21:38:13Z

src/NuGet.Services.GitHub/Ingest/IVulnerabilityWriter.cs

+        /// Commit changes to source
+        /// </summary>
+        /// <param name="outputFileName">Optional parameter to output to disk.</param>
+        void FlushAsync(string outputFileName = null);


void

void -> Task. It is better to avoid using async-void.

zhhyu · 2023-10-18T21:42:57Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+            try
+            {
+                await _packageVulnerabilityService.UpdateVulnerabilityAsync(vulnerability, wasWithdrawn);
+                return true;


return true;

Do we need to return a bool value? If exceptions are thrown, it will always terminate the job because of the rethrow.

That's a good question. I'll look in to this to find out if there was a reason I made it return a bool in the first place.

Doesn't seem like I can think of a good reason that it returned bool here as it is unused. I'll wait on making changes for now to see if anybody else has any opinions on this, and then I'll change it.

zhhyu · 2023-10-18T22:48:44Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+        }
+
+        public async Task<int> WriteVulnerabilitiesAsync(IEnumerable<PackageVulnerability> vulnerabilities)
+        {


It seems that this method is not called by 2Db job. Do we need the implementation?

That's a great point.
It is a trivial implementation, so we can probably drop it here.

After thinking about it a bit, I would like to leave this here in case the library changes more to call these methods on this interface. To this point, I have also updated the throwing implementation above to trivially do nothing and return.

zhhyu · 2023-10-18T22:49:58Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+
+namespace GitHubVulnerabilities2Db.Gallery
+{
+    public class GalleryDbVulnerabilityWriter : IVulnerabilityWriter


Consider adding test coverage for this class.

Test coverage was effectively already added. The existing tests call through this class now.

zhhyu · 2023-10-18T22:52:14Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+            }
+            catch (Exception ex)
+            {
+                _logger.LogError("Failed to write vulnerability: {ErrorMessage}", ex.Message);


Maybe it is better to enrich this exception message, with the identity of a vulnerability like "GitHubDatabaseKey".

I have added a new error logging line here that logs the github Advisory URL that failed.

zhhyu · 2023-10-18T22:56:42Z

src/NuGet.Services.GitHub/Entities/IndexEntry.cs

+
+namespace NuGet.Services.GitHub.Entities
+{
+    public class IndexEntry


This one and "Advisory" are not used in this PR. This "IndexEntry" seems matching the data schema in vulnerability documents. Will it be better that we place these data entities next to the new 2Blob job, rather than put it in the shared layer that is also called by 2Db job?

Hmm. That's a good point. I'll pull them out of this PR. Might be a leftover from when Writer implementations were inside this library instead of the responsibility of the caller.

zhhyu · 2023-10-18T23:03:16Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+
+            foreach(var vulnerability in vulnerabilities)
+            {
+                if (await WriteVulnerabilityAsync(vulnerability))


WriteVulnerabilityAsync(vulnerability)

Any vulnerability in the list can be in the state of "Withdrawn", am I right?

It looks like the PackageVulnerability Entity that is used to pass around here doesn't contain that information. Let me see how feasible it is to create a wrapper class that splits into the NuGet.Entity class.

I updated the API to take a list of Tuples instead of the single PacakgeVulnerability IEnumerable.
This should enable us to pass through the wasWithdrawn.

zhhyu · 2023-10-18T23:11:48Z

tests/GitHubVulnerabilities2Db.Facts/AdvisoryIngestorFacts.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Linq;
 using System.Threading.Tasks;
+using GitHubVulnerabilities2Db.Gallery;


nit: order of dependencies.

erdembayar · 2023-10-20T00:02:39Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+            catch (Exception ex)
+            {
+                _logger.LogError("Failed to write vulnerability {GitHubAdvisoryUrl}", vulnerability.AdvisoryUrl);
+                _logger.LogError("Error Message: {ExceptionMessage}", ex.Message);


Why are we making 2 separate error logs for same exception?
Usually we just log once per exception.

Logging 2 lines splits the lines in AI. I figured that having both information was useful, but since we are rethrowing the exception below, we can probably skip the log of the Exception message as it should show up in logs as exception. I'll take it out.

erdembayar

Could you please add link to tracking issue and spec in description?
And give little more description of exactly what part of spec being implemented here.

ryuyu · 2023-10-23T21:01:36Z

Could you please add link to tracking issue and spec in description? And give little more description of exactly what part of spec being implemented here.

I have included a link the Spec for Vulnerabilities in V3 to the description.
This is a prerequisite change to the existing code to enable a different write target for the vulnerabilities (this library was previously part of another job that was extracted in #9657

agr · 2023-10-26T00:48:52Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+            {
+                var dbVulnerability = vulnerability.Item1;
+                var wasWithdrawn = vulnerability.Item2;
+                await WriteVulnerabilityAsync(dbVulnerability, wasWithdrawn);


This implementation seems to be inserting vulnerabilities one by one. I have two concerns about that: if we are going to be inserting rows in SQL DB, inserting single row at a time is considerably slower than inserting a batch of rows in a single statement/transaction (it might not matter though if the number of rows inserted is low). Second is about failure recovery: what happens if it starts failing after writing some vulnerabilities?

You concern is totally valid, but this is how the current implementation works.
I think if it fails after writing some, it should be ok as worst case, it should just rewrite/modify the existing vulnerabilities.
But I think fixing this at this time is beyond the scope of this PR.
I am again making minimal changes to implementation.

This method is actually not used anywhere, is it? If so, maybe just throw NotImplementedException()?

At the very least, please file a work item to update IPackageVulnerabilitiesManagementService to allow batch updates.

@ryuyu
Have you filed work item for this?

Sorry about the delay on this folks. I have filed #9728 tracking the work @agr mentioned above.

zhhyu · 2023-10-27T01:06:21Z

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

+        public async Task FlushAsync(string outputFileName = null)
+        {
+            // This method is unused for this implementation.
+            return;


Maybe throwing NotImplementedException() is better?

I debated this a lot internally, but with @agr 's suggestion at #9712 (comment), I think we will need a non throwing implementation.

zhhyu · 2023-10-27T01:09:22Z

Thanks for addressing the feedback! I suggest throwing NotImplementedException() for unused implementations.

agr · 2023-10-27T22:52:39Z

src/NuGet.Services.GitHub/Ingest/AdvisoryIngestor.cs

@@ -37,7 +37,7 @@ public async Task IngestAsync(IReadOnlyList<SecurityAdvisory> advisories)
                var vulnerabilityTuple = FromAdvisory(advisory);
                var vulnerability = vulnerabilityTuple.Item1;
                var wasWithdrawn = vulnerabilityTuple.Item2;
-                await _packageVulnerabilityService.UpdateVulnerabilityAsync(vulnerability, wasWithdrawn);
+                await _vulnerabilityWriter.WriteVulnerabilityAsync(vulnerability, wasWithdrawn);


Should Flush be called after all writes are done?

That's a good call. I'll add a call to flush here after so we don't need to make this change later. Note @zhhyu that this change will require a non throwing implementation of flush for the GalleryDbWriter.

Add IVulnerabilityWriter for use in v3 vuln updater.

f0b6787

ryuyu self-assigned this Oct 17, 2023

ryuyu requested a review from a team as a code owner October 17, 2023 23:56

zhhyu reviewed Oct 18, 2023

View reviewed changes

Comments, some spacing/ordering nits.

e8e1943

zhhyu reviewed Oct 18, 2023

View reviewed changes

ryuyu added 2 commits October 18, 2023 16:21

PR feedback updates

3a2e0e1

Update multiwrite to support wasWithdrawn.

9be24d4

keylime-unicorn approved these changes Oct 19, 2023

View reviewed changes

erdembayar reviewed Oct 20, 2023

View reviewed changes

Remove extraneous log.

5a329ac

agr reviewed Oct 26, 2023

View reviewed changes

zhhyu reviewed Oct 27, 2023

View reviewed changes

zhhyu previously approved these changes Oct 27, 2023

View reviewed changes

erdembayar previously approved these changes Oct 27, 2023

View reviewed changes

agr reviewed Oct 27, 2023

View reviewed changes

Call Flush

6f43ced

ryuyu dismissed stale reviews from erdembayar and zhhyu via 6f43ced October 27, 2023 23:24

mariaghiondea approved these changes Oct 31, 2023

View reviewed changes

ryuyu merged commit 11a9f42 into dev Oct 31, 2023

ryuyu mentioned this pull request Nov 7, 2023

Update IPackageVulnerabilitiesManagementService to allow batch updates. #9728

Open

advay26 mentioned this pull request Nov 10, 2023

[Deployment] 2023.11.10 #9730

Closed

11 tasks

ryuyu deleted the ryuyu-githublib-update branch March 16, 2024 02:23

[Vulnerabilites]Add IVulnerabilityWriter #9712

[Vulnerabilites]Add IVulnerabilityWriter #9712

Conversation

ryuyu commented Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Oct 18, 2023 • 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

zhhyu Oct 18, 2023 • 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

zhhyu Oct 18, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Oct 18, 2023 • 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

erdembayar Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar left a comment

Choose a reason for hiding this comment

ryuyu commented Oct 23, 2023

agr Oct 26, 2023 • 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

Choose a reason for hiding this comment

zhhyu commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryuyu commented Oct 17, 2023 •

edited

Loading

zhhyu Oct 18, 2023 •

edited

Loading

zhhyu Oct 18, 2023 •

edited

Loading

zhhyu Oct 18, 2023 •

edited

Loading

zhhyu Oct 18, 2023 •

edited

Loading

erdembayar Oct 20, 2023 •

edited

Loading

agr Oct 26, 2023 •

edited

Loading