Require credentials for non-container clients #2361
Conversation
erikbern
force-pushed
the
erikbern/no-container-credentials-ii
branch
from
74ecd4d to
d470734
Compare
erikbern
changed the title
erikbern
force-pushed
the
erikbern/no-container-credentials-ii
branch
from
d233c4d to
979e9a5
Compare
| def authenticated(self): | ||
| return self._authenticated | ||
| return self._credentials is not None |
There was a problem hiding this comment.
This might just be hard to think about because there are many different ways of creating a Client, but this strikes me as wrong? Can't you end up with a Client that has credentials attached, but we don't know if they are valid? Not sure what the practical implications would be, though.
There was a problem hiding this comment.
So yeah I think this is actually wrong (and let me try to add a test for it) but for a different reason I think. If a Modal app running in the cloud is trying to deploy a Modal app then I think it asserts that the client is authenticated, which will fail. Honestly think it's maybe better to remove this code altogether and just fail in respective handlers – I'm not sure why we need this check.
| if _is_remote(): | ||
| credentials = None | ||
| client_type = api_pb2.CLIENT_TYPE_CONTAINER |
There was a problem hiding this comment.
I wonder if we're setting ourselves up for (more) confusion about the two distinct meanings of the local/remote dichotomy in Modal:
- practical sense:
- local: not on modal infrastructure
- remote: on modal infrastructure
- functional sense:
- local: the context that you run or deploy your applications from
- remote: the context that you run or deploy your applications in
(This is maybe not a perfect set of definitions but hopefully it makes sense)
The _is_remote helper indicates which side you're on in the practical sense (based on the environment variables that we define in containers).
But what about users who want to run or deploy applications from Modal infrastructure? I thought we had people doing that; including people doing it on behalf of their users (such that the orchestrator app and worker app are using different tokens). Are we breaking that behavior? Maybe we're not — but it feels very hard to reason about.
There was a problem hiding this comment.
Let me add a test for deploying Modal apps from inside containers. It's possible this PR broke it because of the authenticated check. That being said, the authentication system (client vs container vs web) is basically orthogonal to what we're doing (creating an app vs running code inside an app).
|
On second thought I think this change conflates a bunch of logic changes with a bunch of test fixture stuff. I'm going to close this one and try to focus on just fixing tests first. That should make it possible to change the actual logic separately from the tests. |
This needs to be rebased on #2350 when merged.We now fail immediately in
Client.from_envif it's not_is_remote()and there are no credentials. This reduces the need forClientHello.We have a separate class method for creating "unauthenticated" clients, but it's only used for
modal token new.This mostly required fixing a bunch of tests that previously relied on the test fixture client having no credentials.
Cleans things up to the point where
ClientHellocan be trivially removed from containers. But I'll do that separately.