This repository has been archived by the owner on Sep 12, 2022. It is now read-only.
Place upper limit on tas user validation, remove selected identity notion #639
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdosborn
changed the title
cdosborn
force-pushed
the
fix-account-creation
branch
3 times, most recently
from
55fbfdf
to
3a8f9a1
Compare
cdosborn
force-pushed
the
fix-account-creation
branch
4 times, most recently
from
a1c851d
to
cd6aef3
Compare
cdosborn
changed the title
cdosborn
force-pushed
the
fix-account-creation
branch
5 times, most recently
from
bf68037
to
802f35d
Compare
cdosborn
force-pushed
the
fix-account-creation
branch
from
802f35d
to
3a581a8
Compare
|
Thanks @cdosborn! I'd like to dig through this a bit more. |
cdosborn
force-pushed
the
fix-account-creation
branch
5 times, most recently
from
705c968
to
5f291b9
Compare
julianpistorius
suggested changes
Aug 3, 2018
There was a problem hiding this comment.
@cdosborn Can you help me understand this one?
ea53272#diff-3f7bc33e5a66a692d2e802229596243dL184
Otherwise looks good. 👍
jetstream/tas_api.py
Outdated
| @@ -16,7 +16,8 @@ def tacc_api_post(url, post_data, username=None, password=None): | |||
| # logger.debug("REQ BODY: %s" % post_data) | |||
| resp = requests.post( | |||
| url, post_data, | |||
| auth=(username, password)) | |||
| auth=(username, password), | |||
| timeout=5) | |||
There was a problem hiding this comment.
I'm worried that one day we will have to modify this in production in a hurry. Can we make this a setting with a default of 5 seconds?
core/models/user.py
Outdated
| @@ -181,7 +181,7 @@ def email_hash(self): | |||
| # Save Hooks Here: | |||
|
|
|||
|
|
|||
| def get_or_create_user_profile(sender, instance, created, **kwargs): | |||
There was a problem hiding this comment.
Huh. What's going on here?
|
Just going to push this into the next release |
|
@julianpistorius i'm tacking on some tests in a short bit |
cdosborn
force-pushed
the
fix-account-creation
branch
4 times, most recently
from
938ee0b
to
56058f2
Compare
2 tasks
|
@julianpistorius This is being blocked by cyverse/clank#277, once that is 👍 the travis build should greenlight. |
cdosborn
force-pushed
the
fix-account-creation
branch
from
56058f2
to
73ae1bf
Compare
The tas api averages about 500ms, but occasionally goes to 3,5,10 sec. This upperbound is to limit how long api/v1/profile will take in the worst case.
Creating new accounts and validation of the user occurred in a side effect of the selcted_identity field of a serializer. These calls need to be made, but should be more explicit. A selected_idenity is a relic from a time before. An important side effect here is that PATCHs no longer create accounts or check if the user is valid, greatly reducing the time to update a user's profile. In addition, create_new_accounts shouldn't check whether it should actually do its job, by calling user.is_valid first.
This is implementing what was mentioned in a TODO comment
cdosborn
force-pushed
the
fix-account-creation
branch
from
dc64bc9
to
943d9b4
Compare
4 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This changeset places an upper limit on the tas api calls (5sec, in practice the average is about 0.6s), removes all references to selected identities, and makes the user validation and account creation explicit in api/v1/profile (originally the purview of select identity logic)
User validation and account creation happen as a side effect of the serialization of a user's "selected identity" in the api/v1/profile endpoint. Atmosphere used to have the notion of a selected identity for each user. A user's actions would occur against the cloud pointed to by their selected identity. In the frontend, the user would first select a cloud, before they could perform actions against it. All references to a selected identity are vestigial. In jetstream we discovered that changing a user's profile would sometimes take 10-15s, this occurred because the serialization of the selected_identity was checking if the user was valid by querying out to the TAS API. After checking that the user was valid, the service would make sure that the user had identities on all providers where they were permitted.
Depends on cyverse/clank#277
Checklist before merging Pull Requests