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

various vulnerability fixes #8950

Conversation

vilay-nference
Copy link

No description provided.

Dockerfile.nfer Outdated
@@ -0,0 +1,40 @@
# Use the specified base image
FROM hz-registry.nferx.com/ubuntu as builder
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vilay-nference Where is the dockerfile for this image? It is not immediately accessible, so we cannot verify its contents.

In general, we use base images provided directly from first-party sources (e.g. https://hub.docker.com/_/ubuntu).

Please change this to use a standard base image and then we can re-evaluate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used for internal purpose. I'll delete this file .. not required for gatk

@@ -232,6 +239,12 @@ configurations {
}

dependencies {
implementation('net.minidev:json-smart:2.4.9') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what these new dependencies are?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used as transitive dependencies in one of the package. Overriding the newer version for the compatibles.

Dockerfile.nfer Outdated
@@ -0,0 +1,40 @@
# Use the specified base image
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this third-party Dockerfile

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. will remove this and update you

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is deleted

exclude group: 'net.minidev', module: 'json-smart'
}
// Example dependencies
implementation 'biz.aQute.bnd:biz.aQute.bndlib:5.1.2'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add new GATK dependencies here unless absolutely necessary

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again dnsjava is used from hadoop client.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@droazen . Let us know if any more modifications required

@lbergelson
Copy link
Member

@vilay-nference Thank you for doing this work. It's nitpicky annoying stuff to figure out.

I have one additional request. Instead of addding additional direct implementation dependencies, could we specify the transtive version requirements in a gradle constraints block?

That will:

  1. make it clear that we don't rely on these directly
  2. prevent us from keeping them around if we do something like remove hadoop in the future
  3. lets us rewrite those force blocks to instead define minimum versions so if the libraries move forward in the future we're not accidentally holding on a to an old version

@lbergelson
Copy link
Member

@vilay-nference Were you able to get this configuration to pass tests on your end? I've attempted to incorporate your changes into #8998, but I'm running into issues with hadoop and protobuf incompatibilities. I see the same problem with your branch when I try to run tests on it. (I also can't run tests without disabling -Werror on your branch since there are still some unresolved deprecation and other minor issues).

Errors look like this:

Caused by: java.lang.ExceptionInInitializerError: Exception java.lang.NoClassDefFoundError: Could not initialize class org.apache.hadoop.security.proto.SecurityProtos [in thread "IPC Server handler 1 on default port 64812"]
	at org.apache.hadoop.hdfs.protocol.proto.ClientNamenodeProtocolProtos.<clinit>(ClientNamenodeProtocolProtos.java)

and you can easily trigger one by running ParallelCopyGCSDirectoryIntoHDFSSparkIntegrationTest

lbergelson added a commit that referenced this pull request Oct 18, 2024
* Update depenencies to fix vulnerabilities as reported in #8950
* Update our dependency management to make use of some newish gradle features
  * Add dependency constraints to update transitive dependencies, this allows us to specify versions without making them
    direct dependencies
  * Remove most force expressions and replace them where necessary with version strict requirements
  * Make use of several published bom's to configure consistent dependency versions for platforms like netty and log4j2
  * Remove exclude statements that are now handled by variant dependency resolution (like guava android vs jdk)
* Exclude the org.bouncycastle:bcprov-jdk15on dependency and replace it with bcprov-jdk18onA
  This adds an unecessary implementation level dependency on what is really a transitive, but I couldn't get gradles explicit
  replacement logic to work so this is a workaround
lbergelson added a commit that referenced this pull request Oct 18, 2024
* Update depenencies to fix vulnerabilities as reported in #8950
* Update our dependency management to make use of some newish gradle features
  * Add dependency constraints to update transitive dependencies, this allows us to specify versions without making them
    direct dependencies
  * Remove most force expressions and replace them where necessary with version strict requirements
  * Make use of several published bom's to configure consistent dependency versions for platforms like netty and log4j2
  * Remove exclude statements that are now handled by variant dependency resolution (like guava android vs jdk)
* Exclude the org.bouncycastle:bcprov-jdk15on dependency and replace it with bcprov-jdk18onA
  This adds an unecessary implementation level dependency on what is really a transitive, but I couldn't get gradles explicit
  replacement logic to work so this is a workaround
lbergelson added a commit that referenced this pull request Oct 23, 2024
* Updating dependency management and vulnerable dependencies

* Update dependencies to fix vulnerabilities as reported in #8950
* Update our dependency management to make use of some newish gradle features
  * Add dependency constraints to update transitive dependencies, this allows us to specify versions without making them
    direct dependencies
  * Remove most force expressions and replace them where necessary with version strict requirements
  * Make use of several published bom's to configure consistent dependency versions for platforms like netty and log4j2
  * Remove exclude statements that are now handled by variant dependency resolution (like guava android vs jdk)
* Exclude the org.bouncycastle:bcprov-jdk15on dependency and replace it with bcprov-jdk18onA
  This adds an unnecessary testUtilImplementation level dependency on what is really a transitive, but I couldn't get gradle's explicit version replacement to work.
  replacement logic to work so this is a workaround
@lbergelson
Copy link
Member

I've incorporated the the changes from here into #9006 which is now merged. Thank you @vilay-nference

@lbergelson lbergelson closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants