Skip to content

Check security properties in the main transaction#2300

Merged
nickva merged 3 commits intoprototype/fdb-layerfrom
check-security-in-transaction
Nov 14, 2019
Merged

Check security properties in the main transaction#2300
nickva merged 3 commits intoprototype/fdb-layerfrom
check-security-in-transaction

Conversation

@nickva
Copy link
Copy Markdown
Contributor

@nickva nickva commented Nov 5, 2019

Previously we checked security properties in a separate transaction, after
opening the db or fetching it from the cache. To avoid running an extra
transaction move the check inside the main transaction right after the metadata
check runs. That ensure it will be consistent and it won't be accidentally
missed as all operations run the ensure_current metadata check.

Also remove the special get_config/2 function in fabric2_fdb for getting
revs limit and security properties and just read them directly from the db map.

Test with:

make && make eunit apps=fabric,couch_views,couch_jobs

make && make elixir tests=test/elixir/test/basics_test.exs,test/elixir/test/replication_test.exs,test/elixir/test/map_test.exs,test/elixir/test/all_docs_test.exs,test/elixir/test/bulk_docs_test.exs

@nickva nickva force-pushed the check-security-in-transaction branch from 8e601a6 to c8d152c Compare November 5, 2019 19:37
Previously we checked security properties in a separate transaction, after
opening the db or fetching it from the cache. To avoid running an extra
transaction move the check inside the main transaction right after the metadata
check runs. That ensure it will be consistent and it won't be accidentally
missed as all operations run the `ensure_current` metadata check.

Also remove the special `get_config/2` function in `fabric2_fdb` for getting
revs limit and security properties and just read them directly from the db map.
@nickva nickva force-pushed the check-security-in-transaction branch 5 times, most recently from d601c59 to 91cca43 Compare November 13, 2019 20:59
@nickva
Copy link
Copy Markdown
Contributor Author

nickva commented Nov 13, 2019

Quick benchmark with this PR:

prototype/fdb-layer (FDB master):

time ./add_docs.py
> db url: http://adm:pass@127.0.0.1:15984
> db:info {u'update_seq': u'00000002313b676000000000', u'disk_size': 0, u'sizes': {u'active': 0, u'external': 2, u'file': 0}, u'purge_seq': 0, u'doc_count': 10000, u'compact_running': False, u'cluster': {u'q': 0, u'r': 0, u'w': 0, u'n': 0}, u'db_name': u'db', u'disk_format_version': 0, u'instance_start_time': u'0', u'other': {u'data_size': 2}, u'data_size': 0, u'doc_del_count': 0}

real	0m55.825s
user	0m10.583s
sys	0m3.986s

This PR:

time ./add_docs.py
> db url: http://adm:pass@127.0.0.1:15984
> db:info {u'update_seq': u'00000002313b676000000000', u'disk_size': 0, u'sizes': {u'active': 0, u'external': 2, u'file': 0}, u'purge_seq': 0, u'doc_count': 10000, u'compact_running': False, u'cluster': {u'q': 0, u'r': 0, u'w': 0, u'n': 0}, u'db_name': u'db', u'disk_format_version': 0, u'instance_start_time': u'0', u'other': {u'data_size': 2}, u'data_size': 0, u'doc_del_count': 0}

real	0m34.587s
user	0m10.486s
sys	0m4.118s

add_docs.py: https://gist.github.com/nickva/5d88c389d45ab538950a5e0f34b7d2ae

First ran script once to create and populate db then run again to read the documents

With the chttpd_auth_request security check back in got:

real	0m44.132s
user	0m11.676s
sys	0m4.437s

Copy link
Copy Markdown
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

Looks good overall. Few questions though.

Previously, a stale db handle could be re-used across a few separate
transactions. That would result in the database getting re-opened before every
one of those operations.

To prevent that from happening, check the cache before the transaction starts,
and if there is a newer version of the db handle and use that.
Previously `set_config/3` needed keys and values to be transalted to binaries,
now that is done inside the function. It's a bit more consistent as binary
config values and encodings are better encapsulated in the `fabric2_fdb`
module.

Since `set_config` does, it made sense to update get_config as well. There, it
turns out it was used only to load configuration setting after a db open, so
the function was renamed to `load_config` and was made private.
@nickva nickva force-pushed the check-security-in-transaction branch from 91cca43 to 5e55060 Compare November 14, 2019 18:08
Copy link
Copy Markdown
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

One minor nit, but +1 once you tweak the map update operations.

@nickva nickva merged commit 44f660f into prototype/fdb-layer Nov 14, 2019
@nickva nickva deleted the check-security-in-transaction branch November 14, 2019 23:09
nickva added a commit that referenced this pull request Nov 14, 2019
Forgot to push this in the previous PR so made a new commit.

#2300 (comment)
nickva added a commit that referenced this pull request Nov 14, 2019
Forgot to push this in the previous PR so made a new commit.

#2300 (comment)
davisp pushed a commit that referenced this pull request Feb 26, 2020
Forgot to push this in the previous PR so made a new commit.

#2300 (comment)
davisp pushed a commit that referenced this pull request Feb 27, 2020
Forgot to push this in the previous PR so made a new commit.

#2300 (comment)
davisp pushed a commit that referenced this pull request Mar 2, 2020
Forgot to push this in the previous PR so made a new commit.

#2300 (comment)
davisp pushed a commit that referenced this pull request Mar 2, 2020
Forgot to push this in the previous PR so made a new commit.

#2300 (comment)
davisp pushed a commit that referenced this pull request Sep 9, 2020
Forgot to push this in the previous PR so made a new commit.

#2300 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants