Skip to content

Fix sslib new versions incompatibility bug - #159

Merged
kairoaraujo merged 9 commits into
repository-service-tuf:mainfrom
MVrachev:fix-sslib-compatibility-issue
Jan 23, 2023
Merged

kairoaraujo merged 9 commits into
repository-service-tuf:mainfrom
MVrachev:fix-sslib-compatibility-issue

Conversation

@MVrachev

@MVrachev MVrachev commented Jan 5, 2023

Copy link
Copy Markdown
Member

The incompatibility with newer securesystemslib versions was caused because of a new breaking change introduced in:
secure-systems-lab/securesystemslib#231

The changes were tested locally and the bootstrap works.

It seems that Pipenv.lock wasn't updated for a while as many changes not related were added.
I ran pipenv lock and on the time of submitting this pr on the latest main 248 lines of insertions and 200 lines of deletion are introduced in Pipenv.lock.

I added unit tests testing the new restrict argument to the LocalStorage.put() call which is used by default.
Also, updated the old tests to be consistent with the newer code.

Closes #118

Signed-off-by: Martin Vrachev mvrachev@vmware.com

@MVrachev

MVrachev commented Jan 6, 2023

Copy link
Copy Markdown
Member Author

There seems to be a lint failure. I will work to fix it today.

@MVrachev
MVrachev force-pushed the fix-sslib-compatibility-issue branch 2 times, most recently from b30b6e3 to b7d1dc7 Compare January 9, 2023 15:45
@MVrachev
MVrachev requested a review from kairoaraujo January 9, 2023 15:52
@codecov-commenter

codecov-commenter commented Jan 9, 2023

Copy link
Copy Markdown

Codecov Report

Base: 99.42% // Head: 99.05% // Decreases project coverage by -0.37% ⚠️

Coverage data is based on head (b33c1f2) compared to base (7225eff).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   99.42%   99.05%   -0.37%     
==========================================
  Files          12       12              
  Lines         522      530       +8     
==========================================
+ Hits          519      525       +6     
- Misses          3        5       +2     
Impacted Files Coverage Δ
repository_service_tuf_worker/interfaces.py 100.00% <100.00%> (ø)
repository_service_tuf_worker/repository.py 98.88% <100.00%> (+0.01%) ⬆️
...itory_service_tuf_worker/services/storage/local.py 96.22% <100.00%> (-3.78%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment thread requirements.txt Outdated
@@ -1,5 +1,5 @@
-i https://pypi.org/simple
alembic==1.8.1

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

When I updated the securesystemslib I noticed that tox -e requirements failed suddenly.
I thought it was somehow connected to the newer securesytemslib versions but in their requirements files, I don't see those dependencies...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I merged #156
If you rebase it now, the changes only related to your PR will be clear.

@MVrachev
MVrachev force-pushed the fix-sslib-compatibility-issue branch from b88a3a8 to f8a3348 Compare January 11, 2023 12:40
@MVrachev

Copy link
Copy Markdown
Member Author

Rebased and it's ready for review.

@kairoaraujo

Copy link
Copy Markdown
Member

Can you please rebase again?

@MVrachev
MVrachev force-pushed the fix-sslib-compatibility-issue branch from f8a3348 to b3281b8 Compare January 18, 2023 19:37
@MVrachev

Copy link
Copy Markdown
Member Author

@kairoaraujo I rebased.

kairoaraujo
kairoaraujo previously approved these changes Jan 19, 2023

@kairoaraujo kairoaraujo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM
nit: You can update the dependencies, sslib 0.26.0 was released, or let our workflow update after the merge.

$ pipenv update
$ make requirements
$  git diff
diff --git a/Pipfile.lock b/Pipfile.lock
index 3b5333c..4855f21 100644
--- a/Pipfile.lock
+++ b/Pipfile.lock
@@ -567,11 +567,11 @@
         },
         "securesystemslib": {
             "hashes": [
-                "sha256:04bc11593edd68405939d3dfc318080bfb31f1ebb5d81c7911914b42dfd4bf2f",
-                "sha256:10d5a066e70cb87704c9bf2cef1ef6d8a06fab5ef7602dd59c26d06251317a11"
+                "sha256:41c7b25c52dc0bafe774413b5738bbf4431f094e72a091e83d9921901972ae4c",
+                "sha256:a8fa49831d6a7e48f81050984ddfac3713af0c326f558727113533edb5ca8eac"
             ],
             "index": "pypi",
-            "version": "==0.25.0"
+            "version": "==0.26.0"
         },
         "setuptools": {
             "hashes": [
@@ -1551,11 +1551,11 @@
         },
         "securesystemslib": {
             "hashes": [
-                "sha256:04bc11593edd68405939d3dfc318080bfb31f1ebb5d81c7911914b42dfd4bf2f",
-                "sha256:10d5a066e70cb87704c9bf2cef1ef6d8a06fab5ef7602dd59c26d06251317a11"
+                "sha256:41c7b25c52dc0bafe774413b5738bbf4431f094e72a091e83d9921901972ae4c",
+                "sha256:a8fa49831d6a7e48f81050984ddfac3713af0c326f558727113533edb5ca8eac"
             ],
             "index": "pypi",
-            "version": "==0.25.0"
+            "version": "==0.26.0"
         },
         "setuptools": {
             "hashes": [
diff --git a/requirements-dev.txt b/requirements-dev.txt
index 4c49be2..c969b33 100644
--- a/requirements-dev.txt
+++ b/requirements-dev.txt
@@ -59,7 +59,7 @@ pytz==2022.7.1
 pyyaml==6.0 ; python_version >= '3.6'
 redis==4.4.2
 requests==2.28.2 ; python_version >= '3.7' and python_version < '4'
-securesystemslib==0.25.0
+securesystemslib==0.26.0
 setuptools==66.0.0 ; python_version >= '3.7'
 six==1.16.0 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'
 snowballstemmer==2.2.0
diff --git a/requirements.txt b/requirements.txt
index f7b9136..a7a7973 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -27,7 +27,7 @@ pynacl==1.5.0
 pytz==2022.7.1
 redis==4.4.2
 requests==2.28.2 ; python_version >= '3.7' and python_version < '4'
-securesystemslib==0.25.0
+securesystemslib==0.26.0
 setuptools==66.0.0 ; python_version >= '3.7'
 six==1.16.0 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'
 sqlalchemy==1.4.46

@MVrachev

Copy link
Copy Markdown
Member Author

Done, updated to securesystemslib 0.26.0

@MVrachev

Copy link
Copy Markdown
Member Author

@kairoaraujo suddenly, after the update to securesystemslib 0.26.0 the tests started to fail and the main problem seems to be they can't load the settings:

  >           settings.STORAGE = settings.STORAGE_BACKEND(**storage_kwargs)
  E           TypeError: Can't instantiate abstract class LocalStorage with abstract methods create_folder, getsize, list_folder, remove

Locally I don't have that problem and the tests pass.
Any ideas?

@MVrachev
MVrachev requested a review from kairoaraujo January 20, 2023 14:22
@MVrachev

Copy link
Copy Markdown
Member Author

I removed the need for StorageBackendInterface, updated LocalStorage.put and updated the tests accordingly.
Ready for another review.

Martin Vrachev added 7 commits January 21, 2023 19:27
The incompatibility with newer securesystemslib versions was caused
because of a new breaking change introduced in:
secure-systems-lab/securesystemslib#231

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
We want to use the "restrict" by default as we are the ones who will
save metadata and target files through LocalStorage.put and we want
to have as least privileges as possible because of security.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Simplify IStorage by not inheriting StorageBackendInterface anymore.
In the last securesystemslib version, the contributors there added
a lot of functionality inside the StorageBackendInterface which we
don't use and is irrelevant to us.
Additionally, we had at least two cases where our tests failed because
of StorageBackendInterface API changes and we were blocked to use an
older securesystemslib version or update IStorage accordingly.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
By mistake I have made IStorage a regular class as opposed to an
interface which is its purpose.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
MVrachev force-pushed the fix-sslib-compatibility-issue branch from 81d72e5 to b33c1f2 Compare January 21, 2023 17:34
@MVrachev

MVrachev commented Jan 21, 2023

Copy link
Copy Markdown
Member Author

Rebased because of merge conflicts.

Can we review this pr and merge it with a higher priority as every bump of every dependency in requirements.txt or requirements-dev.txt causes merge conflicts and I have to rebase?

Comment thread repository_service_tuf_worker/repository.py Outdated
Martin Vrachev added 2 commits January 23, 2023 17:52
Move the logic where we actually save the file content into the
IStorage.put API call as this will help us easily support more
storages within persist() call.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
MVrachev requested a review from kairoaraujo January 23, 2023 18:33
@MVrachev

Copy link
Copy Markdown
Member Author

Ready for another review @kairoaraujo.

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.

Latest sslib updated to the worker isn't compatible

3 participants