Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: move debian parser from vulnerability dict to dataclass #645

Closed
wants to merge 3 commits into from

Conversation

willmurphyscode
Copy link
Contributor

Previously, the parser was using a deep copy of a dict literal to make each new instance of the vulnerability it was emitting. Vunnel has since added a dataclass to represent vulnerabilities. Switch to using that dataclass.

See #644

Previously, the parser was using a deep copy of a dict literal to make
each new instance of the vulnerability it was emitting. Vunnel has since
added a dataclass to represent vulnerabilities. Switch to using that
dataclass.

Signed-off-by: Will Murphy <[email protected]>
@willmurphyscode willmurphyscode added the run-pr-quality-gate Triggers running of quality gate on PRs label Jul 30, 2024
@willmurphyscode willmurphyscode self-assigned this Jul 30, 2024
Signed-off-by: Will Murphy <[email protected]>
@willmurphyscode
Copy link
Contributor Author

It looks like the quality gate is failing because somehow there are a mix of Vulnerability dataclasses and dicts in vuln_records:

   File "/home/runner/work/vunnel/vunnel/src/vunnel/providers/debian/parser.py", line 553, in get
    vuln_record["Vulnerability"].Severity = "Unknown"
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'dict' object has no attribute 'Severity'

@westonsteimel
Copy link
Contributor

It looks like the quality gate is failing because somehow there are a mix of Vulnerability dataclasses and dicts in vuln_records:

   File "/home/runner/work/vunnel/vunnel/src/vunnel/providers/debian/parser.py", line 553, in get
    vuln_record["Vulnerability"].Severity = "Unknown"
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'dict' object has no attribute 'Severity'

@willmurphyscode , could that be due to the "legacy" record path?

@willmurphyscode
Copy link
Contributor Author

@westonsteimel that seems likely. I knew the Debian provider was downloading cached data. There must be a place where it's parsing that as dict instead of Vulnerability.

Do you think it's safe to change that path? I know there's some information we care about that's only available in the legacy cache, so I'm wondering if I should abandon this for the Debian provider in particular. But I can't think of a reason that writing those records back to the cache as Vulnerability would matter - the cache is just JSON if I recall correctly.

@westonsteimel
Copy link
Contributor

It should be safe to do. Alex recently changed it so that the legacy cache is a sqlite db with exactly the vunnel result shape

@willmurphyscode
Copy link
Contributor Author

This is waiting on #647 so that any differences in snapshots will be possible to review.

@westonsteimel
Copy link
Contributor

westonsteimel commented Jul 31, 2024

Oh, one danger with python dataclass that we ran into with enterprise is that it can't handle new properties being added to the json. So, for instance, if we added a new property to the os schema (non breaking change) and published that and then consumed in proxy mode, old vunnel clients using dataclass would actually fail due to an additional unknown property being present.

So for enterprise specifically we've replaced most of the uses of dataclass with pydantic models; however, that is a rather weighty dependency to bring in to vunnel, and enterprise is stuck on v1 of pydantic for the foreseeable future. That won't be a hindrance for vunnel much longer though since vunnel will cease to be a dependency of enterprise in the next release

@willmurphyscode
Copy link
Contributor Author

@westonsteimel does that mean we should hold off on this type of change until the next release?

@willmurphyscode
Copy link
Contributor Author

There's doubt about whether this is the right direction and it's not on top of the priority pile. We can revisit this if it comes up again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-pr-quality-gate Triggers running of quality gate on PRs
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants