Hi Sergey, Thanks for sharing your thoughts. It could somehow help but didn't get to the root of this issue.
According to the documentation, Flink shaded is used to provide a single instance of a shaded dependency across sub-modules in Flink repo. Shaded namespaces should be used where shaded dependencies are configured. After connectors have been externalized, it ends up with more repos depending on one shaded jar, e.g. guava. This is a "monolithic" dependency setup that makes it difficult to change the root(flink-shade), because any changes of the root have to be propagated to all downstream repos. Even worse is that not every downstream repo is known while modifying the root. Since all externalized connectors have their own repos and are not sub-modules of Flink anymore, I would suggest the following upgrade: 1. Connectors should use their own classloader instead of Flink's classloader. This will break the monolithic dependency. Connectors and Flink can use different versions of flink-shaded. 2. [optional] It would be even better that all connector repos depend on their own individual shaded repo, e.g. flink-connector-shaded. flink-shaded should only be used by Flink. WDYT? Best regards, Jing On Thu, Sep 14, 2023 at 11:28 PM Sergey Nuyanzin <snuyan...@gmail.com> wrote: > Yes, that's a reasonable question, thanks for raising it. > > I think this is not only about flink-shaded, rather about dependencies in > general > > I guess there is no rule of thumb, or at least I'm not aware of > Here are my thoughts > 1. If bumping dependency doesn't require breaking changes and passes > existing tests then just bump it > 2. In case there are breaking changes we could consider doing this within > next major release > for minor release > a. try to answer a question whether it impacts Flink or not > b. in case it impacts Flink and fix itself is relatively small then to > avoid breaking change > we could copy classes with solutions to Flink repo like it usually > happens with Calcite related fixes. > The problem of this approach is that I guess it will not work for > non jvm deps like e.g. RocksDB > c. In case no way to do it without breaking changes for minor release > then probably need sort of announcement motivating to move to another major > version where the issue is fixed > > looking forward to seeing other opinions about that > > On Wed, Sep 13, 2023 at 9:47 PM Jing Ge <j...@ververica.com.invalid> > wrote: > > > Hi Sergey, > > > > Thanks for doing the analysis and providing the great insight. I did my > own > > analysis and got the same conclusion. I just wanted to use this example > to > > kick off a discussion and check if there is a common guideline or concept > > in the community to handle such cases, since it seems any bump-up might > > have a big impact. This time, we are kind of lucky. What if CVE related > > code has been used in Flink? I do see it is an issue that upgrading a lib > > in the flink-shaded repo is not recommended because of the complexity. > > IMHO, we don't want to put the cart before the horse. > > > > Best regards, > > Jing > > > > On Wed, Sep 13, 2023 at 9:11 PM Sergey Nuyanzin <snuyan...@gmail.com> > > wrote: > > > > > Thanks for raising this > > > > > > I would suggest trying to double check whether it actually impacts > Flink > > or > > > not. > > > > > > For instance from one side Calcite between 1.22.0..1.31.0 has a > critical > > > CVE-2022-39135 [1] > > > from another side Flink does not use this functionality and is not > > impacted > > > by this. > > > > > > Regarding guava > > > After closer look at Guava's high CVE you've mentioned [2] > > > Based on Github issue describing the problem (exists since 2016) [3] > > > there is a security issue with class > > > com.google.common.io.FileBackedOutputStream > > > > > > While looking at source code for > > > com.google.common.io.FileBackedOutputStream usage I was not able to > find > > > such. > > > Also I was not able to find usage of > > > org.apache.flink.shaded.guava30.com.google.common.io > .Files#createTempDir > > > which was fixed within commit [4] > > > Also I was not able to find other Guava classes which use the problem > > one. > > > Currently I tend to think that it probably does not impact Flink as > well. > > > > > > Please correct me if I'm wrong > > > > > > [1] https://nvd.nist.gov/vuln/detail/CVE-2022-39135 > > > [2] https://nvd.nist.gov/vuln/detail/CVE-2023-2976 > > > [3] https://github.com/google/guava/issues/2575 > > > [4] > > > > > > > > > https://github.com/google/guava/commit/b3719763b9ca777b94554d7ea2d2b92e27ac6164 > > > > > > On Wed, Sep 13, 2023 at 6:26 PM Jing Ge <j...@ververica.com.invalid> > > > wrote: > > > > > > > Hi Sergey, Hi Martijn, > > > > > > > > Thanks for the information and suggestions. It is rational. > > > > > > > > Since on one hand, we should not do the upgrade according to all your > > > > thoughts, and on the other hand, Guava 30.1.1-jre has known CVEs[1] > and > > > the > > > > next closest version that has no CVEs is 32.0.1-jre. Without the > > upgrade, > > > > users might not be able to use Flink 1.17 in their production and > Flink > > > > 1.18 has not been released yet. Do we have any known concept, rule, > or > > > > process in the community to handle such a common CVE issue? > > > > > > > > Best regards, > > > > Jing > > > > > > > > > > > > > > > > [1] > > https://mvnrepository.com/artifact/com.google.guava/guava/30.1.1-jre > > > > > > > > On Wed, Sep 13, 2023 at 4:31 PM Martijn Visser < > > martijnvis...@apache.org > > > > > > > > wrote: > > > > > > > > > Hi Jing, > > > > > > > > > > Like Sergey said, making that change would break all connectors > that > > > used > > > > > Flink Shaded in the released versions, given that Guava is part of > > the > > > > > package name. I think that was the case for Kafka, Pulsar, JDBC, > > > PubSub, > > > > > HBase, Cassandra and maybe even more. We shouldn't do this upgrade. > > > > > > > > > > Best regards, > > > > > > > > > > Martijn > > > > > > > > > > On Wed, Sep 13, 2023 at 2:54 PM Sergey Nuyanzin < > snuyan...@gmail.com > > > > > > > > wrote: > > > > > > > > > > > Hi Jing, > > > > > > > > > > > > If I remember correctly, bumping of guava to another major > version > > > > > usually > > > > > > leads to package rename > > > > > > since the major version number is a part of the package name... > > > > > > Not 100% sure however this could be the reason for some potential > > > > > breaking > > > > > > changes (we also faced that with connectors while the last > > > flink-shaded > > > > > > update) > > > > > > > > > > > > On Wed, Sep 13, 2023 at 2:43 PM Jing Ge > <j...@ververica.com.invalid > > > > > > > > > wrote: > > > > > > > > > > > > > Hi Martijn, > > > > > > > > > > > > > > For example, bump up guava from 30.1.1-jre to 32.1.2-jre. > > > > > > > > > > > > > > Best regards, > > > > > > > Jing > > > > > > > > > > > > > > On Wed, Sep 13, 2023 at 2:37 PM Martijn Visser < > > > > > martijnvis...@apache.org > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Jing, > > > > > > > > > > > > > > > > A PR to update the readme sounds like a good plan. > > > > > > > > > > > > > > > > I think it depends on what are the expected updates for a > > > > > flink-shaded > > > > > > > > 16.2, since that version doesn't exist. > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > > > > > Martijn > > > > > > > > > > > > > > > > On Wed, Sep 13, 2023 at 1:48 PM Jing Ge > > > <j...@ververica.com.invalid > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Martijn, > > > > > > > > > > > > > > > > > > Thanks for your reply with details. Appreciate it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Flink-Shaded is usually only updated whenever a > > > > > > > > > > new Flink minor version is released and only at the > > beginning > > > > of > > > > > > the > > > > > > > > > > release cycle, so that there's enough time to stabilize > > > Flink. > > > > > > > > > > > > > > > > > > > > > > > > > > > This is the information I am looking for. It will help devs > > > > > > understand > > > > > > > > the > > > > > > > > > compatibility between different versions of flink and > > > > flink-shaded, > > > > > > if > > > > > > > it > > > > > > > > > could be described in the readme. If you don't mind, I can > > > > create a > > > > > > pr > > > > > > > > and > > > > > > > > > update it. > > > > > > > > > > > > > > > > > > Speaking of this rule, I have a follow-up question: do you > > have > > > > any > > > > > > > > concern > > > > > > > > > if flink-shaded 16.2 will be released and upgraded(from > 16.1 > > to > > > > > 16.2) > > > > > > > in > > > > > > > > > Flink 1.17? Is there anything we should pay attention to > > while > > > > > > > releasing > > > > > > > > a > > > > > > > > > new minor flink-shaded version? > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > Jing > > > > > > > > > > > > > > > > > > On Wed, Sep 13, 2023 at 9:01 AM Martijn Visser < > > > > > > > martijnvis...@apache.org > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Jing, > > > > > > > > > > > > > > > > > > > > Flink Shaded exists so that Flinks internal usage of > > commonly > > > > > used > > > > > > > > > packages > > > > > > > > > > such as Guava, Jackson inside of Flink don't clash with > > > > different > > > > > > > > > versions > > > > > > > > > > that users might use when creating a Flink application. > > When > > > I > > > > > did > > > > > > > the > > > > > > > > > > upgrade of Flink Shaded, we already ran into a bunch of > > > > problems > > > > > > > > because > > > > > > > > > a > > > > > > > > > > lot of the externalized connectors relied on Flink > Shaded, > > > > which > > > > > > made > > > > > > > > it > > > > > > > > > > problematic to get the connector to work on both Flink > 1.17 > > > and > > > > > > Flink > > > > > > > > > 1.18. > > > > > > > > > > There's been quite a lot of effort put into making sure > > that > > > > > > > > externalized > > > > > > > > > > connectors don't rely on Flink Shaded at all anymore, by > > > either > > > > > > using > > > > > > > > > their > > > > > > > > > > own versions of shaded artifacts (which was the case with > > the > > > > > > Pulsar > > > > > > > > > > connector) or just removing the dependency on Flink > Shaded > > > all > > > > > > > > together, > > > > > > > > > by > > > > > > > > > > using regular Java. > > > > > > > > > > > > > > > > > > > > If you would upgrade flink-shaded from 16.1 to 17.0 in > > Flink > > > > > 1.17, > > > > > > > you > > > > > > > > > > would break all externalized connectors that rely on > Flink > > > > > Shaded's > > > > > > > > Guava > > > > > > > > > > version, plus you potentially would impact the runtime > > given > > > > that > > > > > > > > > there's a > > > > > > > > > > newer Netty version etc. Flink-Shaded is usually only > > updated > > > > > > > whenever > > > > > > > > a > > > > > > > > > > > > > > > > > > new Flink minor version is released and only at the > beginning > > > of > > > > > the > > > > > > > > > > release cycle, so that there's enough time to stabilize > > > Flink. > > > > > > > > > > > > > > > > > > All in all, we shouldn't upgrade flink-shaded in Flink > 1.17. > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > > > > > > > > > Martijn > > > > > > > > > > > > > > > > > > > > On Tue, Sep 12, 2023 at 7:26 PM Jing Ge > > > > > <j...@ververica.com.invalid > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Dev, > > > > > > > > > > > > > > > > > > > > > > Currently Flink 1.17 is using flink-shaded 16.1 and > Flink > > > > 1.18 > > > > > is > > > > > > > > using > > > > > > > > > > > flink-shaded 17.0. Do we need to consider any > > compatibility > > > > > rules > > > > > > > > > between > > > > > > > > > > > them? E.g. is there any concern to upgrade flink-shaded > > > from > > > > > 16.1 > > > > > > > to > > > > > > > > > 17.x > > > > > > > > > > > for Flink 1.17? Or there are some implicit dependency > > rules > > > > > > between > > > > > > > > > > > them. Looking > > > > > > > > > > > forward to hearing from you. > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > Jing > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Best regards, > > > > > > Sergey > > > > > > > > > > > > > > > > > > > > > > > > -- > > > Best regards, > > > Sergey > > > > > > > > -- > Best regards, > Sergey >
