Skip to content

Commit

Permalink
Fixed DecisionStrategy not persisted
Browse files Browse the repository at this point in the history
- Implemented review comments

Signed-off-by: Theodor Mihalache <[email protected]>
  • Loading branch information
tmihalac committed Aug 1, 2024
1 parent 884a9d0 commit a86de1a
Show file tree
Hide file tree
Showing 15 changed files with 130 additions and 24 deletions.
5 changes: 5 additions & 0 deletions docs/getting-started/concepts/permission.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ The `Permission` class identifies a single permission configured on the feature
- `actions`: The actions authorized by this permission. Defaults to `ALL_VALUES`, an alias defined in the `action` module.
- `policy`: The policy to be applied to validate a client request.

The `decision strategy` parameter defines the strategy to be applied when multiple permissions match an execution request, it defaults to `UNANIMOUS`. and the possible values are:
- `UNANIMOUS`: All policies must evaluate to a positive decision for the final decision to be also positive.
- `AFFIRMATIVE`: At least one policy must evaluate to a positive decision
- `UNANIMOUS`: The number of positive decisions must be greater than the number of negative decisions.

Given the above definitions, the feature store can be configured with granular control over each resource, enabling partitioned access by
teams to meet organizational requirements for service and data sharing, and protection of sensitive information.

Expand Down
10 changes: 9 additions & 1 deletion protos/feast/registry/RegistryServer.proto
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ service RegistryServer{
rpc Refresh (RefreshRequest) returns (google.protobuf.Empty) {}
rpc Proto (google.protobuf.Empty) returns (feast.core.Registry) {}
rpc SetDecisionStrategy (SetDecisionStrategyRequest) returns (google.protobuf.Empty) {}

rpc GetDecisionStrategy (GetDecisionStrategyRequest) returns (GetDecisionStrategyResponse) {}
}

message RefreshRequest {
Expand Down Expand Up @@ -363,3 +363,11 @@ message SetDecisionStrategyRequest {
string project = 2;
bool commit = 3;
}

message GetDecisionStrategyRequest {
string project = 1;
}

message GetDecisionStrategyResponse {
feast.core.ProjectMetadata.DecisionStrategy decision_strategy = 1;
}
10 changes: 6 additions & 4 deletions sdk/python/feast/cli_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
)
from feast.feast_object import FeastObject
from feast.permissions.action import ALL_ACTIONS
from feast.permissions.decision import DecisionEvaluator
from feast.permissions.decision import DecisionEvaluator, DecisionStrategy
from feast.permissions.permission import Permission
from feast.permissions.policy import Policy, RoleBasedPolicy
from feast.permissions.user import User
Expand Down Expand Up @@ -291,9 +291,7 @@ def handler_list_all_permissions_roles_verbose(

if matching_permissions:
evaluator = DecisionEvaluator(
store.registry.list_project_metadata(project=store.project)[
0
].decision_strategy,
get_global_decision_strategy(store),
len(matching_permissions),
)
for p in matching_permissions:
Expand Down Expand Up @@ -321,3 +319,7 @@ def handler_list_all_permissions_roles_verbose(
[a.value.upper() for a in permitted_actions],
]
)


def get_global_decision_strategy(store: FeatureStore) -> DecisionStrategy:
return store.get_decision_strategy(store.project)
5 changes: 5 additions & 0 deletions sdk/python/feast/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -419,3 +419,8 @@ def __init__(self, query: str):
class ZeroColumnQueryResult(Exception):
def __init__(self, query: str):
super().__init__(f"This query returned zero columns:\n{query}")


class DecisionStrategyNotFound(FeastObjectNotFoundException):
def __init__(self, project: str):
super().__init__(f"DecisionStrategy does not exist in project {project}")
12 changes: 12 additions & 0 deletions sdk/python/feast/feature_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -2015,6 +2015,18 @@ def set_decision_strategy(self, decision_strategy: DecisionStrategy):
"""
return self._registry.set_decision_strategy(self.project, decision_strategy)

def get_decision_strategy(self, project: str) -> DecisionStrategy:
"""
Get the project decision strategy.
Args:
project: The project to get the decision strategy for.
Returns:
The decision strategy or None.
"""
return self._registry.get_decision_strategy(project)


def _print_materialization_log(
start_date, end_date, num_feature_views: int, online_store: str
Expand Down
12 changes: 12 additions & 0 deletions sdk/python/feast/infra/registry/base_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -824,3 +824,15 @@ def set_decision_strategy(self, project: str, decision_strategy: DecisionStrateg
decision_strategy: The decision strategy to set for the project
"""
raise NotImplementedError

