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

Remove Phoenix5 connector #20739

Closed
wants to merge 1 commit into from
Closed

Remove Phoenix5 connector #20739

wants to merge 1 commit into from

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Feb 18, 2024

Shaded Phoenix 5.1.3 client brings over 75+ Critical and High CVEs into the Trino codebase (90% of the CVE count for the entire codebase consisting of 800+ dependencies).

Neither 5.1.4-SNAPSHOT nor 5.2.0-SNAPSHOT versions bring any significant improvement to this situation.

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is 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:

# Phoenix5 connector
* Drop support for Phoenix5 connector. ({issue}`issuenumber`)

@wendigo
Copy link
Contributor Author

wendigo commented Feb 19, 2024

Here's the list of all Phoenix 5 CVEs:

_Users_mateuszgajewski_Projects_src_github com_trinodb_trino_grype3 html

Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Mar 11, 2024
@mosabua
Copy link
Member

mosabua commented Mar 19, 2024

Has the upstream project been informed about this mess? I dont think we should remove the connector? Anybody concerned about the binary files can just delete the plugin directory. But we should try to get this improved by the Phoenix project and maybe we can avoid using a shaded jar. Do we have any contacts to talk to ?

@wendigo
Copy link
Contributor Author

wendigo commented Mar 19, 2024

@mosabua it was on the mailing list. The answer was that this won't be improved upon.

@mosabua
Copy link
Member

mosabua commented Mar 19, 2024

@mosabua it was on the mailing list. The answer was that this won't be improved upon.

Jeez .. great. So is the project dead?

Can we somehow remove a lot of that shaded stuff?

@wendigo
Copy link
Contributor Author

wendigo commented Mar 19, 2024

@mosabua major effort with a little benefit

@mosabua
Copy link
Member

mosabua commented Mar 19, 2024

Fair .. maybe we just let it linger for now

@mosabua
Copy link
Member

mosabua commented Mar 19, 2024

We could ask in the community about usage and willingness to help .. or replace it with a hbase connector instead of just removing it

@electrum
Copy link
Member

Could we switch the connector to use https://phoenix.apache.org/server.html

@wendigo
Copy link
Contributor Author

wendigo commented Mar 19, 2024

@electrum maybe yes but I don't have neither knowledge nor an experience to do so

@github-actions github-actions bot removed the stale label Mar 20, 2024
@findepi
Copy link
Member

findepi commented Mar 26, 2024

By looking at #21251, i suspect not many people are using the connector

@findepi
Copy link
Member

findepi commented Mar 26, 2024

cc @chenjian2664 @lhofhansl

@chenjian2664
Copy link
Contributor

My former experience is that no longer uses this connector in production, one of the major reason is that phoenix services lack effective maintenance, but the test environment still has this connector. @willzgw

@bitsondatadev
Copy link
Member

My 2 cents is that this connector has little benefit being maintained by Trino at this point. Any technology on its way out will always have some users somewhere and that shouldn't determine if Trino will spend the communities resources on maintaining Phoenix or HBase.

That doesn't mean they cease to exist and anyone using this is screwed, it just means someone currently using it will take ownership of patches etc...

We definitely should bring this up in the next contributor call, but with all the other valuable ways maintainers could be spending their time, I think it's kinder to everyone to know when to cut ties.

It also hurts folks who are writing these PRs. They have some expectation and hope that they won't have to maintain this connector. If Trino isn't supporting this then it's a clear signal to the engineer and their company that to stay on Phoenix they must accept the burden to maintain it.

Better we rip off the bandaid than to let the unhealthy relationship drag on.

Sorry Phoenix, it's not you, it's your CVEs and lackluster maintenance. 👋

@wendigo wendigo force-pushed the serafin/drop-phoenix5 branch from 7f0beb5 to 4f2e2e7 Compare March 27, 2024 09:32
@wendigo
Copy link
Contributor Author

wendigo commented Mar 27, 2024

@mosabua can you send an invitation to the phoenix maintainers whether they would like to join our contributors call in 2 weeks?

@wendigo wendigo force-pushed the serafin/drop-phoenix5 branch from 4f2e2e7 to 59f87ab Compare March 27, 2024 13:08
@mosabua
Copy link
Member

mosabua commented Mar 27, 2024

Will do.

@mosabua
Copy link
Member

mosabua commented Mar 27, 2024

Posted on Phoenix channel in ASF slack. if necessary I will post on their dev mailing list as well

@wendigo
Copy link
Contributor Author

wendigo commented Mar 27, 2024

@mosabua you know... places 😃

