You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2020/05/07 13:55:48 UTC
[GitHub] [couchdb] eiri opened a new pull request #2875: Convert aegis key cach to LRU with hard expiration time
eiri opened a new pull request #2875:
URL: https://github.com/apache/couchdb/pull/2875
## Overview
This converts aegis's key cache into LRU with hard expiration time.
The cache evicts the least recently used entries when number of entries in the cache exceeds "cache_limit" parameter with default set to 100000.
Additionally keys removed once their lifetime exceeds "cache_max_age_sec" parameter with default set to 30 minutes.
## Testing recommendations
` make eunit apps=aegis suites=aegis_server_test tests=lru_cache_with_expiration_test_`
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
- [x] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] eiri commented on a change in pull request #2875: Convert aegis key cach to LRU with hard expiration time
Posted by GitBox <gi...@apache.org>.
eiri commented on a change in pull request #2875:
URL: https://github.com/apache/couchdb/pull/2875#discussion_r421665881
##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -95,7 +101,7 @@ encrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
{error, Reason} ->
erlang:error(Reason)
end;
- false ->
+ _ ->
Review comment:
Unlike epep we can't refresh keys async, because refreshing requires reading a wrapped key from fdb, i.e. it must be done in the same process as the transaction. This means that we are going to have this high latency regardless if we removing key and fetching it on the next read or fetching it proactively once it's "stale".
The only way to avoid this is to add another server process to aegis that'll do refresh of the stale keys in its own separate transactions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on pull request #2875: Convert aegis key cach to LRU with hard expiration time
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #2875:
URL: https://github.com/apache/couchdb/pull/2875#issuecomment-625422560
we've agreed to revisit the eviction pattern in a new PR. Would like to avoid evicting only to immediately repopulate on the next request but needs thought.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a change in pull request #2875: Convert aegis key cach to LRU with hard expiration time
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2875:
URL: https://github.com/apache/couchdb/pull/2875#discussion_r421696145
##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -276,6 +279,15 @@ do_decrypt(DbKey, #{uuid := UUID}, Key, Value) ->
end.
+is_key_present(UUID) ->
Review comment:
this returns false if the key is present but expired. `is_key_fresh`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] eiri commented on a change in pull request #2875: Convert aegis key cach to LRU with hard expiration time
Posted by GitBox <gi...@apache.org>.
eiri commented on a change in pull request #2875:
URL: https://github.com/apache/couchdb/pull/2875#discussion_r421665163
##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -120,7 +128,7 @@ decrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
{error, Reason} ->
erlang:error(Reason)
end;
- false ->
+ _ ->
process_flag(sensitive, true),
Review comment:
Good call! I've added it in `insert_key` handler called from `do_open_db` to avoid second gen_call.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a change in pull request #2875: Convert aegis key cach to LRU with hard expiration time
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2875:
URL: https://github.com/apache/couchdb/pull/2875#discussion_r421582281
##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -197,10 +205,21 @@ handle_call(_Msg, _From, St) ->
{noreply, St}.
+handle_cast({accessed, UUID}, St) ->
+ NewSt = bump_last_accessed(St, UUID),
+ {noreply, NewSt};
+
+
handle_cast(_Msg, St) ->
{noreply, St}.
+handle_info(maybe_remove_expired, St) ->
+ remove_expired_entries(St),
+ CheckInterval = timer:seconds(expiration_check_interval()),
Review comment:
there's a cleaner api for time unit conversion. is this done for compatibility reasons? (it's not clear from the funciton name that it does seconds to milliseconds)
##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -109,8 +115,10 @@ decrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
uuid := UUID
} = Db,
- case ets:member(?KEY_CHECK, UUID) of
- true ->
+ Now = fabric2_util:now(sec),
+
+ case ets:lookup(?KEY_CHECK, UUID) of
+ [{UUID, ExpiresAt}] when ExpiresAt >= Now ->
Review comment:
this pattern is reproduced twice in the module, can we extract it?
##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -120,7 +128,7 @@ decrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
{error, Reason} ->
erlang:error(Reason)
end;
- false ->
+ _ ->
process_flag(sensitive, true),
Review comment:
shouldn't we evict the entry here?
##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -95,7 +101,7 @@ encrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
{error, Reason} ->
erlang:error(Reason)
end;
- false ->
+ _ ->
Review comment:
Now that I look at it, we'll be exposing users to a high latency period every 30 minutes that they _don't_ revoke access to their key, which we expect to be the case almost every time.
Like in epep, we had "fresh" "stale" and "expired" to cover this. A fresh item is used directly, as your first clause does. An "expired" one is not used (gets removed from cache if present, expensive external call must happen again). but a "stale" entry _is_ used and also kicks off a check that we still have access (if we do, nothing happens, if we don't, we evict our entry).
##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -259,17 +278,125 @@ do_decrypt(DbKey, #{uuid := UUID}, Key, Value) ->
%% cache functions
-insert(Cache, UUID, DbKey) ->
- Entry = #entry{uuid = UUID, encryption_key = DbKey},
+insert(St, UUID, DbKey) ->
+ #{
+ cache := Cache,
+ by_access := ByAccess,
+ counter := Counter
+ } = St,
+
+ Now = fabric2_util:now(sec),
+ ExpiresAt = Now + max_age(),
+
+ Entry = #entry{
+ uuid = UUID,
+ encryption_key = DbKey,
+ counter = Counter,
+ last_accessed = Now,
+ expires_at = ExpiresAt
+ },
+
true = ets:insert(Cache, Entry),
- true = ets:insert(?KEY_CHECK, {UUID, true}),
- ok.
+ true = ets:insert_new(ByAccess, Entry),
+ true = ets:insert(?KEY_CHECK, {UUID, ExpiresAt}),
+
+ maybe_evict_old_entries(St),
Review comment:
how can this ever be more than 1 item? "old" here is misleading as it implies time is a factor, but we're really evicted the least recently used item to make way for this one. It seems we could make that clearer by checking ets:size here and removing the (single) oldest item if it goes over.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org