You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Kirk Lund <kl...@apache.org> on 2018/12/21 22:42:23 UTC
Re: [PROPOSAL] use default value for validate-serializable-objects in dunit
<bump>
I filed GEODE-6202: DUnit should not enable VALIDATE_SERIALIZABLE_OBJECTS
by default
https://issues.apache.org/jira/browse/GEODE-6202
And submitted PR #3023
https://github.com/apache/geode/pull/3023
Please review and/or discuss further if needed.
Thanks,
Kirk
On Thu, Mar 15, 2018 at 12:00 PM Bruce Schuchardt <bs...@pivotal.io>
wrote:
> +0.5 I think we can turn this off (back to the default) now since the
> AnalyzeSerializables tests for other modules have been created. These
> tests ensure that serializable objects are properly white-listed or
> excluded and are able to be serialized/deserialized.
>
> Excluded classes are not tested, though, so if you add an excluded class
> and are wrong about whether it is sent over the wire you will break
> Geode. Having serialization validation enabled in dunit tests protects
> against this if you write tests that use the excluded class.
>
> We also have non-default values for cluster configuration in dunit runs.
>
>
> On 3/13/18 10:03 AM, Kirk Lund wrote:
> > I want to propose using the default value for
> validate-serializable-object
> > in dunit tests instead of forcing it on for all dunit tests. I'm
> > sympathetic to the reason why this was done: ensure that all existing
> code
> > and future code will function properly with this feature enabled.
> > Unfortunately running all dunit tests with it turned on is not a good way
> > to achieve this.
> >
> > Here are my reasons:
> >
> > 1) All tests should start out with the same defaults that Users have. If
> we
> > don't do this, we are going to goof up sometime and release something
> that
> > only works with this feature turned on or worsen the User experience of
> > Geode in some way.
> >
> > 2) All tests should have sovereign control over their own configuration.
> We
> > should strive towards being able to look at a test and determine its
> config
> > at a glance without having to dig through the framework or other classes
> > for hidden configuration. We continue to improve dunit in this area but I
> > think adding to the problem is going in the wrong direction.
> >
> > 3) It sets a bad precedent. Do we follow this approach once or keep
> adding
> > additional non-default features when we need to test them too? Next one
> is
> > GEODE-4769 "Serialize region entry before putting in local cache" which
> > will be disabled by default in the next Geode release and yet by turning
> it
> > on by default for all of precheckin we were able to find lots of problems
> > in both the product code and test code.
> >
> > 4) This is already starting to cause confusion for developers thinking
> its
> > actually a product default or expecting it to be enabled in other
> > (non-dunit) tests.
> >
> > Alternatives for test coverage:
> >
> > There really are no reasonable shortcuts for end-to-end test coverage of
> > any feature. We need to write new tests or identify existing tests to
> > subclass with the feature enabled.
> >
> > 1) Subclass specific tests to turn validate-serializable-object on for
> that
> > test case. Examples of this include a) dunit tests that execute Region
> > tests with OffHeap enabled, b) dunit tests that flip on HTTP over GFSH,
> c)
> > dunit tests that run with SSL or additional security enabled.
> >
> > 2) Write new tests that cover all features with
> validate-serializable-object
> > enabled.
> >
> > 3) Rerun all of dunit with and without the option. This doesn't sound
> very
> > reasonable to me, but it's the closest to what we really want or need.
> >
> > Any other ideas or suggestions other than forcing all dunit tests to run
> > with a non-default value?
> >
> > Thanks,
> > Kirk
> >
>
>
Re: [PROPOSAL] use default value for validate-serializable-objects in
dunit
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-1
I'm fearful that removing full testing of serialization validation
leaves the project exposed to java-serialization bombs. We need to
ensure that our servers and clients are protected from malicious
deserialization attacks.
On 2018/12/21 22:42:23, Kirk Lund <k....@apache.org> wrote:
> <bump>>
>
> I filed GEODE-6202: DUnit should not enable
VALIDATE_SERIALIZABLE_OBJECTS>
> by default>
> https://issues.apache.org/jira/browse/GEODE-6202>
>
> And submitted PR #3023>
> https://github.com/apache/geode/pull/3023>
>
> Please review and/or discuss further if needed.>
>
> Thanks,>
> Kirk>
>
> On Thu, Mar 15, 2018 at 12:00 PM Bruce Schuchardt <bs...@pivotal.io>>
> wrote:>
>
> > +0.5 I think we can turn this off (back to the default) now since the>
> > AnalyzeSerializables tests for other modules have been created. These>
> > tests ensure that serializable objects are properly white-listed or>
> > excluded and are able to be serialized/deserialized.>
> >>
> > Excluded classes are not tested, though, so if you add an excluded
class>
> > and are wrong about whether it is sent over the wire you will break>
> > Geode. Having serialization validation enabled in dunit tests
protects>
> > against this if you write tests that use the excluded class.>
> >>
> > We also have non-default values for cluster configuration in dunit
runs.>
> >>
> >>
> > On 3/13/18 10:03 AM, Kirk Lund wrote:>
> > > I want to propose using the default value for>
> > validate-serializable-object>
> > > in dunit tests instead of forcing it on for all dunit tests. I'm>
> > > sympathetic to the reason why this was done: ensure that all
existing>
> > code>
> > > and future code will function properly with this feature enabled.>
> > > Unfortunately running all dunit tests with it turned on is not a
good way>
> > > to achieve this.>
> > >>
> > > Here are my reasons:>
> > >>
> > > 1) All tests should start out with the same defaults that Users
have. If>
> > we>
> > > don't do this, we are going to goof up sometime and release
something>
> > that>
> > > only works with this feature turned on or worsen the User
experience of>
> > > Geode in some way.>
> > >>
> > > 2) All tests should have sovereign control over their own
configuration.>
> > We>
> > > should strive towards being able to look at a test and determine
its>
> > config>
> > > at a glance without having to dig through the framework or other
classes>
> > > for hidden configuration. We continue to improve dunit in this
area but I>
> > > think adding to the problem is going in the wrong direction.>
> > >>
> > > 3) It sets a bad precedent. Do we follow this approach once or keep>
> > adding>
> > > additional non-default features when we need to test them too?
Next one>
> > is>
> > > GEODE-4769 "Serialize region entry before putting in local cache"
which>
> > > will be disabled by default in the next Geode release and yet by
turning>
> > it>
> > > on by default for all of precheckin we were able to find lots of
problems>
> > > in both the product code and test code.>
> > >>
> > > 4) This is already starting to cause confusion for developers
thinking>
> > its>
> > > actually a product default or expecting it to be enabled in other>
> > > (non-dunit) tests.>
> > >>
> > > Alternatives for test coverage:>
> > >>
> > > There really are no reasonable shortcuts for end-to-end test
coverage of>
> > > any feature. We need to write new tests or identify existing
tests to>
> > > subclass with the feature enabled.>
> > >>
> > > 1) Subclass specific tests to turn validate-serializable-object
on for>
> > that>
> > > test case. Examples of this include a) dunit tests that execute
Region>
> > > tests with OffHeap enabled, b) dunit tests that flip on HTTP over
GFSH,>
> > c)>
> > > dunit tests that run with SSL or additional security enabled.>
> > >>
> > > 2) Write new tests that cover all features with>
> > validate-serializable-object>
> > > enabled.>
> > >>
> > > 3) Rerun all of dunit with and without the option. This doesn't
sound>
> > very>
> > > reasonable to me, but it's the closest to what we really want or
need.>
> > >>
> > > Any other ideas or suggestions other than forcing all dunit tests
to run>
> > > with a non-default value?>
> > >>
> > > Thanks,>
> > > Kirk>
> > >>
> >>
> >>
>