[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2518 Forgot to mention that the NiFi Flow I used to test, is available here: https://gist.github.com/ijokarumawak/c455075f7830935189e5efbd83e5d863 ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2518 @bbende Thanks for your comment. That's true, we need to rename not only NAR file but also component class names. I agree with staying 1.1.2 unless there is no necessary updates. @MikeThomsen The latest Travis failed to build, but the reason was timeout. I'm going to build and test locally once again just in case now. If it looks fine, then I will squash commits and merge. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user bbende commented on the issue: https://github.com/apache/nifi/pull/2518 @MikeThomsen @ijokarumawak I'd say unless there is something in the 1.2 client that is needed to implement the visibility labels, then it might be safer to just stay on 1.1.2, and hopefully soon we can put out a new NAR for the 2.0.0 client service, and of course name it "2_x_x" or something. I wish I had named the current service and NAR "1_1_x" or "1_x_x" from the beginning :( If we renamed things now then we'd have to rename the NAR and three services - HBase_1_1_2_ClientService, HBase_1_1_2_RecordLookupService, and HBase_1_1_2_MapCacheClientService... and then anyone who has an existing flow with those services will have ghosted components that can no longer find those class names, and then have to recreate them with the new class to get it working again. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @ijokarumawak version reverted and building... ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 I'll add a patch to revert the version # on the client lib. Since HDP is on 1.1.2 and CDH 5.14 is still on 1.2, I don't think it's safe to jump up to anything higher than a 1.2 client anyway. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2518 @MikeThomsen Thanks for the updates. It mostly looks good. The only concern I have is the gap between the name of NAR and the HBase client version. One possible solution is to rename the NAR from `nifi-hbase_1_1_2-client-service` to `nifi-hbase_1_1_x-client-service` or `nifi-hbase_1_1-client-service`, to avoid having too specific version in it so that we can update it in the future, assuming NiFi can pick the right NAR for existing flow when user upgrade NiFi even nar name changes. If you agree with that approach, or to keep discussion on that specific topic separately, would you revert the HBase dependency version back to 1.1.2 for this PR, and submit another JIRA to do the version bump? Then we can close this one. @bbende Any comment from you on this subject would be appreciated. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @ijokarumawak Anything left before you're comfortable merging this? ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 Added `additionalDetails` for `PutHBaseCell` and `PutHBaseRow`. Also fixed up some of the behavior of the `pickVisibilityLabel` method. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @ijokarumawak forgot the documentation... ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @ijokarumawak I think it's all in now. I just finished the last change you requested. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2518 @joshelser Thanks for your prompt answer. I didn't know that different values can be stored with the same key but different visibility label expression. The explanation 'The expression also contributes to the uniqueness of that key, almost acting as a kind of "attribute" for the record being store.' makes current design more understandable for me. If that is a part of key for the value, then perfect matching one should be specified. I will experiment it later. Thanks! ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user joshelser commented on the issue: https://github.com/apache/nifi/pull/2518 > Would you please educate me why delete operation uses expression instead of a comma separated labels as scan does The general reasoning is this: you may have multiple "values" for the same key at different levels of visibility. Consider credit-card information: ``` josh-creditcard1 f:number [private] -> 123456-1234-12345678 josh-creditcard1 f:number [seller] -> xx--5678 ``` The visibility label for Accumulo is an expression that defines if a user with a collection of visibility labels is allowed to see that record. The expression also contributes to the uniqueness of that key, almost acting as a kind of "attribute" for the record being store. Requiring the exact visibility label to delete the record is also important in a multi-tenant system with various levels of visibility because you may not know if other copies of the Key exist that you are unaware of. For example, if a "seller" was trying to delete my creditcard1 information, they would be unaware that my full creditcard number also exists there -- if we gave acknowledgment that it was deleted, that would be an information leak. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2518 @MikeThomsen Thanks for the quick update, I will continue reviewing. @bbende Thanks for the info. What I was wondering about is why HBase requires a 'Visibility Label Expression' instead of a comma separated visibility label list for delete operations. In order to understand the theme more, I installed and played with Accumulo, too. It turned out Accumulo and HBase both require 'Visibility Label Expression' for delete. @joshelser Would you please educate me why delete operation uses expression instead of a comma separated labels as scan does? E.g. to delete a cell having `(apple&carrot)|broccoli|spinach`, I had to specify exactly the same expression. I expected executing a delete request by a user having all of authorizations would delete the cell, or the logical expression would match if `spinach` was specified. But it didn't behave like that, a matching expression was needed. Following results are what I got executing delete operations based on the [Accumulo example](https://accumulo.apache.org/1.7/examples/visibility): ``` # Insert test data username@instance vistest> insert row f1 q1 v1 -l A username@instance vistest> insert row f2 q2 v2 -l A&B username@instance vistest> insert row f3 q3 v3 -l (apple&carrot)|broccoli|spinach # It seems users authorizations are not used for delete, and need to be specified explicitly username@instance vistest> delete row f1 q1 username@instance vistest> scan row f1:q1 [A]v1 row f2:q2 [A&B]v2 row f3:q3 [(apple&carrot)|broccoli|spinach]v3 username@instance vistest> delete row f1 q1 -l A username@instance vistest> scan row f2:q2 [A&B]v2 row f3:q3 [(apple&carrot)|broccoli|spinach]v3 # Specifying wrong label does not return error username@instance vistest> delete row f3 q3 -l spina username@instance vistest> scan row f3:q3 [(apple&carrot)|broccoli|spinach]v3 # The visibility label expression should match exactly username@instance vistest> delete row f3 q3 -l spinach username@instance vistest> scan row f3:q3 [(apple&carrot)|broccoli|spinach]v3 username@instance vistest> delete row f3 q3 -l spinach|broccoli username@instance vistest> scan row f3:q3 [(apple&carrot)|broccoli|spinach]v3 username@instance vistest> delete row f3 q3 -l spinach|broccoli|(apple&carrot) username@instance vistest> scan row f3:q3 [(apple&carrot)|broccoli|spinach]v3 username@instance vistest> delete row f3 q3 -l (apple&carrot)|broccoli username@instance vistest> scan row f3:q3 [(apple&carrot)|broccoli|spinach]v3 username@instance vistest> delete row f3 q3 -l (apple&carrot)|broccoli|spinach username@instance vistest> scan # There are no rows username@instance vistest> ``` ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @ijokarumawak pushed the documentation change you requested and switched PutHBaseCell/JSON to use dynamic properties. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user bbende commented on the issue: https://github.com/apache/nifi/pull/2518 I think we are saying the same thing, the authorizations can come from anywhere, but I was just using end-user as an example. In Accumulo you would issue an operation and pass in an array of labels like [TS, S]. Cells that have (TS), (S), (TS || S), or (TS & S) would all match. Cells that have (TS & FOO) would not. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2518 @MikeThomsen @bbende HBase allows user to set visibility label expression using labels those are not associated to the user by default. There is a server side configuration to strict this `hbase.security.visibility.mutations.checkauths`. I am not familiar with Accumulo behavior but there was a mention about the difference between Accumulo and HBase. > The deletes happen based on pattern matching. If suppose a Cell has SECRET&TOPSECRET as the visibility expression then any delete coming with the same expression is matched for, but TOPSECRET&SECRET is also considered to be same > A&B<=>B&A > A|B<=>B|A. > This is the difference between what Accumulo does in terms of delete where only the exact match is looked out for. https://issues.apache.org/jira/browse/HBASE-10885?focusedCommentId=14053522&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14053522 ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @bbende that is my understanding as well. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user bbende commented on the issue: https://github.com/apache/nifi/pull/2518 @MikeThomsen @ijokarumawak I haven't looked at how Hbase visibility labels work compared to Accumulo, but in Accumulo you pass in the authorizations for an operations which are then compared against the visibility strings on each cell. The authorizations on the operation usually come from authenticating an end-user against LDAP and then running an operation on their behalf. When scanning cells for the current operation, if the passed in authorizations don't meet the visiblity string for a given cell, then it is as if this cell doesn't exist. So if you were issuing a delete on behalf of an end-user, I would expect they can only delete cells that are visible to them based on their authorizations. Curious to hear that Josh has to say. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @joshelser Quick question about Accumulo, does it require you to specify the visibility string in a delete mutation? ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2518 @MikeThomsen Thanks for rebasing. I am now testing against my HBase instance. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @ijokarumawak changes pushed. Ready for review. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @ijokarumawak Disregard that last comment. I apparently forgot to update the EL support. Will get that done today. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @ijokarumawak New build pushed. Sorry about the squash commit wiping out the existing comments, but I had to do it in order to save time and stress on integrating the EL changes and things like that. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user ijokarumawak commented on the issue: https://github.com/apache/nifi/pull/2518 @MikeThomsen For upgrading HBase dependency version, I'd do create a new NAR. So that people have dependency with HBase 1.1.x can keep using HBase_1_1_2_ClientService. We do the same to support different Kafka versions. About the naming, 'labels' or 'authorizations', I added [another comment above](#discussion_r182954434). Please check that out. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @bbende @ijokarumawak To move that IT into the client test package, I have to upgrade the client's hbase-client dependency to 1.2.X from 1.1.X. Do you see anything wrong with doing that? At this point 1.1.2 is very old. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 Thanks for jumping on this @ijokarumawak I'll try to find some time today to resolve and push a fix. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @bbende @ijokarumawak could you take a look at this once 1.6 is released? ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user joshelser commented on the issue: https://github.com/apache/nifi/pull/2518 > Would you be open to a code review in the future (months away at least) if I were to start adding Accumulo support? Feel free to hit me up :) ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 @joshelser Unrelated to this... Would you be open to a code review in the future (months away at least) if I were to start adding Accumulo support? ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 Had to rebase because of the commit that added `ScanHBase`. That doesn't have visibility labels added to it yet. That needs to be done. Keeping this ticket open because there's plenty that can be reviewed in the mean time if anyone is interested. ---
[GitHub] nifi issue #2518: NIFI-4637 Added support for visibility labels to the HBase...
Github user MikeThomsen commented on the issue: https://github.com/apache/nifi/pull/2518 This also adds a new processor: DeleteHBaseCells. ---
