Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: Support adding in an accumulo-access expression to a column visibility #5293

Draft
wants to merge 2 commits into
base: 2.1
Choose a base branch
from
Draft
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Adds new constructor to ColumnVisibility
Adds a new constructor that supports an AccessExpression.
Also adds deep copy method for existing ColumnVisibilities.
ddanielr committed Jan 29, 2025
commit 1645738f5a70a58980b99571097b2e345bf2878b
6 changes: 6 additions & 0 deletions core/pom.xml
Original file line number Diff line number Diff line change
@@ -85,6 +85,10 @@
<groupId>io.opentelemetry</groupId>
<artifactId>opentelemetry-context</artifactId>
</dependency>
<dependency>
<groupId>org.apache.accumulo</groupId>
<artifactId>accumulo-access</artifactId>
</dependency>
<dependency>
<groupId>org.apache.accumulo</groupId>
<artifactId>accumulo-start</artifactId>
@@ -274,6 +278,8 @@
<allow>javax[.]security[.]auth[.]DestroyFailedException</allow>
<!-- allow questionable Hadoop exceptions for mapreduce -->
<allow>org[.]apache[.]hadoop[.]mapred[.](FileAlreadyExistsException|InvalidJobConfException)</allow>
<!-- allow the following types from the visibility API -->
<allow>org[.]apache[.]accumulo[.]access[.].*</allow>
</allows>
</configuration>
</execution>
Original file line number Diff line number Diff line change
@@ -28,6 +28,7 @@
import java.util.List;
import java.util.TreeSet;

import org.apache.accumulo.access.AccessExpression;
import org.apache.accumulo.core.data.ArrayByteSequence;
import org.apache.accumulo.core.data.ByteSequence;
import org.apache.accumulo.core.util.BadArgumentException;
@@ -133,6 +134,24 @@ public void add(Node child) {
children.add(child);
}

/**
* Creates a new node by performing a deep copy of an existing node object
*
* @param node Node object
* @since 2.1.4
*/
private Node(Node node) {
List<Node> childrenNew =
node.children.isEmpty() ? EMPTY : new ArrayList<>(node.children.size());
for (Node child : node.children) {
childrenNew.add(new Node(child));
}
this.type = node.type;
this.start = node.start;
this.end = node.end;
this.children = childrenNew;
}

public NodeType getType() {
return type;
}
@@ -505,6 +524,31 @@ public ColumnVisibility(byte[] expression) {
validate(expression);
}

/**
* Creates a column visibility for a Mutation from an AccessExpression.
*
* @param expression visibility expression, encoded as UTF-8 bytes
* @see #ColumnVisibility(String)
* @since 2.1.4
*/
public ColumnVisibility(AccessExpression expression) {
// AccessExpression is a validated immutable object, so no need to re validate
this.expression = expression.getExpression().getBytes(UTF_8);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just an optimization, or does calling validate cause an issue? A client could use AccessExpression's and then create a ColumnVisibilty using the byte[] constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an optimization to avoid calling validate for performance reasons. If an AccessExpression is passed in then it has already performed a validate operation previous and does not need to parse the expression.

Copy link
Contributor

@dlmarion dlmarion Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a constructor (byte[]) then that doesn't perform validation, mark it deprecated, and in the javadoc:

  1. that this constructor was added for clients that want to use Accumulo-Access before Accumulo 4.0
  2. this will be removed in 4.0
  3. using this incorrectly (with an invalid column visibility) will likely cause exceptions and no data to be returned

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that the above approach may be a good compromise to allow Accumulo-Access to be used with 2.1 without introducing the dependency in a patch release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that we already have a ColumnVisibility(byte[]) constructor, we would likely need to create a ColumnVisibility(byte[], boolean) constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that we already have a ColumnVisibility(byte[]) constructor, we would likely need to create a ColumnVisibility(byte[], boolean) constructor.

I added that constructor but made it private. Then I added a new static method to use the private constructor called fromAccessExpression so that creation of these unvalidated ColumnVisibilities is very explicit.

public static ColumnVisibility fromAccessExpression(byte[] incomingExpression) {
    return new ColumnVisibility(incomingExpression, false);
  }

The name should probably be changed, so I'm open to suggestions.

}

/**
* Creates a new column visibility by performing a deep copy of an existing column visibility
* object
*
* @param visibility ColumnVisibility object
* @since 2.1.4
*/
public ColumnVisibility(ColumnVisibility visibility) {
byte[] incomingExpression = visibility.expression;
this.expression = Arrays.copyOf(incomingExpression, incomingExpression.length);
this.node = new Node(visibility.node);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constructor is still needed since it also copies the parse tree from the original ColVis

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is an issue here with the parse tree being mutable. Given that ColumnVisibility's can be cached, and that the parse tree is mutable, it's possible that the parse tree could be mutating in another thread while this copy constructor is called. I'm not sure how to resolve this issue. Using the Cloneable interface and adding a clone method doesn't fully solve this issue either. I think this issue exists in #5217 as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that doing the following may help the issue I raised above:

  1. Modify Node.children to be Collections.synchronizedList(new ArrayList<>()) in Node.add
  2. synchronize on visibility.node in the copy constructor

}

@Override
public String toString() {
return "[" + new String(expression, UTF_8) + "]";
Original file line number Diff line number Diff line change
@@ -22,6 +22,7 @@
import static org.apache.accumulo.core.security.ColumnVisibility.quote;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

@@ -238,6 +239,15 @@ public void testParseTreesOrdering() {
assertTrue(flat.indexOf('b') < flat.indexOf('a'), "shortest children sort first");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add test cases for new constructors.

@Test
public void testDeepCopy() {
ColumnVisibility cv1 = new ColumnVisibility("(b&c&d)|((a|m)&y&z)|(e&f)");
ColumnVisibility cv2 = new ColumnVisibility(cv1);
assertNotSame(cv1.getExpression(), cv2.getExpression());
assertNotSame(cv1.getParseTree(), cv2.getParseTree());
assertEquals(cv1.toString(), cv2.toString());
}

private Node parse(String s) {
ColumnVisibility v = new ColumnVisibility(s);
return v.getParseTree();
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
@@ -146,6 +146,7 @@
<surefire.reuseForks>true</surefire.reuseForks>
<unitTestMemSize>-Xmx1G</unitTestMemSize>
<!-- dependency and plugin versions managed with properties -->
<version.accumulo-access>1.0.0-SNAPSHOT</version.accumulo-access>
<version.auto-service>1.1.1</version.auto-service>
<version.bouncycastle>1.78.1</version.bouncycastle>
<version.curator>5.5.0</version.curator>
@@ -328,6 +329,11 @@
<artifactId>junit</artifactId>
<version>4.13.2</version>
</dependency>
<dependency>
<groupId>org.apache.accumulo</groupId>
<artifactId>accumulo-access</artifactId>
<version>${version.accumulo-access}</version>
</dependency>
<dependency>
<groupId>org.apache.accumulo</groupId>
<artifactId>accumulo-compaction-coordinator</artifactId>