Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test https://github.com/wolfv/quetz/tree/testing #9

Open
najose opened this issue Jan 11, 2024 · 6 comments
Open

Test https://github.com/wolfv/quetz/tree/testing #9

najose opened this issue Jan 11, 2024 · 6 comments

Comments

@najose
Copy link

najose commented Jan 11, 2024

@wolfv I have tried testing testing the changes in https://github.com/wolfv/quetz/tree/testing and noted my observations below.

Observations

  • Quetz crashes on handling an incoming request with the error "Google IAM is not configured". On checking the code here - it supposed to raise the error if [googleiam] section is not configured in the config.toml. But in my testing the section is present in the config. On adding more debug statements into the code, it seems calling config.configured_section('googleiam) calls config.get('googleiam') and this fails with the same error message as the ones seen in the annotated test failures in this PR workflow run - (AttributeError: 'super' object has no attribute 'getattr'). Also "Google IAM is not configured" seems to be raised in the test failure annotations for IAP middleware PR

Testing environment

I have a built docker image using the below dockerfile and pushed it a publically accessible registry in case you want to inspect the image I'm using.

Image name: us-east1-docker.pkg.dev/package-factory-sandbox/quetz-sdgr/quetz-testing:latest

# Build conda environment
FROM condaforge/mambaforge:4.9.2-7 as conda

COPY environment.yml /tmp/environment.yml
RUN CONDA_COPY_ALWAYS=true mamba env create -p /env -f /tmp/environment.yml \
  && conda clean -afy

COPY . /code
RUN conda run -p /env python -m pip install --no-deps /code && \
    conda run -p /env python -m pip install /code/plugins/quetz_googleiap && \
    conda run -p /env python -m pip install xattr google-api-python-client google-auth-httplib2 google-auth-oauthlib quetz-frontend gcsfs

# Create image
FROM debian:buster-slim

ENV LANG=C.UTF-8 LC_ALL=C.UTF-8

COPY --from=conda /env /env

# Set WORKDIR to /tmp because quetz always creates a quetz.log file
# in the current directory
WORKDIR /tmp
ENV PATH /env/bin:$PATH
EXPOSE 8000

# The following command assumes that a deployment has been initialized
# in the /quetz-deployment volume
RUN apt-get update -y && \
    apt-get install curl -y

CMD ["quetz", "start", "/quetz-deployment", "--host", "0.0.0.0", "--port", "8000"]

The container startup script creates a deployment and starts Quetz:

quetz create {{ $quetzDeploymentDirectory }} --copy-conf {{ $quetzConfigMapVolMountPath }}/{{ $quetzConfigFileName }} --exists-ok
quetz start {{ $quetzDeploymentDirectory }} --host 0.0.0.0 --port {{ $quetzContainerPort }}

Config

[pamauthenticator]
# name for the provider, used in the login URL
provider = "pam"
# use the following to translate the Unix groups that
# users might belong to user role on Quetz server
admin_groups = ["quetz"]
maintainer_groups = []
user_groups = []

[sqlalchemy]
# In production we use postgres, but the issue can be reproduced with sqlite.
database_url = "sqlite:///./quetz.sqlite"

[session]
# openssl rand -hex 32. The below is not the secret that is actually used :)
secret = "984d92e652ba8ac0801a5a9c8f79b5b0bcf9f3461f65eda7c2867d29a3a276f9"
https_only = false

[logging]
level = "DEBUG"
file = "quetz.log"

[users]
admins = []

[storage]
soft_delete_channel = true
soft_delete_package = true

[googleiam]
server_admin_emails = [ "me@domain.com" ]

[profiling]
enable_sampling = false

[gcs]
project = "package-factory-sandbox"
bucket_prefix = "package-factory-"
bucket_suffix = "-packages"
cache_timeout = "0"
region="us-central1"

I'll add more observations in the GitHub issue as and when I find them. Please let me know when I can test again with the issues resolved in the testing branch.

@najose
Copy link
Author

najose commented Jan 16, 2024

@wolfv, So the above mentioned issue was an error from my side. The persistent volume had the old config, so creating a deployment with --exists-ok must've skipped bringing in the new changes to the configs.

Once I fixed this, I was able to test all the other features as well. I have made some changes but I couldn't push as I don't have write access, so I've added diffs of the changes I made below.

  • Google IAP proxy middleware:

    • This feature is mostly working as expected. It correctly picks up the user information from the headers passed on by IAP

    • Fix needed: The logic to handle server_admin_emails seems like it needs the below fix to compare user email instead of a userid:

      Show/Hide diff
      diff --git a/plugins/quetz_googleiap/quetz_googleiap/middleware.py b/plugins/quetz_googleiap/quetz_googleiap/middleware.py
      index 24e4e31..9a2a23d 100644
      --- a/plugins/quetz_googleiap/quetz_googleiap/middleware.py
      +++ b/plugins/quetz_googleiap/quetz_googleiap/middleware.py
      @@ -86,7 +86,7 @@ class GoogleIAMMiddleware(BaseHTTPMiddleware):
                       )
                       dao.create_channel(channel, user.id, "owner")
       
      -            self.google_role_for_user(user_id, dao)
      +            self.google_role_for_user(user_id, email, dao)
                   user_id = uuid.UUID(bytes=user.id)
                   # drop the db and dao to remove the connection
                   del db, dao
      @@ -100,15 +100,15 @@ class GoogleIAMMiddleware(BaseHTTPMiddleware):
               response = await call_next(request)
               return response
       
      -    def google_role_for_user(self, user_id, dao):
      -        if not user_id:
      +    def google_role_for_user(self, user_id, username, dao):
      +        if not user_id or not username:
                   return
       
      -        if user_id in self.server_admin_emails:
      -            logger.info(f"User {user_id} is server admin")
      +        if username in self.server_admin_emails:
      +            logger.info(f"User '{username}' with user id '{user_id}' is server admin")
                   dao.set_user_role(user_id, "owner")
               else:
      -            logger.info(f"User {user_id} is not a server admin")
      +            logger.info(f"User '{username}' with user id '{user_id}' is not a server admin")
                   dao.set_user_role(user_id, "member")
  • Soft Delete: This is working as expected for both packages and channels once I made a few changes regarding config.

    • I had to make a few fixes to get this to work

      Show/Hide diff
      diff --git a/quetz/main.py b/quetz/main.py
      index 9f4d398..267d42e 100644
      --- a/quetz/main.py
      +++ b/quetz/main.py
      @@ -658,7 +658,7 @@ def delete_channel(
           auth.assert_delete_channel(channel)
           dao.delete_channel(channel.name)
           try:
      -        if not config.storage.soft_delete_channel:
      +        if not config.storage_soft_delete_channel:
                   pkgstore.remove_channel(channel.name)
           except FileNotFoundError:
               logger.warning(
      @@ -897,7 +897,7 @@ def delete_package(
           db.commit()
       
           for filename in filenames:
      -        if not config.storage.soft_delete_package:
      +        if not config.storage_soft_delete_package:
                   pkgstore.delete_file(channel_name, filename)
       
           dao.update_channel_size(channel_name)
      @@ -1246,7 +1246,7 @@ def delete_package_version(
           db.commit()
       
           path = os.path.join(platform, filename)
      -    if not config.storage.soft_delete_package:
      +    if not config.storage_soft_delete_package:
               pkgstore.delete_file(channel_name, path)
       
           dao.cleanup_channel_db(channel_name, package_name)
  • Package promotion: I couldn't get this to work, whenever I tried to copy a package it seems to fail with below stack trace in the server logs:

    Show/Hide Stack Trace
    ```
    INFO:     35.191.17.221:44308 - "POST /api/channels/soft-delete-test/packages/copy?source_channel=test-channel&source_package=eigen&subdir=linux-64&filename=eigen-3.4.0-h00ab1b0_0.conda HTTP/1.1" 500 Internal Server Error
    ERROR:    Exception in ASGI application
    Traceback (most recent call last):
      File "/env/lib/python3.10/site-packages/uvicorn/protocols/http/h11_impl.py", line 408, in run_asgi
        result = await app(  # type: ignore[func-returns-value]
      File "/env/lib/python3.10/site-packages/uvicorn/middleware/proxy_headers.py", line 84, in __call__
        return await self.app(scope, receive, send)
      File "/env/lib/python3.10/site-packages/fastapi/applications.py", line 1054, in __call__
        await super().__call__(scope, receive, send)
      File "/env/lib/python3.10/site-packages/starlette/applications.py", line 123, in __call__
        await self.middleware_stack(scope, receive, send)
      File "/env/lib/python3.10/site-packages/starlette/middleware/errors.py", line 186, in __call__
        raise exc
      File "/env/lib/python3.10/site-packages/starlette/middleware/errors.py", line 164, in __call__
        await self.app(scope, receive, _send)
      File "/env/lib/python3.10/site-packages/starlette/middleware/sessions.py", line 83, in __call__
        await self.app(scope, receive, send_wrapper)
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 189, in __call__
        with collapse_excgroups():
      File "/env/lib/python3.10/contextlib.py", line 153, in __exit__
        self.gen.throw(typ, value, traceback)
      File "/env/lib/python3.10/site-packages/starlette/_utils.py", line 91, in collapse_excgroups
        raise exc
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 191, in __call__
        response = await self.dispatch_func(request, call_next)
      File "/env/lib/python3.10/site-packages/quetz_googleiap/middleware.py", line 100, in dispatch
        response = await call_next(request)
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 165, in call_next
        raise app_exc
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 151, in coro
        await self.app(scope, receive_or_disconnect, send_no_error)
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 189, in __call__
        with collapse_excgroups():
      File "/env/lib/python3.10/contextlib.py", line 153, in __exit__
        self.gen.throw(typ, value, traceback)
      File "/env/lib/python3.10/site-packages/starlette/_utils.py", line 91, in collapse_excgroups
        raise exc
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 191, in __call__
        response = await self.dispatch_func(request, call_next)
      File "/env/lib/python3.10/site-packages/quetz/main.py", line 153, in dispatch
        response = await call_next(request)
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 165, in call_next
        raise app_exc
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 151, in coro
        await self.app(scope, receive_or_disconnect, send_no_error)
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 189, in __call__
        with collapse_excgroups():
      File "/env/lib/python3.10/contextlib.py", line 153, in __exit__
        self.gen.throw(typ, value, traceback)
      File "/env/lib/python3.10/site-packages/starlette/_utils.py", line 91, in collapse_excgroups
        raise exc
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 191, in __call__
        response = await self.dispatch_func(request, call_next)
      File "/env/lib/python3.10/site-packages/quetz/metrics/middleware.py", line 83, in dispatch
        raise e from None
      File "/env/lib/python3.10/site-packages/quetz/metrics/middleware.py", line 75, in dispatch
        response = await call_next(request)
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 165, in call_next
        raise app_exc
      File "/env/lib/python3.10/site-packages/starlette/middleware/base.py", line 151, in coro
        await self.app(scope, receive_or_disconnect, send_no_error)
      File "/env/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 62, in __call__
        await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send)
      File "/env/lib/python3.10/site-packages/starlette/_exception_handler.py", line 64, in wrapped_app
        raise exc
      File "/env/lib/python3.10/site-packages/starlette/_exception_handler.py", line 53, in wrapped_app
        await app(scope, receive, sender)
      File "/env/lib/python3.10/site-packages/starlette/routing.py", line 762, in __call__
        await self.middleware_stack(scope, receive, send)
      File "/env/lib/python3.10/site-packages/starlette/routing.py", line 782, in app
        await route.handle(scope, receive, send)
      File "/env/lib/python3.10/site-packages/starlette/routing.py", line 297, in handle
        await self.app(scope, receive, send)
      File "/env/lib/python3.10/site-packages/starlette/routing.py", line 77, in app
        await wrap_app_handling_exceptions(app, request)(scope, receive, send)
      File "/env/lib/python3.10/site-packages/starlette/_exception_handler.py", line 64, in wrapped_app
        raise exc
      File "/env/lib/python3.10/site-packages/starlette/_exception_handler.py", line 53, in wrapped_app
        await app(scope, receive, sender)
      File "/env/lib/python3.10/site-packages/starlette/routing.py", line 72, in app
        response = await func(request)
      File "/env/lib/python3.10/site-packages/fastapi/routing.py", line 299, in app
        raise e
      File "/env/lib/python3.10/site-packages/fastapi/routing.py", line 294, in app
        raw_response = await run_endpoint_function(
      File "/env/lib/python3.10/site-packages/fastapi/routing.py", line 193, in run_endpoint_function
        return await run_in_threadpool(dependant.call, **values)
      File "/env/lib/python3.10/site-packages/starlette/concurrency.py", line 40, in run_in_threadpool
        return await anyio.to_thread.run_sync(func, *args)
      File "/env/lib/python3.10/site-packages/anyio/to_thread.py", line 56, in run_sync
        return await get_async_backend().run_sync_in_worker_thread(
      File "/env/lib/python3.10/site-packages/anyio/_backends/_asyncio.py", line 2134, in run_sync_in_worker_thread
        return await future
      File "/env/lib/python3.10/site-packages/anyio/_backends/_asyncio.py", line 851, in run
        result = context.run(func, *args)
      File "/env/lib/python3.10/site-packages/quetz/main.py", line 961, in copy_package
        auth.assert_channel_read(source_channel)
      File "/env/lib/python3.10/site-packages/quetz/authorization.py", line 327, in assert_channel_read
        if channel.private:
    ```
    

@wolfv
Copy link
Owner

wolfv commented Jan 16, 2024

Thanks, fixed the first issue like you did (and pushed to both testing and the feature branch). Now going to work on the second issue – will also fix up all the tests today.

@wolfv
Copy link
Owner

wolfv commented Jan 16, 2024

@najose I've fixed two additional issues I've encountered for the copying. Maybe you can test with the latest state again :)

@wolfv
Copy link
Owner

wolfv commented Jan 16, 2024

quick note that I implemented the feature you asked for re. skipping the authorization for health endpoints.

@najose
Copy link
Author

najose commented Jan 17, 2024

@wolfv, Thanks will test it out soon.

@najose
Copy link
Author

najose commented Jan 23, 2024

@wolfv, I tested out the latest changes (fix for copying packages, health check with no authN) from the testing branch, the changes are all working as expected.

You seem to have missed one fix for handling server_admin_emails. The current logic seems to be looking at an integer user id in a server_admin_emails list which is supposed to contain a list of emails.

diff --git a/plugins/quetz_googleiap/quetz_googleiap/middleware.py b/plugins/quetz_googleiap/quetz_googleiap/middleware.py
index 71e05c5..cd18b80 100644
--- a/plugins/quetz_googleiap/quetz_googleiap/middleware.py
+++ b/plugins/quetz_googleiap/quetz_googleiap/middleware.py
@@ -91,7 +91,7 @@ class GoogleIAMMiddleware(BaseHTTPMiddleware):
                 )
                 dao.create_channel(channel, user.id, "owner")
 
-            self.google_role_for_user(user_id, dao)
+            self.google_role_for_user(user_id, email, dao)
             user_id = uuid.UUID(bytes=user.id)
             # drop the db and dao to remove the connection
             del db, dao
@@ -105,15 +105,15 @@ class GoogleIAMMiddleware(BaseHTTPMiddleware):
         response = await call_next(request)
         return response
 
-    def google_role_for_user(self, user_id, dao):
-        if not user_id:
+    def google_role_for_user(self, user_id, username, dao):
+        if not user_id or not username:
             return
 
-        if user_id in self.server_admin_emails:
-            logger.info(f"User {user_id} is server admin")
+        if username in self.server_admin_emails:
+            logger.info(f"User '{username}' with user id '{user_id}' is server admin")
             dao.set_user_role(user_id, "owner")
         else:
-            logger.info(f"User {user_id} is not a server admin")
+            logger.info(f"User '{username}' with user id '{user_id}' is not a server admin")
             dao.set_user_role(user_id, "member")
 

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

No branches or pull requests

2 participants