def get_decision_strategy(self, project: str) -> DecisionStrategy:
"""
Get the project decision strategy.
Args:
project: The project to get the decision strategy for.
Returns:
The decision strategy or None.
"""
raise NotImplementedError
16 changes: 16 additions & 0 deletions sdk/python/feast/infra/registry/registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from feast.errors import (
ConflictingFeatureViewNames,
DataSourceNotFoundException,
DecisionStrategyNotFound,
EntityNotFoundException,
FeatureServiceNotFoundException,
FeatureViewNotFoundException,
Expand Down Expand Up @@ -987,3 +988,18 @@ def set_decision_strategy(self, project: str, decision_strategy: DecisionStrateg
registry.project_metadata.append(project_metadata)

self.commit()

def get_decision_strategy(self, project: str) -> DecisionStrategy:
registry_proto = self._get_registry_proto(project=project)

for idx, existing_project_metadata in enumerate(
registry_proto.project_metadata
):
if existing_project_metadata.project == project:
return DecisionStrategy[
ProjectMetadataProto.DecisionStrategy.Name(
existing_project_metadata.decision_strategy
)
]

raise DecisionStrategyNotFound(project=project)
11 changes: 11 additions & 0 deletions sdk/python/feast/infra/registry/remote.py
Original file line number Diff line number Diff line change
Expand Up @@ -698,3 +698,14 @@ def set_decision_strategy(self, project: str, decision_strategy: DecisionStrateg
self.stub.SetDecisionStrategy(request=request, metadata=metadata)
else:
self.stub.SetDecisionStrategy(request)

def get_decision_strategy(self, project: str) -> DecisionStrategy:
request = RegistryServer_pb2.GetDecisionStrategyRequest(project=project)

if self.auth_config.type is not AuthType.NONE.value:
metadata = create_auth_header(self.auth_config)
response = self.stub.GetDecisionStrategy(request=request, metadata=metadata)
else:
response = self.stub.GetDecisionStrategy(request)

return DecisionStrategy(response)
23 changes: 23 additions & 0 deletions sdk/python/feast/infra/registry/snowflake.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from feast.entity import Entity
from feast.errors import (
DataSourceObjectNotFoundException,
DecisionStrategyNotFound,
EntityNotFoundException,
FeatureServiceNotFoundException,
FeatureViewNotFoundException,
Expand Down Expand Up @@ -1161,6 +1162,8 @@ def commit(self):
pass

def set_decision_strategy(self, project: str, decision_strategy: DecisionStrategy):
self._maybe_init_project_metadata(project)

with GetSnowflakeConnection(self.registry_config) as conn:
query = f"""
UPDATE {self.registry_path}."FEAST_METADATA"
Expand All @@ -1174,3 +1177,23 @@ def set_decision_strategy(self, project: str, decision_strategy: DecisionStrateg
AND metadata_key = '{FeastMetadataKeys.PERMISSIONS_DECISION_STRATEGY.value}',
"""
execute_snowflake_statement(conn, query)

def get_decision_strategy(self, project: str) -> DecisionStrategy:
with GetSnowflakeConnection(self.registry_config) as conn:
query = f"""
SELECT
metadata_value
FROM
{self.registry_path}."FEAST_METADATA"
WHERE
project_id = '{project}'
AND metadata_key = '{FeastMetadataKeys.PERMISSIONS_DECISION_STRATEGY.value}'
LIMIT 1
"""
df = execute_snowflake_statement(conn, query).fetch_pandas_all()

if not df.empty:
for row in df.iterrows():
return DecisionStrategy(row[1]["METADATA_VALUE"])

raise DecisionStrategyNotFound(project=project)
16 changes: 16 additions & 0 deletions sdk/python/feast/infra/registry/sql.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from feast.entity import Entity
from feast.errors import (
DataSourceObjectNotFoundException,
DecisionStrategyNotFound,
EntityNotFoundException,
FeatureServiceNotFoundException,
FeatureViewNotFoundException,
Expand Down Expand Up @@ -1004,6 +1005,8 @@ def delete_permission(self, name: str, project: str, commit: bool = True):
raise PermissionNotFoundException(name, project)

def set_decision_strategy(self, project: str, decision_strategy: DecisionStrategy):
self._maybe_init_project_metadata(project)

with self.engine.begin() as conn:
values = {
"metadata_key": FeastMetadataKeys.PERMISSIONS_DECISION_STRATEGY.value,
Expand All @@ -1023,3 +1026,16 @@ def set_decision_strategy(self, project: str, decision_strategy: DecisionStrateg
)

conn.execute(update_stmt)

def get_decision_strategy(self, project: str) -> DecisionStrategy:
with self.engine.begin() as conn:
stmt = select(feast_metadata).where(
feast_metadata.c.metadata_key
== FeastMetadataKeys.PERMISSIONS_DECISION_STRATEGY.value,
feast_metadata.c.project_id == project,
)
row = conn.execute(stmt).first()
if row:
return DecisionStrategy(row._mapping["metadata_value"])

raise DecisionStrategyNotFound(project=project)
4 changes: 2 additions & 2 deletions sdk/python/feast/permissions/decision.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class DecisionEvaluator:
Create the instance and specify the strategy and number of decisions:
`evaluator = DecisionEvaluator(DecisionStrategy.UNANIMOUS, 3)
For each vote that you receivem, add a decision grant: `evaluator.add_grant(vote, message)`
For each vote that you receive, add a decision grant: `evaluator.add_grant(vote, message)`
and check if the decision process ended: `if evaluator.is_decided():`
Once decided, get the result and the failure explanations using:
`grant, explanations = evaluator.grant()`
Expand Down Expand Up @@ -86,7 +86,7 @@ def grant(self) -> tuple[bool, list[str]]:
def add_grant(self, grant: bool, explanation: str):
"""
Add a single vote to the decision computation, with a possible denial reason.
If the evalluation process already ended, additional votes are discarded.
If the evaluation process already ended, additional votes are discarded.
Args:
grant: `True` is the decision is accepted, `False` otherwise.
Expand Down
4 changes: 1 addition & 3 deletions sdk/python/feast/permissions/security_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,7 @@ def decision_strategy(self) -> DecisionStrategy:
Returns:
DecisionStrategy: the DecisionStrategy configured in the Feast registry.
"""
return self._registry.list_project_metadata(project=self._project)[
0
].decision_strategy
return self._registry.get_decision_strategy(project=self._project)

def assert_permissions(
self,
Expand Down
5 changes: 5 additions & 0 deletions sdk/python/feast/registry_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,11 @@ def SetDecisionStrategy(
)
return Empty()

def GetDecisionStrategy(
self, request: RegistryServer_pb2.GetDecisionStrategyRequest, context
):
return self.proxied_registry.get_decision_strategy(project=request.project)


def start_server(store: FeatureStore, port: int, wait_for_termination: bool = True):
auth_manager_type = str_to_auth_manager_type(store.config.auth_config.type)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1528,21 +1528,12 @@ def test_set_decision_strategy_success(test_registry):
# Register Entity
test_registry.apply_entity(entity, project)

project_metadata = test_registry.list_project_metadata(project=project)
assert len(project_metadata) == 1
project_uuid = project_metadata[0].project_uuid
assert len(project_metadata[0].project_uuid) == 36
assert_project_uuid(
project, project_uuid, DecisionStrategy.UNANIMOUS, test_registry
)
decision_strategy = test_registry.get_decision_strategy(project=project)
assert decision_strategy == DecisionStrategy.UNANIMOUS

test_registry.set_decision_strategy(project, DecisionStrategy.AFFIRMATIVE)
project_metadata = test_registry.list_project_metadata(project=project)
assert len(project_metadata) == 1
project_uuid = project_metadata[0].project_uuid
assert len(project_metadata[0].project_uuid) == 36
assert_project_uuid(
project, project_uuid, DecisionStrategy.AFFIRMATIVE, test_registry
)

decision_strategy = test_registry.get_decision_strategy(project=project)
assert decision_strategy == DecisionStrategy.AFFIRMATIVE

test_registry.teardown()
2 changes: 2 additions & 0 deletions sdk/python/tests/unit/permissions/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from feast import FeatureView
from feast.infra.registry.base_registry import BaseRegistry
from feast.permissions.decision import DecisionStrategy
from feast.permissions.decorator import require_permissions
from feast.permissions.permission import AuthzedAction, Permission
from feast.permissions.policy import RoleBasedPolicy
Expand Down Expand Up @@ -89,6 +90,7 @@ def security_manager() -> SecurityManager:
registry = Mock(spec=BaseRegistry)
registry.list_permissions = Mock(return_value=permissions)
registry.list_project_metadata = Mock(return_value=[project_metadata])
registry.get_decision_strategy = Mock(return_value=DecisionStrategy.UNANIMOUS)
sm = SecurityManager(project="any", registry=registry)
set_security_manager(sm)
return sm

0 comments on commit a86de1a

Please sign in to comment.