support new backend authentication for garage-push and garage-deploy #1767
Conversation
There was a problem hiding this comment.
Looks fine, but this needs to be covered by unit tests. All the existing authentication methods are currently covered in authenticate_test.cc and we have to do something similar for this.
I was also wondering if there are any documentation changes necessary, but it looks like https://docs.ota.here.com/ota-client/latest/provisioning-methods-and-credentialszip.html doesn't going into details about the contents of treehub.json. I don't know if anything else does, either. Your thorough comment is probably good enough.
.vscode/launch.json
Outdated
| "ignoreFailures": true | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
I was never particularly fond of this file being part of the repo in the first place, but if it's going to be there, it really shouldn't have absolute paths that won't work for anyone else. I'd prefer if this file were removed and put it .gitignore, but I'd accept it if the paths were relative.
There was a problem hiding this comment.
yep. make sense remove the file. anyway configuration for aktualizr does not work
| @@ -17,6 +17,7 @@ boost::filesystem::path certs_dir; | |||
| * Parse authentication information from treehub.json. */ | |||
| TEST(authenticate, good_zip) { | |||
| // Authenticates with the ATS portal to the SaaS instance. | |||
| // It is outdated test. kepp it for backward compatibility | |||
src/sota_tools/authenticate_test.cc
Outdated
| @@ -98,12 +99,22 @@ TEST(authenticate, no_json_zip) { | |||
| /* Extract credentials from a provided JSON file. */ | |||
| TEST(authenticate, good_json) { | |||
| // Authenticates with the ATS portal to the SaaS instance. | |||
| // Outdated. Keep backward compatibility | |||
There was a problem hiding this comment.
This comment is good, but honestly, we can probably get rid of the whole json-only authentication at this point. The last time that was officially supported was over three years ago and it's been "deprecated" ever since. You can worry about that later in another PR.
There was a problem hiding this comment.
https://saeljira.it.here.com/browse/OTA-5341 to keep it and update test data after backend release
src/sota_tools/authenticate_test.cc
Outdated
| TEST(authenticate, good_json_v2) { | ||
| // Authenticates with new backend. | ||
| // Note: update auth_test_goog_v2.json after deploy on prod. current file uses HAT | ||
| boost::filesystem::path filepath = "tests/sota_tools/auth_test_goog_v2.json"; |
There was a problem hiding this comment.
Is it intentional that the comment and path here are goog, but the json is goox and the zip is good?
I suggest not supporting json directly but only the zip file. I also suggest creating the zip file with a script at build or test time instead of hardcoding a binary file into the repo. We try to avoid binaries as much as possible, and it makes things much easier to read, debug, and maintain to do it with a script. There are some examples of this already in the repo and it was one of our goals to do this everywhere.
Codecov Report
@@ Coverage Diff @@
## master #1767 +/- ##
==========================================
- Coverage 84.58% 84.51% -0.07%
==========================================
Files 174 174
Lines 11937 11949 +12
==========================================
+ Hits 10097 10099 +2
- Misses 1840 1850 +10
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
The code looks fine. However, there are still some lingering issues here:
- Now up to nine commits including merges. Please rebase and squash some of the smaller fixup commits.
- The zip file is currently unused. Since the json auth method is deprecated anyway, I would prefer if the zip file were used instead and the new json file excluded.
- The zip file should be constructed dynamically at build or test time via a script. Binaries should only be added to the repo if absolutely necessary.
… OTA-5285 Signed-off-by: Anatoliy Odukha <[email protected]>
aodukha
force-pushed
the
feat/new_backend_authentication
branch
from
1cb841a to
4ef5ba1
Compare
|
done. also "create zip at run-time" added to OTA-5341 task |
it is OTA-5285
Before back-end start support cognito authentication we do in code something like:
curl -X POST /token
-d "grant_type=client_credentials"
-u :
now we have to keep backward compatabitly with old credetials.zip and for new use :
curl -X POST
-d "grant_type=client_credentials"
-d "scope="
-u :
difference bewteen new and old credentials.zip are in treehub.json. New have "scope" and auth-server-url ends with /token
Signed-off-by: Anatoliy Odukha [email protected]