ctubbsii commented on pull request #1968:
URL: https://github.com/apache/accumulo/pull/1968#issuecomment-807745614
> > instance.crypto.opts.table.t1.SOMEKEY=SOMEVALUE
>
> I don't think this will work with the way Accumulo handles table
properties differently. Also aren't `instance` properties used to determine the
hash for a cluster? It would also be confusing because it breaks the
delineations set by the prefix properties.
`general.*` would also work (and could be changed at runtime, unlike
`instance.*`). The main point here is that these aren't characteristics of the
table, but rather, characteristics of the crypto service module that inform it
of how to behave in specific contexts.
> > With both of these, the main point is that there is only ever a single
crypto service for the system. However, the functionality of that service is
capable of providing crypto services for multiple tables and/or the WAL. Your
change just relocates the implementation into inner classes, but they are still
effectively separate classes that are configured separately, meaning separate
authorities for managing the file-based crypto.
>
> Yes but the crypto implementation for Table class won't work with WALs,
due to the type of encryption.
Right, that's why the config is associated with the crypto service module,
as opts, rather than tied to a table's characteristics. What opts work for
which contexts, depends largely on the crypto service module's capabilities and
is not solely an inherent characteristics of Accumulo objects (tables, WALs,
etc.). Offloading this responsibility to the module
> I was trying to keep them separate enough to make this clear that not all
types of crypto will work with WALs vs Tables but keep them together enough to
implement the same interfaces.
That makes sense, but I don't think we need to have separate services
conforming to the same interface for these. We can have a single service
interface that provides services for both streaming encrypters/decrypters
suitable for WAL, and for block encrypters/decrypters suitable for RFiles. If
we really wanted to have them conform to the same interface, maybe that's okay,
but instead of merely being colocated and configured separately, they could
dangle off a single crypto service interface as sub-services, so you only have
to configure the main crypto service module, and that acts as a factory for the
sub-services that dangle from it.
> I believe what you want is what we have now, passing in the environment to
the impl and letting it decided Table vs WAL.
I do lean towards the design principles that were put into the earlier
effort, yes.
> The problem is Accumulo needs to know whether the CryptoService is being
used per table or tserver (WAL). Otherwise it gets very confusing looking at
the code and trying to decipher the purpose of the CryptoService. Some of this
can be derived from the code, for example within RFile or BCFile is obvious.
But other parts are not, like the Tserver or FileUtilties. We can make sure to
name variables based on use but the CryptoServiceFactory is a static utility.
I think we can adapt the existing design by incorporating additional context
information into the API, so the crypto service module can make decisions about
the encrypters/decrypters to provide, based on the context it is given.
In general, what I'm arguing for is an API design that looks like:
```java
public interface SomeService {
public SomeObject getSomeObject(Context ctx);
}
```
rather than one that looks like:
```java
public interface SomeService {
public SomeObject getSomeObject();
}
public class SomeServiceForContext1 extend SomeService {
public SomeObject getSomeObject() {};
}
public interface SomeServiceForContext2 extend SomeService {
public SomeObject getSomeObject() {};
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
