You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@fluo.apache.org by GitBox <gi...@apache.org> on 2022/06/07 02:13:26 UTC
[GitHub] [fluo] wjsl opened a new pull request, #1120: First pass at adding scan time authorizations to transactions/snapshots
wjsl opened a new pull request, #1120:
URL: https://github.com/apache/fluo/pull/1120
Rough cut of this. Needs to fix documentation, and add assertion to the tests. Open to suggestions on better test cases.
This PR adds support for the following:
1. Specifying authentication tokens to use with the `FluoClient` on construction.
2. Specifying authentication tokens to use with a `Snapshot` when performing a read.
I also think it makes sense to ensure the fluo table has the constraint set on it so unreadable data can't be written. Currently the behavior I'd see in my simple test is Fluo would kick back an error about a failed transaction because it couldn't read the data back. I think the better behavior (as Keith suggested) would be avoid any confusion in that case and kick back a "hey this data violates a table constraint due to its visibilities" error.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on a diff in pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #1120:
URL: https://github.com/apache/fluo/pull/1120#discussion_r1059231874
##########
modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java:
##########
@@ -1571,6 +1579,20 @@ private void beginCommitAsyncTest(CommitData cd) {
}
public SnapshotScanner newSnapshotScanner(Span span, Collection<Column> columns) {
- return new SnapshotScanner(env, new SnapshotScanner.Opts(span, columns, false), startTs, stats);
+ return newSnapshotScanner(span, columns, env.getAuthorizations());
Review Comment:
Looking into to it, this whole method can be removed. Just pushed a change to my branch with that update.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #1120:
URL: https://github.com/apache/fluo/pull/1120#issuecomment-1367705887
@wjsl I was looking at this. The API returned a Snapshot which gave me the feel that maybe it would not change the current snapshot object, but would create a new one with the auths set. That was not the behavior though. So I pulled the PR down locally to play around with making some changes to the API. I made a PR against your branch with the changes from my experimentation. That PR is at https://github.com/wjsl/fluo/pull/1
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on a diff in pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #1120:
URL: https://github.com/apache/fluo/pull/1120#discussion_r892872914
##########
modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java:
##########
@@ -1571,6 +1579,20 @@ private void beginCommitAsyncTest(CommitData cd) {
}
public SnapshotScanner newSnapshotScanner(Span span, Collection<Column> columns) {
- return new SnapshotScanner(env, new SnapshotScanner.Opts(span, columns, false), startTs, stats);
+ return newSnapshotScanner(span, columns, env.getAuthorizations());
+ }
+
+ public SnapshotScanner newSnapshotScanner(Span span, Collection<Column> columns,
Review Comment:
There is also a parallel snapshots scanner used by this class that is a wrapper around the batch scanner. I am not sure if its gettings the correct auths passed to it.
##########
modules/api/src/main/java/org/apache/fluo/api/client/Snapshot.java:
##########
@@ -29,4 +31,8 @@ public interface Snapshot extends SnapshotBase, AutoCloseable {
*/
@Override
void close();
+
+ default Snapshot useScanTimeAuthz(Collection<String> authz) {
+ return this;
Review Comment:
```suggestion
/**
* Sets the authorizations to use when reading data. This overrides anything that may have been set in the configuration used to create a FluoClient.
*
* @since 1.2.0
*/
default Snapshot useScanTimeAuthorizations(Collection<String> authz) {
throw new UnsupportedOperationException();
```
##########
modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java:
##########
@@ -1571,6 +1579,20 @@ private void beginCommitAsyncTest(CommitData cd) {
}
public SnapshotScanner newSnapshotScanner(Span span, Collection<Column> columns) {
- return new SnapshotScanner(env, new SnapshotScanner.Opts(span, columns, false), startTs, stats);
+ return newSnapshotScanner(span, columns, env.getAuthorizations());
Review Comment:
Do we want to use env.getAuthorizations()? What about auths set on the snapshot?
##########
modules/api/src/main/java/org/apache/fluo/api/client/FluoClient.java:
##########
@@ -75,6 +77,10 @@ public interface FluoClient extends AutoCloseable {
*/
MetricsReporter getMetricsReporter();
+ default FluoClient useAuthorizations(Collection<String> authLabels) {
+ return this;
+ }
+
Review Comment:
If authorizations can be set in the FluoConfiguration, then this method is probably not needed. FluoClients are always created from configuration and if the configuration has auths then the client can get them that way.
##########
modules/integration-tests/src/main/java/org/apache/fluo/integration/client/FluoClientIT.java:
##########
@@ -83,4 +97,72 @@ public void testFailures() {
Logger.getLogger(FluoClientImpl.class).setLevel(clientLevel);
Logger.getLogger(FluoFactory.class).setLevel(factoryLevel);
}
+
+ @Test(expected = CommitException.class)
+ public void testWriteWithDefaultAuths() throws Throwable {
+ Column labeledColumn = new Column("data", "private_column", "PRIVATE");
+ try (FluoClient client = FluoFactory.newClient(config);
+ Transaction txn = client.newTransaction()) {
+ txn.set("bill", labeledColumn, "a value");
+ txn.commit();
+ }
+ }
+
+ @Test
+ public void testScanTimeAuths() throws Throwable {
+ try (AccumuloClient accumulo = AccumuloUtil.getClient(config)) {
+ accumulo.securityOperations().changeUserAuthorizations(config.getAccumuloUser(),
+ new Authorizations("PRIVATE", "PUBLIC"));
+ }
+
+ Column ssn = new Column("", "ssn", "PRIVATE");
+ Column name = new Column("", "name", "PUBLIC");
+ try (FluoClientImpl client = (FluoClientImpl) FluoFactory.newClient(config)) {
+ client.useAuthorizations(ImmutableList.of("PRIVATE", "PUBLIC"));
Review Comment:
In addition to setting auths on client, would be nice to also test setting them on snapshot.
##########
modules/integration-tests/src/main/java/org/apache/fluo/integration/client/FluoClientIT.java:
##########
@@ -83,4 +97,72 @@ public void testFailures() {
Logger.getLogger(FluoClientImpl.class).setLevel(clientLevel);
Logger.getLogger(FluoFactory.class).setLevel(factoryLevel);
}
+
+ @Test(expected = CommitException.class)
+ public void testWriteWithDefaultAuths() throws Throwable {
+ Column labeledColumn = new Column("data", "private_column", "PRIVATE");
+ try (FluoClient client = FluoFactory.newClient(config);
+ Transaction txn = client.newTransaction()) {
+ txn.set("bill", labeledColumn, "a value");
+ txn.commit();
+ }
+ }
+
+ @Test
+ public void testScanTimeAuths() throws Throwable {
+ try (AccumuloClient accumulo = AccumuloUtil.getClient(config)) {
+ accumulo.securityOperations().changeUserAuthorizations(config.getAccumuloUser(),
+ new Authorizations("PRIVATE", "PUBLIC"));
+ }
+
+ Column ssn = new Column("", "ssn", "PRIVATE");
+ Column name = new Column("", "name", "PUBLIC");
+ try (FluoClientImpl client = (FluoClientImpl) FluoFactory.newClient(config)) {
+ client.useAuthorizations(ImmutableList.of("PRIVATE", "PUBLIC"));
+ try (Transaction txn = client.newTransaction()) {
+ txn.set("bill", ssn, "1");
+ txn.set("bill", name, "william");
+ txn.set("bob", ssn, "2");
+ txn.set("bob", name, "robert");
+ txn.commit();
+ }
+
+ System.out.println("***********************************");
+ System.out.println("Default auths");
+ System.out.println("***********************************");
+ try (Snapshot snapshot = client.newSnapshot()) {
+ System.out.println(snapshot.get(Bytes.of("bill")));
Review Comment:
snapshot has multiple get methods, some use scanner and some use batch scanner. Would be nice to test them all.
##########
modules/api/src/main/java/org/apache/fluo/api/config/FluoConfiguration.java:
##########
@@ -510,6 +514,19 @@ public String getAccumuloPassword() {
throw new NoSuchElementException(ACCUMULO_PASSWORD_PROP + " is not set!");
}
+ public FluoConfiguration setAccumuloAuthorizations(String authsCsv) {
+ setProperty(ACCUMULO_AUTH_PROP, authsCsv);
+ return this;
+ }
+
+ public String getAccumuloAuthorizations() {
Review Comment:
This could return a list.
```suggestion
/**
* Returns the authorizations configured for reading data or an empty list if none are configured.
*
* @since 1.2.0
*/
public List<String> getAccumuloAuthorizations() {
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on a diff in pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #1120:
URL: https://github.com/apache/fluo/pull/1120#discussion_r1059229771
##########
modules/api/src/main/java/org/apache/fluo/api/client/FluoClient.java:
##########
@@ -15,6 +15,8 @@
package org.apache.fluo.api.client;
+import java.util.Collection;
+
Review Comment:
```suggestion
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] ctubbsii commented on a diff in pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #1120:
URL: https://github.com/apache/fluo/pull/1120#discussion_r1059479860
##########
modules/api/src/main/java/org/apache/fluo/api/config/SimpleConfiguration.java:
##########
@@ -242,6 +243,27 @@ public void setProperty(String key, String value) {
internalConfig.setProperty(key, value);
}
+ public void setProperties(String key, String... values) {
+ if (values == null) {
Review Comment:
`values` can be null, but you can also have an array with some of the elements null. Both cases should be an `IllegalArgumentException`, I think.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner merged pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
keith-turner merged PR #1120:
URL: https://github.com/apache/fluo/pull/1120
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #1120:
URL: https://github.com/apache/fluo/pull/1120#issuecomment-1336170938
@wjsl I was thinking of spinning a Fluo release. Is this something you would like to see in the next release?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on a diff in pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #1120:
URL: https://github.com/apache/fluo/pull/1120#discussion_r1059230134
##########
modules/api/src/main/java/org/apache/fluo/api/config/SimpleConfiguration.java:
##########
@@ -242,6 +243,27 @@ public void setProperty(String key, String value) {
internalConfig.setProperty(key, value);
}
+ public void setProperties(String key, String... values) {
+ if (values == null) {
Review Comment:
Thinking it would be best to throw an exception if null.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] wjsl commented on pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
wjsl commented on PR #1120:
URL: https://github.com/apache/fluo/pull/1120#issuecomment-1336251047
I do. I have some changes we needed while integrating that I need to push.
Will have it early next week.
On Sat, Dec 3, 2022 at 9:26 AM Keith Turner ***@***.***>
wrote:
> @wjsl <https://github.com/wjsl> I was thinking of spinning a Fluo
> release. Is this something you would like to see in the next release?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/fluo/pull/1120#issuecomment-1336170938>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAKB7ZKJXKPVVXQVV6V2ITLWLNKBVANCNFSM5YBJANZQ>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on a diff in pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #1120:
URL: https://github.com/apache/fluo/pull/1120#discussion_r1059229937
##########
modules/api/src/main/java/org/apache/fluo/api/config/FluoConfiguration.java:
##########
@@ -510,6 +514,19 @@ public String getAccumuloPassword() {
throw new NoSuchElementException(ACCUMULO_PASSWORD_PROP + " is not set!");
}
+ public FluoConfiguration setAccumuloAuthorizations(String... auths) {
Review Comment:
```suggestion
/**
* @since 2.0.0
*/
public FluoConfiguration setAccumuloAuthorizations(String... auths) {
```
##########
modules/api/src/main/java/org/apache/fluo/api/config/FluoConfiguration.java:
##########
@@ -510,6 +514,19 @@ public String getAccumuloPassword() {
throw new NoSuchElementException(ACCUMULO_PASSWORD_PROP + " is not set!");
}
+ public FluoConfiguration setAccumuloAuthorizations(String... auths) {
+ setProperties(ACCUMULO_AUTH_PROP, auths);
+ return this;
+ }
+
+ public String[] getAccumuloAuthorizations() {
Review Comment:
```suggestion
/**
* @since 2.0.0
*/
public String[] getAccumuloAuthorizations() {
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fluo] keith-turner commented on pull request #1120: First pass at adding scan time authorizations to transactions/snapshots
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #1120:
URL: https://github.com/apache/fluo/pull/1120#issuecomment-1383051349
@wjsl I ran the build locally and took another look at this, looks good. Thanks for the contribution. If you would like to add yourself as a contributor please submit a PR to update [people.md](https://github.com/apache/fluo-website/edit/main/pages/people.md).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@fluo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org