HADOOP-19315. Bumped avro to 1.11.4 and implemented new setter/getter adjustments #7128
+63
−27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I was confused looking at this as the classes were already using the setters; turns out I was on an internal version where somebody already did something similar. But their change actually modified more files than this. Is it a complete set? Also: check that LICENSE-binary file. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Avro test failures. looks like there's now a sysprop which needs to be set. There's some implications there for testing sysprop needs to be set in execution or test setup. More importantly: mapreduce uses avro files in production. All uses of that code needs to be reviewed to make sure it isn't using standard object ser/deser as otherwise every runtime deployment of the services will need to set up a new sysprop -which is going to be an incompatible change |
|
...looking at our internal changes; some files in rumen module needed fixing too. nothing significant |
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran Can you please specify what the ToDo is here on my side? Seems like this sysprop needs to be set either way right? What do you mean by checking the standard serializer methods in mapreduce? I'm missing the full picture here. |
|
@steveloughran I can see why Avro need this packages property but the implementation makes things difficult for Hadoop. One option would be for the Hadoop code to set the property itself. It could reset the property, taking the existing value and appending the necessary Hadoop packages. If the property is empty, we would need to add the default setting too if we were to add some of our own. The default applied is: |
|
@dom93dd two issues
@pjfanning turns out the issue is AVRO-3985. It's not a recently discovered CVE as such, just that they've added the ability to lock down which packages are allowed to contain classes instantiated through reflection: this new property is the core aspect of the feature. |
|
For the test classes it is an easy fix. Just added a change. Regarding the productive code, as @pjfanning already started a discussion about this property, we should have a common agreement where this property should be set. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Oct 25, 2024
| @@ -404,6 +404,8 @@ public void testGetName() { | |||
|
|
|||
| @Test (timeout = 30000) | |||
| public void testAvroReflect() throws Exception { | |||
| // Avro expects explicitely stated, trusted packages used for (de-)serialization | |||
| System.setProperty("org.apache.avro.SERIALIZABLE_PACKAGES", "org.apache.hadoop.fs.Path"); | |||
There was a problem hiding this comment.
make the property name a constant somewhere in hadoop-common tests and refer to it in both places
There was a problem hiding this comment.
the property is for package names as opposed to class names - should really have value like "org.apache.hadoop.fs"
There was a problem hiding this comment.
Yup you're right, just checked the avro implementation how they check the package names.
| @@ -344,6 +344,8 @@ public void testConcurrentEncodeDecode() throws Exception{ | |||
|
|
|||
| @Test | |||
| public void testAvroReflect() throws Exception { | |||
| // Avro expects explicitely stated, trusted packages used for (de-)serialization | |||
| System.setProperty("org.apache.avro.SERIALIZABLE_PACKAGES", "org.apache.hadoop.io.Text"); | |||
|
Added new changes - the two serialization tests are successful locally. |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Bumped avro version from 1.9.2 to 1.11.4 and adjusted some parts where still member variables instead of getter/setter methods where used (avro made member variables private).
How was this patch tested?
run mvn clean install javadoc:javadoc checkstyle:checkstyle
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?