accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From William Slacum <wilhelm.von.cl...@accumulo.net>
Subject Re: ACCUMULO-958 - Pluggable encryption in walogs
Date Wed, 30 Jan 2013 19:02:22 GMT
Adam, I think you need to invert your premise. You need to consider the
benefit to the community of some piece of work before committing back to
the community. A plug-in framework has no value if there are no plug-ins.
Adding in untested, unreviewed layers of indirection for reading and
writing data is a bad idea, plain and simple. Furthermore, you cannot say
you are avoiding forking by supplying the patch and then openly state you
are witholding portions that make said patch useful.

On Wed, Jan 30, 2013 at 1:45 PM, Adam Fuchs <scubafuchs@gmail.com> wrote:

> Bill,
>
> The release date was not pushed back just for this ticket -- there were
> several other changes that motivated that date change. We can discuss that
> aspect separately from a discussion of ACCUMULO-958, and we need to start a
> separate thread to talk about the remaining milestones before the 1.5.0
> release.
>
> I would also like to amend your statement to be "... the patch has no value
> added [for] general users [without the addition of extensions that are not
> included with the patch]." This is a more accurate yet much weaker premise,
> and you should consider the implications on the broader ecosystem.
>
> It seems to me that the main points against this patch are that it is
> imperfect. I don't think that feature freeze is the time at which we should
> demand perfection. Several valid issues have been raised, which should be
> fixed by code freeze (the date of which is not yet set). However, the
> utility of this work is obvious to me. At the end of the day, what bar are
> we trying to set for inclusion of a patch?
>
> Adam
>
>
>
> On Wed, Jan 30, 2013 at 11:05 AM, William Slacum <
> wilhelm.von.cloud@accumulo.net> wrote:
>
> > Bottom line, the patch has no value added to general users. The idea
> behind
> > pushing back a release date to stuff in unoperational code is very bad
> > practice. It sets a precedent for not considering alternative approaches
> > while simultaneously having no justification for choosing the approach we
> > did. If a specific customer/group/person wants a feature, and that
> feature
> > does not exist yet, the code is freely available to be modified,
> > distributed and open to public review. Adam, I strongly disagree that
> > forking the code is bad, considering the progress that other projects
> make
> > specifically because they have experimental forks (HBase).
> >
> > On Wed, Jan 30, 2013 at 10:40 AM, Adam Fuchs <afuchs@apache.org> wrote:
> >
> > > Let me attempt to make another argument for why the 958 patch should be
> > > included in 1.5.0. What this patch represents is not an encryption
> > solution
> > > for WAL, but an experimental extension point that will be used for
> > building
> > > an encryption solution as a pluggable module. We need to judge its
> merit
> > > based on whether it is a successful experimental extension point or
> not.
> > > There are three main reasons for including the patch in 1.5.0:
> > > 1. Test the performance impact of the null cipher solution (default
> > > configuration) in all the performance tests we will be running for the
> > > 1.5.0 release. If it causes problems there then we can roll it back.
> > > 2. Enable the use of this extension after 1.5 is released. External
> > > experiments have dependencies on this extension point. Without the
> > > extension point we will have to test with unreleased versions of
> > Accumulo,
> > > which would be less than ideal.
> > > 3. It is not harmful and somebody wants it. The reason for wanting this
> > > code in is well documented, so you need a very strong reason to throw
> it
> > > out. Otherwise you will encourage forking of the project (which would
> be
> > > bad).
> > >
> > > Adam
> > >
> > >
> > >
> > >
> > > On Wed, Jan 30, 2013 at 10:09 AM, Eric Newton <eric.newton@gmail.com>
> > > wrote:
> > >
> > > > Some comments about the comments in ACCUMULO-958:
> > > >
> > > > Josh writes:
> > > >
> > > > "We still have the ability to review this even after the feature
> freeze
> > > > happens, it's just frustrating from my point of view in generating
> the
> > > best
> > > > 1.5.0 candidate possible (we tend to go through x.y.0 releases pretty
> > > darn
> > > > quick)."
> > > >
> > > > John writes:
> > > >
> > > > "Yes, but we get stuck on x.y.* for a year or so, so it does become a
> > > race
> > > > to get all the features you want to see in the next year."
> > > >
> > > > As Accumulo matures, we will need to start thinking a little more
> > > flexibly
> > > > about what goes into minor releases.  We have implemented new (small)
> > > > features in minor releases before.
> > > >
> > > > I would have no problem including ACCUMULO-958 into 1.5.1 after a
> test
> > > > phase, and after some basic experience with the feature.  However I'm
> > > very
> > > > uncomfortable including this in 1.5.0 because there is not a single
> > test,
> > > > and no real implementation behind the factory that anyone would use
> In
> > > Real
> > > > Life.  Is this an appropriate API?  I have no idea.  Comments in the
> > code
> > > > about the stability of the interface basically admit that the author
> > > isn't
> > > > completely comfortable with it, either.
> > > >
> > > > Let's not rush it, and when it is done right, I'm all for putting it
> > into
> > > > the next release.  For now, I would hold back incorporating these
> > changes
> > > > until they are more fully implemented. After we branch 1.5, commit
> this
> > > to
> > > > trunk, and back-port it to the 1.5 branch when experience and tests
> > show
> > > it
> > > > is ready to be released.
> > > >
> > > > -Eric
> > > >
> > > >
> > > >
> > > > On Wed, Jan 30, 2013 at 9:13 AM, Josh Elser <josh.elser@gmail.com>
> > > wrote:
> > > >
> > > > > All,
> > > > >
> > > > > It's been a few days and I haven't seen much chatter at all on
> > > > > ACCUMULO-958 [1] since the patch was applied. There are a couple
of
> > > > > concerns I have that I definitely want to see addressed before a
> > 1.5.0
> > > > > release.
> > > > >
> > > > > - It worries me that the provided patch is fail-open (when we can't
> > > load
> > > > > the configured encryption strategies/modules, we don't decrypt
> > > anything.
> > > > I
> > > > > think for a security-minded database, we should probably be
> > defaulting
> > > to
> > > > > fail-close; but, that brings up an issue, what happens when we
> can't
> > > > > encrypt a WAL? Do minor compactions fail gracefully? What does
> > Accumulo
> > > > do?
> > > > >
> > > > > - John said he had been reviewing the patch before he applied it;
> it
> > > > > bothers me that there was a version of this patch that had been
> > > reviewed
> > > > > privately for some amount of time when we had already pushed back
> the
> > > > > feature freeze date by a week waiting for features that weren't
> done.
> > > > >
> > > > > - The author noted himself with the deprecation of the CryptoModule
> > > > > interface that "we anticipate changing [this] in non-backwards
> > > compatible
> > > > > ways as we explore requirements for encryption in Accumulo...".
> This
> > > > tells
> > > > > me that implementation of WAL encryption overall hasn't been
> properly
> > > > > thought out.
> > > > >
> > > > > Given all of this, it gives me great pause to knowingly include
> this
> > > > patch
> > > > > into a 1.5.0 release. I see no signs that this has been truly
> thought
> > > > out,
> > > > > there is no default provided encryption strategy for 1.5.0 with
> this
> > > > patch
> > > > > for the WAL and there is still no support at all for RFile
> encryption
> > > (no
> > > > > end-to-end Accumulo encryption for a user). All of these issues
> > > > considered
> > > > > make me believe that this is an incomplete feature that is not
> ready
> > > for
> > > > an
> > > > > Apache Accumulo release.
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > - Josh
> > > > >
> > > > > [1] https://issues.apache.org/**jira/browse/ACCUMULO-958<
> > > > https://issues.apache.org/jira/browse/ACCUMULO-958>
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message