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

Exclude snakeyaml from pinot dependencies #16842

Merged
merged 3 commits into from
Apr 7, 2023

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Apr 1, 2023

It's used transitively by calcite to load model definitions

Description

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Apr 1, 2023
@wendigo wendigo requested a review from hashhar April 1, 2023 18:35
kokosing
kokosing previously approved these changes Apr 1, 2023
plugin/trino-pinot/pom.xml Show resolved Hide resolved
@martint
Copy link
Member

martint commented Apr 2, 2023

What’s the motivation for this?

@wendigo
Copy link
Contributor Author

wendigo commented Apr 2, 2023

@martint snakeyaml has known vulnerabilites that are reportered by security scanners. We don't use it so it's better to exclude it

@martint
Copy link
Member

martint commented Apr 2, 2023

If it’s not used then those vulnerabilities don’t affect Trino, so what problem does it solve?

@wendigo
Copy link
Contributor Author

wendigo commented Apr 2, 2023

@martint Some security scanners are eager to report vulnerability by mere existence of the jar. This particular CVE has score of 9.8/10

@martint
Copy link
Member

martint commented Apr 2, 2023

Adding an exclusion is brittle. We need to remember to remove it when the library is updated, it adds more cruft to the pom, etc.

If it’s a problem, it’d be better to get it updated upstream.

@wendigo
Copy link
Contributor Author

wendigo commented Apr 2, 2023

@martint This is harder to do and will take a lot more time and resources

@kokosing
Copy link
Member

kokosing commented Apr 2, 2023

Adding an exclusion is brittle

It is is. The library can be added again as transitive dependency for something else.

Let's use something like https://maven.apache.org/enforcer/enforcer-rules/bannedDependencies.html. Let's add it first here but then let's add it to airbase.

@wendigo
Copy link
Contributor Author

wendigo commented Apr 2, 2023

@kokosing I don't know if we can do that. Tempto and benchto are still dependant on snakeyaml. I've updated it in https://github.com/trinodb/tempto/pull/109/files, but in benchto it's harder and will take some more time.

@kokosing
Copy link
Member

kokosing commented Apr 2, 2023

We can do it as follow up.

@wendigo wendigo requested a review from kokosing April 3, 2023 14:32
testing/trino-product-tests-launcher/pom.xml Outdated Show resolved Hide resolved
@wendigo wendigo force-pushed the serafin/trino-pinot-snakeyaml branch 2 times, most recently from 721b99e to 21444a6 Compare April 4, 2023 13:47
wendigo added 2 commits April 6, 2023 11:07
It's used transitively by calcite to load model definitions
Allow it only temporary for Tempto and benchto usages
@wendigo wendigo force-pushed the serafin/trino-pinot-snakeyaml branch from 0f35b7e to 91f5ffd Compare April 6, 2023 09:07
@hashhar
Copy link
Member

hashhar commented Apr 7, 2023

maven-checks timeout is known issue - see #16863

re-ran anyway to make sure compile works

@wendigo
Copy link
Contributor Author

wendigo commented Apr 7, 2023

@hashhar cancelled :(

@hashhar hashhar merged commit 54a0b0b into trinodb:master Apr 7, 2023
@hashhar hashhar added the no-release-notes This pull request does not require release notes entry label Apr 7, 2023
@github-actions github-actions bot added this to the 413 milestone Apr 7, 2023
@wendigo wendigo deleted the serafin/trino-pinot-snakeyaml branch April 11, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

4 participants