fix(gs-cache): cleanup cache usage, don't try to save key_db_entry no… #1037

Avantol13 · 2022-08-26T16:56:18Z

New Features

Breaking Changes

Bug Fixes

Fix intermittent issue with db cache containing info that can't be loaded into JSON

Improvements

Dependency updates

Update poetry install in travis.yml due to https://python-poetry.org/blog/announcing-poetry-1.2.0a1/#deprecation-of-the-get-poetrypy-script

Deployment changes

…w that we already have expiration

github-actions · 2022-08-26T16:56:46Z

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

coveralls · 2022-08-26T17:03:10Z

Pull Request Test Coverage Report for Build 12795

4 of 7 (57.14%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.003%) to 73.907%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
fence/blueprints/data/indexd.py	4	7	57.14%

Files with Coverage Reduction	New Missed Lines	%
fence/blueprints/data/indexd.py	1	93.84%

Totals
Change from base Build 12686:	-0.003%
Covered Lines:	6866
Relevant Lines:	9290

💛 - Coveralls

Avantol13 · 2022-09-01T20:00:55Z

fence/blueprints/data/indexd.py

                key_db_entry.expires,
            )

            db_entry = {}
            db_entry["gcp_proxy_group_id"] = proxy_group_id
-            db_entry["gcp_private_key"] = json.dumps(str(private_key))
-            db_entry["gcp_key_db_entry"] = str(key_db_entry)
+            db_entry["gcp_private_key"] = str(private_key)


why don't we just json.dumps this and then json.loads should work without the need to do that string manipulation?

>>> import json >>> test = {"foo": "bar"} >>> db_entry = json.dumps(test) >>> db_entry '{"foo": "bar"}' >>> json.loads(db_entry) {'foo': 'bar'}

okay yes, done. Tested it out and its working

Avantol13 · 2022-09-01T20:01:09Z

fence/blueprints/data/indexd.py

-                        json.loads(cache.gcp_key_db_entry),
-                        cache.expires_at,
+                    private_key = json.loads(
+                        str(cache.gcp_private_key).replace("'", '"')


I'm not sure I understand why this is necessary now. See next comment

Avantol13 · 2022-09-01T20:02:35Z

tests/data/test_indexed_file.py

-                keydbentry = UserGoogleAccountToProxyGroup()
-                keydbentry.expires = 10
-                google_object._assume_role_cache_gs = {"1": ("key", keydbentry, 10)}
+                google_object._assume_role_cache_gs = {"1": ("key", 10)}


we could probably add some more unit tests here to regression test this? What do you think , I'm not sure we have to have integration tests

Avantol13 · 2022-09-06T16:44:47Z

tests/data/test_indexed_file.py

+            ):
+                indexed_file = IndexedFile(file_id="some id")
+                google_object = GoogleStorageIndexedFileLocation("gs://some/location")
+                google_object._assume_role_cache_gs = {"1": ("key", 10)}


maybe we should use a dictionary as the key to better simulate the real world data (and hopefully catch any issues with the JSON parsing)

Avantol13 · 2022-09-06T16:45:17Z

tests/data/test_indexed_file.py

+                google_object = GoogleStorageIndexedFileLocation("gs://some/location")
+                google_object._assume_role_cache_gs = {"1": ("key", 10)}
+
+                assert google_object._assume_role_cache_gs


this isn't really necessary, b/c you set it in the test, we're not really asserting behavior of the unit under test

Avantol13 · 2022-09-06T16:47:46Z

tests/data/test_indexed_file.py

+                after_cache = db_session.query(AssumeRoleCacheGCP).first()
+
+                assert after_cache
+                assert before_cache != after_cache


Why do we expect these to be different? Also I think we should test the actual contents of the cache, make sure it's a dictionary or JSON (whatever we actually expect in the code)

Avantol13 · 2022-09-06T16:47:56Z

tests/data/test_indexed_file.py

