-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-19315. Bumped avro to 1.11.4 and implemented new setter/getter adjustments #7128
base: trunk
Are you sure you want to change the base?
Conversation
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. |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above
Added new changes - the two serialization tests are successful locally. |
💔 -1 overall
This message was automatically generated. |
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-binary
files?