-
Notifications
You must be signed in to change notification settings - Fork 273
Part 1 : Adds RLS and CLS control Policies #2048
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.apache.polaris.core.policy; | ||
|
||
import static org.apache.polaris.core.policy.PredefinedPolicyTypes.ACCESS_CONTROL; | ||
|
||
import com.google.common.collect.Lists; | ||
import java.util.List; | ||
import org.apache.iceberg.expressions.Expression; | ||
import org.apache.iceberg.expressions.Expressions; | ||
import org.apache.iceberg.expressions.UnboundPredicate; | ||
import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; | ||
import org.apache.polaris.core.policy.content.AccessControlPolicyContent; | ||
|
||
public class PolicyUtil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (best practices) can you please add some unit tests? |
||
private PolicyUtil() {} | ||
|
||
public static String replaceContextVariable( | ||
String content, PolicyType policyType, AuthenticatedPolarisPrincipal authenticatedPrincipal) { | ||
if (policyType == ACCESS_CONTROL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why force all policies to go through this |
||
try { | ||
AccessControlPolicyContent policyContent = AccessControlPolicyContent.fromString(content); | ||
List<Expression> evaluatedRowFilterExpressions = Lists.newArrayList(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
for (Expression rowFilterExpression : policyContent.getRowFilters()) { | ||
// check if the expression refers to context variable current_principal_role | ||
if (rowFilterExpression instanceof UnboundPredicate<?>) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (design) I would recommend to use a visitor pattern (like |
||
UnboundPredicate<?> boundPredicate = (UnboundPredicate<?>) rowFilterExpression; | ||
// check if this references to current_principal | ||
if (boundPredicate.ref().name().equals("$current_principal_role")) { | ||
// compare the literal and replace | ||
String val = (String) boundPredicate.literal().value(); | ||
if (authenticatedPrincipal.getActivatedPrincipalRoleNames().contains(val)) { | ||
// TODO: see if we can utilize the expression evaluation of iceberg SDK | ||
Expression result = | ||
boundPredicate.op().equals(Expression.Operation.EQ) | ||
? Expressions.alwaysTrue() | ||
: Expressions.alwaysFalse(); | ||
evaluatedRowFilterExpressions.add(result); | ||
} else { | ||
Expression result = | ||
boundPredicate.op().equals(Expression.Operation.NOT_EQ) | ||
? Expressions.alwaysTrue() | ||
: Expressions.alwaysFalse(); | ||
evaluatedRowFilterExpressions.add(result); | ||
} | ||
} else if (boundPredicate.ref().name().equals("$current_principal")) { | ||
String val = (String) boundPredicate.literal().value(); | ||
if (authenticatedPrincipal.getName().equals(val)) { | ||
Expression result = | ||
boundPredicate.op().equals(Expression.Operation.EQ) | ||
? Expressions.alwaysTrue() | ||
: Expressions.alwaysFalse(); | ||
evaluatedRowFilterExpressions.add(result); | ||
} else { | ||
Expression result = | ||
boundPredicate.op().equals(Expression.Operation.NOT_EQ) | ||
? Expressions.alwaysTrue() | ||
: Expressions.alwaysFalse(); | ||
evaluatedRowFilterExpressions.add(result); | ||
} | ||
} else { | ||
evaluatedRowFilterExpressions.add(rowFilterExpression); | ||
} | ||
} | ||
} | ||
|
||
policyContent.setRowFilters(evaluatedRowFilterExpressions); | ||
return AccessControlPolicyContent.toString(policyContent); | ||
} catch (Exception e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (design) is it the right thing to return the original content if an exception occurs? |
||
return content; | ||
} | ||
} | ||
return content; | ||
} | ||
|
||
public static boolean filterApplicablePolicy( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (design) this method doesn't have any javadoc, but shouldn't this belong to some dedicated class related to FGAC instead? |
||
PolicyEntity policyEntity, AuthenticatedPolarisPrincipal authenticatedPrincipal) { | ||
if (policyEntity.getPolicyType().equals(ACCESS_CONTROL)) { | ||
AccessControlPolicyContent content = | ||
AccessControlPolicyContent.fromString(policyEntity.getContent()); | ||
String applicablePrincipal = content.getPrincipalRole(); | ||
return applicablePrincipal == null | ||
|| authenticatedPrincipal.getActivatedPrincipalRoleNames().isEmpty() | ||
|| authenticatedPrincipal | ||
.getActivatedPrincipalRoleNames() | ||
.contains(content.getPrincipalRole()); | ||
} | ||
|
||
return true; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,8 @@ public enum PredefinedPolicyTypes implements PolicyType { | |
DATA_COMPACTION(0, "system.data-compaction", true), | ||
METADATA_COMPACTION(1, "system.metadata-compaction", true), | ||
ORPHAN_FILE_REMOVAL(2, "system.orphan-file-removal", true), | ||
SNAPSHOT_EXPIRY(3, "system.snapshot-expiry", true); | ||
SNAPSHOT_EXPIRY(3, "system.snapshot-expiry", true), | ||
ACCESS_CONTROL(4, "system.access-control", false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The term The "expression" part related to the fact that this policy uses Iceberg expressions to represent filters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC we'll use the same policy to also have non-expression based filtering, but I think that something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can the same policy type support different contents? What is the approach to processing different contents within the same policy type? |
||
|
||
private final int code; | ||
private final String name; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.apache.polaris.core.policy.content; | ||
|
||
import com.fasterxml.jackson.core.JsonProcessingException; | ||
import com.fasterxml.jackson.databind.annotation.JsonDeserialize; | ||
import com.fasterxml.jackson.databind.annotation.JsonSerialize; | ||
import com.google.common.base.Strings; | ||
import java.util.List; | ||
import java.util.Set; | ||
import org.apache.iceberg.expressions.Expression; | ||
import org.apache.polaris.core.policy.validator.InvalidPolicyException; | ||
|
||
public class AccessControlPolicyContent implements PolicyContent { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (design) it may be seen as a preference, but it seems the overall language community is moving towards immutable objects as data carriers (like java records) and wonder if this is something we should adopt here as well |
||
|
||
// Optional, if there means policies is applicable to the role | ||
private String principalRole; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this policy apply to a role? What is the mechanism? I could not find this in the linked doc 🤔 |
||
|
||
// TODO: model them as iceberg transforms | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a plan to redo this policy definition after merging or before merging this PR? |
||
private List<String> columnProjections; | ||
|
||
// Iceberg expressions without context functions for now. | ||
// Use a custom deserializer for the list of Iceberg Expressions | ||
@JsonDeserialize(using = IcebergExpressionListDeserializer.class) | ||
@JsonSerialize(using = IcebergExpressionListSerializer.class) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This binds Polaris API to internal serialization code in Iceberg. Iceberg changes in |
||
private List<Expression> rowFilters; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can this work "without context functions" (code comment above)? How will Polaris code interface with these expressions? |
||
|
||
private static final String DEFAULT_POLICY_SCHEMA_VERSION = "2025-02-03"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this date represent? |
||
private static final Set<String> POLICY_SCHEMA_VERSIONS = Set.of(DEFAULT_POLICY_SCHEMA_VERSION); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unused? |
||
|
||
public static AccessControlPolicyContent fromString(String content) { | ||
if (Strings.isNullOrEmpty(content)) { | ||
throw new InvalidPolicyException("Policy is empty"); | ||
} | ||
|
||
AccessControlPolicyContent policy; | ||
try { | ||
policy = PolicyContentUtil.MAPPER.readValue(content, AccessControlPolicyContent.class); | ||
} catch (Exception e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (design) Not all exceptions are worth catching? Should a |
||
throw new InvalidPolicyException(e); | ||
} | ||
|
||
boolean isProjectionsEmpty = | ||
policy.getColumnProjections() == null || policy.getColumnProjections().isEmpty(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (design) Maybe we can instruct Jackson to treat null values as empty collection and avoid null checks? |
||
boolean isRowFilterEmpty = policy.getRowFilters() == null || policy.getRowFilters().isEmpty(); | ||
if (isProjectionsEmpty && isRowFilterEmpty) { | ||
throw new InvalidPolicyException("Policy must contain 'columnProjections' or 'rowFilters'."); | ||
} | ||
|
||
return policy; | ||
} | ||
|
||
public static String toString(AccessControlPolicyContent content) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (design) Why a static method vs an instance method like |
||
if (content == null) { | ||
return null; | ||
} | ||
try { | ||
return PolicyContentUtil.MAPPER.writeValueAsString(content); | ||
} catch (JsonProcessingException e) { | ||
throw new InvalidPolicyException("Failed to convert policy content to JSON string", e); | ||
} | ||
} | ||
|
||
// Constructors, getters, and setters | ||
public AccessControlPolicyContent() {} | ||
|
||
public String getPrincipalRole() { | ||
return principalRole; | ||
} | ||
|
||
public void setPrincipalRole(String principalRole) { | ||
this.principalRole = principalRole; | ||
} | ||
|
||
public List<String> getColumnProjections() { | ||
return columnProjections; | ||
} | ||
|
||
public void setAllowedColumns(List<String> columnProjections) { | ||
this.columnProjections = columnProjections; | ||
} | ||
|
||
public List<Expression> getRowFilters() { | ||
return rowFilters; | ||
} | ||
|
||
public void setRowFilters(List<Expression> rowFilters) { | ||
this.rowFilters = rowFilters; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return "AccessControlPolicyContent{" | ||
+ "principalRole='" | ||
+ principalRole | ||
+ '\'' | ||
+ ", columnProjections=" | ||
+ columnProjections | ||
+ ", rowFilters=" | ||
+ rowFilters | ||
+ '}'; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.apache.polaris.core.policy.content; | ||
|
||
import com.fasterxml.jackson.core.JsonParser; | ||
import com.fasterxml.jackson.databind.DeserializationContext; | ||
import com.fasterxml.jackson.databind.JsonDeserializer; | ||
import com.fasterxml.jackson.databind.JsonNode; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import java.io.IOException; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import org.apache.iceberg.expressions.Expression; | ||
import org.apache.iceberg.expressions.ExpressionParser; | ||
|
||
public class IcebergExpressionListDeserializer extends JsonDeserializer<List<Expression>> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (design) I wonder why a serializer/deserializer pair for the list is needed vs having it for the elementy type ( |
||
@Override | ||
public List<Expression> deserialize(JsonParser p, DeserializationContext ctxt) | ||
throws IOException { | ||
ObjectMapper mapper = (ObjectMapper) p.getCodec(); | ||
JsonNode node = mapper.readTree(p); | ||
|
||
List<Expression> expressions = new ArrayList<>(); | ||
if (node.isArray()) { | ||
for (JsonNode element : node) { | ||
// Convert each JSON element back to a string and pass it to ExpressionParser.fromJson | ||
expressions.add(ExpressionParser.fromJson(mapper.writeValueAsString(element))); | ||
} | ||
} | ||
return expressions; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.apache.polaris.core.policy.content; | ||
|
||
import com.fasterxml.jackson.core.JsonGenerator; | ||
import com.fasterxml.jackson.databind.JsonSerializer; | ||
import com.fasterxml.jackson.databind.SerializerProvider; | ||
import java.io.IOException; | ||
import java.util.List; | ||
import org.apache.iceberg.expressions.Expression; | ||
import org.apache.iceberg.expressions.ExpressionParser; | ||
|
||
/** | ||
* Custom Jackson JsonSerializer for a List of Iceberg Expression objects. This serializer converts | ||
* each Iceberg Expression into its string representation. | ||
*/ | ||
public class IcebergExpressionListSerializer extends JsonSerializer<List<Expression>> { | ||
|
||
@Override | ||
public void serialize( | ||
List<Expression> expressions, | ||
JsonGenerator jsonGenerator, | ||
SerializerProvider serializerProvider) | ||
throws IOException { | ||
jsonGenerator.writeStartArray(); | ||
if (expressions != null) { | ||
for (Expression expression : expressions) { | ||
jsonGenerator.writeString(ExpressionParser.toJson(expression)); | ||
} | ||
} | ||
jsonGenerator.writeEndArray(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
|
||
import com.fasterxml.jackson.databind.DeserializationFeature; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import org.apache.iceberg.rest.RESTSerializers; | ||
|
||
public class PolicyContentUtil { | ||
public static final ObjectMapper MAPPER = configureMapper(); | ||
|
@@ -30,6 +31,8 @@ private static ObjectMapper configureMapper() { | |
mapper.configure(DeserializationFeature.FAIL_ON_MISSING_CREATOR_PROPERTIES, true); | ||
// Fails if a required field is present but explicitly null, e.g., {"enable": null} | ||
mapper.configure(DeserializationFeature.FAIL_ON_NULL_CREATOR_PROPERTIES, true); | ||
// This will make sure all the Iceberg parsers are loaded. | ||
RESTSerializers.registerAll(mapper); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could that cause conflicts? |
||
return mapper; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
import org.apache.polaris.core.entity.PolarisEntity; | ||
import org.apache.polaris.core.policy.PolicyEntity; | ||
import org.apache.polaris.core.policy.PredefinedPolicyTypes; | ||
import org.apache.polaris.core.policy.content.AccessControlPolicyContent; | ||
import org.apache.polaris.core.policy.content.maintenance.DataCompactionPolicyContent; | ||
import org.apache.polaris.core.policy.content.maintenance.MetadataCompactionPolicyContent; | ||
import org.apache.polaris.core.policy.content.maintenance.OrphanFileRemovalPolicyContent; | ||
|
@@ -66,6 +67,9 @@ public static void validate(PolicyEntity policy) { | |
case ORPHAN_FILE_REMOVAL: | ||
OrphanFileRemovalPolicyContent.fromString(policy.getContent()); | ||
break; | ||
case ACCESS_CONTROL: | ||
AccessControlPolicyContent.fromString(policy.getContent()); | ||
break; | ||
default: | ||
throw new IllegalArgumentException("Unsupported policy type: " + type.getName()); | ||
} | ||
|
@@ -98,6 +102,10 @@ public static boolean canAttach(PolicyEntity policy, PolarisEntity targetEntity) | |
case ORPHAN_FILE_REMOVAL: | ||
return BaseMaintenancePolicyValidator.INSTANCE.canAttach(entityType, entitySubType); | ||
|
||
case ACCESS_CONTROL: | ||
// TODO: Add validator for attaching this only to table | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for deferring this? |
||
return true; | ||
|
||
default: | ||
LOGGER.warn("Attachment not supported for policy type: {}", policyType.getName()); | ||
return false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(design) I would recommend to mark the class final as well
Adding javadoc to the class and the public methods would be good as well.