tickets/DM-45523: walk redirects for Lab manually, extracting xsrf along the way #365
Conversation
athornton
changed the title
athornton
force-pushed
the
tickets/DM-45523
branch
from
ad0820a
to
f82b3e1
Compare
athornton
force-pushed
the
tickets/DM-45523
branch
from
f82b3e1
to
e29b4df
Compare
athornton
force-pushed
the
tickets/DM-45523
branch
from
d436683
to
c08bcd2
Compare
src/mobu/storage/nublado.py
Outdated
| # automatically. | ||
| while r.is_redirect: | ||
| xsrf = self._extract_xsrf(r) | ||
| if xsrf and xsrf != self._lab_xsrf: |
There was a problem hiding this comment.
Why do we need to check this against the _lab_xsrf instead of _hub_xsrf? My initial understanding is that we want to change the _hub_sxrf if we get a different one back in any of the responses.
I'm actually sort of confused as to why we're comparing the _lab_xsrf to the _hub_xsrf in auth_to_lab also, but that's not being changed here...
There was a problem hiding this comment.
It's a bug in auth_to_lab as well. Looks like an old copy-paste error of mine.
There was a problem hiding this comment.
Do we even need to check if the token from the response cookie is different than the one we're storing, or can we just say "if we get an xsrf cookie in a response, store it"?
There was a problem hiding this comment.
if xsrf:
self._hub_xsrf = xsrf?
There was a problem hiding this comment.
Hm. I misunderstood the intention. I want to check if the token is different than the one we store to cut down on the number of times we log that we're setting the token, so that I can also understand the cadence with which we get genuinely new ones.
There was a problem hiding this comment.
Ahh gotcha. Yeah, with the logging it makes sense to keep the check. My main question was about comparing the hub token to the lab token and vice vera, and it looks like that was a bug originally and is fixed now.
athornton
force-pushed
the
tickets/DM-45523
branch
from
27568f4
to
a6cbe19
Compare
| @@ -188,6 +188,7 @@ async def execution_idle(self) -> bool: | |||
| return await self.pause(self.options.execution_idle_time) | |||
|
|
|||
| async def shutdown(self) -> None: | |||
| await self.hub_login() | |||
There was a problem hiding this comment.
What about putting a hub_login call in delete_lab? Tradeoff is that we may call it a few extra times.
There was a problem hiding this comment.
I think I prefer it be explicit.
athornton
force-pushed
the
tickets/DM-45523
branch
from
a6cbe19
to
9c71b81
Compare
athornton
force-pushed
the
tickets/DM-45523
branch
2 times, most recently
from
8c94435
to
e94e901
Compare
athornton
force-pushed
the
tickets/DM-45523
branch
from
e94e901
to
ec9b870
Compare
| if xsrf is not None: | ||
| self._logger.debug( | ||
| "Extracted _xsrf cookie", | ||
| method=response.request.method, | ||
| url=response.url.copy_with(query=None, fragment=None), | ||
| status_code=response.status_code, | ||
| # Logging the set-cookie header can be useful here but it | ||
| # leaks secrets. Don't put that code in a release build. | ||
| ) |
There was a problem hiding this comment.
Would we want to put this in each auth method so it would be easier to tell if it happened during the hub auth or the lab auth? It would be duplicated, but maybe more clear?
There was a problem hiding this comment.
Well, if we end up using it, we log that (with whether it is lab or hub as well). I'm not sure we even want to log it in the extraction step. Maybe I'll just leave the comment.
There was a problem hiding this comment.
Eh, I'm going to leave this alone for now. If it's too chatty if we need to turn on debug for real, we can change it then.
Also allow setting log level for individual monkeys, which makes debugging the problem we're solving here a lot easier.