@virajjasani
Copy link

FYI, as the majority of the CVEs are coming from Hadoop, we have recently bumped the version to 3.2.4: apache/phoenix@e988b64

Moreover, Hadoop has released 3.4.0 recently, but it might require a bit more time before both HBase and Phoenix can use the upgraded version.

@mosabua
Copy link
Member

mosabua commented Mar 28, 2024

Here is a suggestions from Istvan Toth @stoty on ASF slack:

I would strongly suggest moving to the new phoenix-mapreduce-byo-hbase artifact introduced in 5.2.
That one does not shade in ANY part of HBase or Hadoop.
It was mentioned earlier that Trino already maintains a patched Hadoop distribution.
Using that one, plus hbase-shaded-mapreduce + phoenix-mapreduce-byo-hbase would let Trino handle any Hadoop specific CVEs, and update the HBase and Phoenix versions independently (within compatability constraints).
As an added bonus, this would save ~80MB in the distribution, and would avid any shading classpath conflicts that using the old phoenix-client may cause.

@wendigo
Copy link
Contributor Author

wendigo commented Mar 28, 2024

@mosabua 5.2 is not yet released so there is nothing to update to. It's been 15 months since the last Phoenix release

@mosabua
Copy link
Member

mosabua commented Mar 28, 2024

The other option I think we might want to consider is keeping the plugin in the codebase and maintaining it going forward and updating as much as we can as well as removing and shaded dependencies that we can get rid of. And in addition .. no longer include the plugin in the default built tarball, rpm and docker container. We can update the docs to tell users to download the plugin from Maven Central separatly and copy it into the plugins folder on their install or create a custom tarball/container/rpm with it. Not as user friendly but avoids the security hit for the majority of users while still providing a pathway for Phoenix users.

Of course it would be really good if Phoenix could upgrade more and faster too.

@wendigo wendigo force-pushed the serafin/drop-phoenix5 branch from 59f87ab to 850f793 Compare March 28, 2024 16:00
@mosabua
Copy link
Member

mosabua commented Mar 28, 2024

@mosabua 5.2 is not yet released so there is nothing to update to. It's been 15 months since the last Phoenix release

I know.. supposedly coming soon though

@wendigo
Copy link
Contributor Author

wendigo commented Mar 28, 2024

@mosabua switching to some new library is a work that needs to be done. I'm not sure how complex the switch will be and who can do this.

@mosabua
Copy link
Member

mosabua commented Mar 28, 2024

@mosabua switching to some new library is a work that needs to be done. I'm not sure how complex the switch will be and who can do this.

I think it would not work anyway since we dont have a hadoop fork to drop into it.

@findepi findepi changed the title Remove Phoenix5 connector support Remove Phoenix5 connector Mar 28, 2024
@stoty
Copy link

stoty commented Mar 29, 2024

FYI using the new Phoenix artifact does not require any code changes, it only requires changing the dependencies / JARs included.
Without checking what APIs Trino uses I cannot confirm that updating to 5.2 would require no code changes, but 5.2 is is meant to be backwards compatible to 5.1.

As for the schedule the 5.2.0 release is process is already underway, and the next RC is expected next week.

@kokosing
Copy link
Member

@stoty any update about the release?

@wendigo
Copy link
Contributor Author

wendigo commented Apr 15, 2024

@kokosing I'd like to proceed with dropping Phoenix connector from Trino. We can move it to a separate repository

Shaded Phoenix 5.1.3 client brings over 60 Critical and High CVEs into the Trino codebase.

5.1.4-SNAPSHOT version doesn't bring any significant improvement to this situation.
@wendigo wendigo force-pushed the serafin/drop-phoenix5 branch from 850f793 to 7c19f43 Compare April 15, 2024 14:18
@stoty
Copy link

stoty commented Apr 15, 2024

The vote is open for the 5.2.0 release @kokosing.
Expected to release this week.

@wendigo
Copy link
Contributor Author

wendigo commented Apr 15, 2024

@stoty is it GA release or RC?

@stoty
Copy link

stoty commented Apr 15, 2024

It's the GA release vote for the hopefully final RC.

@stoty
Copy link

stoty commented Apr 15, 2024

i..e The current RC will be the GA release when the vote passes.

@virajjasani
Copy link

The vote should be completed in a day or two.

@virajjasani
Copy link

5.2.0 is released, I am performing a few additional completion steps but the artifacts are already released:

@wendigo wendigo closed this Apr 23, 2024
@electrum electrum deleted the serafin/drop-phoenix5 branch April 27, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

9 participants