+    create presigned url again
+        make sure cache is set correctly
+    """
+    # db_session.add(


should this be commented out?

Avantol13 · 2022-09-06T20:26:56Z

tests/data/test_indexed_file.py

+
+                assert after_cache
+                assert (
+                    str(type(after_cache))


I'm not sure this assert is worth having. The important thing is that we can json load properly, this type check seems unecessary

Avantol13 · 2022-09-06T20:27:36Z

tests/data/test_indexed_file.py

+                )
+                # check if json loads can properly parse json string stored in cache
+                assert (
+                    str(type(json.loads(after_cache.gcp_private_key)))


I don't think we need this type check either. The assert below will make sure the json.loads works and creates a comparable object to the mock we provided. So I'd remove this line

Avantol13 · 2022-09-06T20:29:06Z

tests/data/test_indexed_file.py

+                assert json.loads(after_cache.gcp_private_key) == sa_private_key
+
+                db_session.delete(after_cache)
+                cleared_cache = db_session.query(AssumeRoleCacheGCP).first()


This isn't the regression test we want, it's not bad to keep this too, but the test we need is:

Generate url

Simulate pod rolling (e.g. the in memory cache being cleared, not the database one)

Generate url, ensure new in memory cache has valid info from the db

Avantol13 · 2022-09-06T20:31:19Z

tests/data/test_indexed_file.py

+
+                assert redo_cache
+                assert (
+                    str(type(redo_cache)) == "<class 'fence.models.AssumeRoleCacheGCP'>"


see comments from above. I think we should remove all these type checks and only leave the json.loads assert. And we may want to do something like this to capture the failure correction:

try: assert json.loads(redo_cache.gcp_private_key) == sa_private_key except Exception: pytest.fail("Could not json.loads(cache)")

Avantol13 · 2022-09-06T21:08:56Z

tests/data/test_indexed_file.py

+                    r_pays_project=None,
+                )
+
+                try:


you only need the try catch around the json.loads

or I guess this makes sense, in case it never got added into the cache. But I'd just assert that explicitly. The only reason for this pattern was b/c the json.loads itself can fail in the test

so here you can just

assert "1" in google_object._assume_role_cache_gs assert len(google_object._assume_role_cache_gs["1"]) > 1 assert google_object._assume_role_cache_gs["1"][0] == sa_private_key

and remove the try/except

Oh, saw these comments after i had added the try-catch for the json.loads. Should i still do this? My reasoning was, whatever is stored in the database should be json.loads-able. Which would be assert google_object._assume_role_cache_gs["1"][0] == sa_private_key

I suppose that would be cleaner to do. Okay, convinced writing this commend that I shoukd just do those asserts.

tests/data/test_indexed_file.py

fix(gs-cache): cleanup cache usage, don't try to save key_db_entry no…

6b1d42d

…w that we already have expiration

BinamB added 9 commits August 30, 2022 10:59

remove expires at tuple

bd3be06

json formatting

734f827

Fix json

2358fd3

fix json

250b8e0

dumps

4f7f72e

revert and fix json storage

a425862

debug

2840843

clean up

3f42d53

fix cache

577f403

Avantol13 commented Sep 1, 2022

View reviewed changes

BinamB added 2 commits September 1, 2022 15:13

resolve

6dd441b

add unit test

c370f30

Avantol13 commented Sep 6, 2022

View reviewed changes

BinamB added 5 commits September 6, 2022 12:00

prints

7097e67

add json parse test

aba35dd

update travis for poetry

fdaa353

add poetry shell

c95a053

remove shell

0604f9a

Avantol13 commented Sep 6, 2022

View reviewed changes

BinamB added 2 commits September 6, 2022 15:49

add in-mem test

34aad86

add try-except

dbe59f7

Avantol13 commented Sep 6, 2022

View reviewed changes

BinamB added 2 commits September 6, 2022 16:14

fix try catch

5682eda

change tests

baa899e

Avantol13 commented Sep 7, 2022

View reviewed changes

tests/data/test_indexed_file.py Outdated Show resolved Hide resolved

Avantol13 and others added 2 commits September 7, 2022 10:27

Update tests/data/test_indexed_file.py

417c389

remove redundant assert

428a2a1

BinamB approved these changes Sep 7, 2022

View reviewed changes

PlanXCyborg added the test-pidgin-getCoremetadataTest label Sep 7, 2022

BinamB merged commit d05f94c into master Sep 7, 2022

BinamB deleted the fix/gs-cache branch September 7, 2022 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gs-cache): cleanup cache usage, don't try to save key_db_entry no… #1037

fix(gs-cache): cleanup cache usage, don't try to save key_db_entry no… #1037

Avantol13 commented Aug 26, 2022 •

edited by BinamB

Loading

github-actions bot commented Aug 26, 2022 •

edited

Loading

coveralls commented Aug 26, 2022 •

edited

Loading

Avantol13 Sep 1, 2022

BinamB Sep 1, 2022

Avantol13 Sep 1, 2022

Avantol13 Sep 1, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

Avantol13 Sep 6, 2022

BinamB Sep 7, 2022 •

edited

Loading

fix(gs-cache): cleanup cache usage, don't try to save key_db_entry no… #1037

fix(gs-cache): cleanup cache usage, don't try to save key_db_entry no… #1037

Conversation

Avantol13 commented Aug 26, 2022 • edited by BinamB Loading

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

github-actions bot commented Aug 26, 2022 • edited Loading

coveralls commented Aug 26, 2022 • edited Loading

Pull Request Test Coverage Report for Build 12795

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BinamB Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Avantol13 commented Aug 26, 2022 •

edited by BinamB

Loading

github-actions bot commented Aug 26, 2022 •

edited

Loading

coveralls commented Aug 26, 2022 •

edited

Loading

BinamB Sep 7, 2022 •

edited

Loading