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

Adding Swagger documentation to Gateway #1205

Merged
merged 18 commits into from
Feb 27, 2024
Merged

Conversation

david-alber
Copy link
Contributor

Summary

Closes #1165
Adding swagger and openapi documentation to gateway.
The UI documentation is available via <host>:<gateway-port>/swagger/ or <host>:<gateway-port>/redoc/
JSON, and YAML is available via <host>:<gateway-port>/swagger.yaml or <host>:<gateway-port>/swagger.json

Details and comments

The API documentation is publically available to any user: No authorization or login is required.

@IceKhan13
Copy link
Member

Thank you @david-alber for your contribution!

@Tansito
Copy link
Member

Tansito commented Feb 1, 2024

Awesome, thank you @david-alber ! ❤️

@psschwei
Copy link
Collaborator

psschwei commented Feb 1, 2024

If I understand correctly, this will serve the swagger docs directly from the gateway pod, right?

Do we also want to makes the docs available offline and add them to a repo, either here or in the main IQP docs one?

@psschwei
Copy link
Collaborator

psschwei commented Feb 1, 2024

Is it possible to hide the Django-specific apis?

image

@Tansito
Copy link
Member

Tansito commented Feb 1, 2024

If I understand correctly, this will serve the swagger docs directly from the gateway pod, right?

Yes, you are right, Paul.

Do we also want to makes the docs available offline and add them to a repo, either here or in the main IQP docs one?

Mmm... I think it's not needed but open to discuss it.

@psschwei
Copy link
Collaborator

psschwei commented Feb 1, 2024

The UI documentation is available via <host>:<gateway-port>/swagger/ or <host>:<gateway-port>/redoc/

Neither of the UI sites work for me: the /swagger/ one asks for a Django login, and the /redoc/ one just loads a blank page. Both the file ones work fine.

@Tansito
Copy link
Member

Tansito commented Feb 1, 2024

Is it possible to hide the Django-specific apis?

@IceKhan13 I didn't think about it when we added the plugin but probably @psschwei has a point here. If I remember correctly we are not using now these end-points without keycloack so we could remove this ViewSet If I'm not wrong 🤔 (we could fix this in another PR).

@psschwei
Copy link
Collaborator

psschwei commented Feb 1, 2024

Do we also want to makes the docs available offline and add them to a repo, either here or in the main IQP docs one?

Mmm... I think it's not needed but open to discuss it.

Looking at the Runtime API as an example, their docs are available at:

The former looks like it makes the doc available similar to how we do here (in their case, via https://runtime-us-east.quantum-computing.ibm.com/static/qiskit_runtime.v1.yaml, which unless I'm mistaken is being served from their app, like we do the gateway here)

Not sure about the docs on IBM Cloud.

In any case, as long as what we're doing can easily move into the main docs (at whichever location), I think we're good...

@Tansito
Copy link
Member

Tansito commented Feb 1, 2024

Good question @psschwei , to be honest I thought that those pages required another additional step. I don't know if exporting our swagger configuration could be more than enough. Definitely something that we can try.

Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of the UI sites work for me: the /swagger/ one asks for a Django login, and the /redoc/ one just loads a blank page. Both the file ones work fine.

Same as @psschwei , I see Django Login if I enter in the /swagger url. Are we doing something wrong @david-alber ? Thank you in advance 😄

@david-alber
Copy link
Contributor Author

Neither of the UI sites work for me: the /swagger/ one asks for a Django login, and the /redoc/ one just loads a blank page. Both the file ones work fine.

Same as @psschwei , I see Django Login if I enter in the /swagger url. Are we doing something wrong @david-alber ? Thank you in advance 😄

I deploy quantum-serverless locally:

docker-compose -f docker-compose-dev.yaml up --build

Then I can see the swagger UI at localhost:8000/swagger/.

The most recent commit allows the rendering if no user is set. Could this have been the issue?
@Tansito, @psschwei how can I reproduce your checks?

@IceKhan13
Copy link
Member

IceKhan13 commented Feb 2, 2024

If I remember correctly we are not using now these end-points without keycloack so we could remove this ViewSet If I'm not wrong 🤔 (we could fix this in another PR).

Yes, I think we can remove them.

#1209

@psschwei
Copy link
Collaborator

psschwei commented Feb 2, 2024

how can I reproduce your checks?

I'm deploying on Kubernetes (local kind cluster) using the Helm chart. Even with the most recent change, I am not able to access the Swagger UI (still getting the Django log in screen). But it does work fine on Docker Compose.

One other thing -- I didn't see a way to pass the token as a header, which I think we might need for users to authenticate against the gateway to run commands

image

@david-alber
Copy link
Contributor Author

how can I reproduce your checks?

I'm deploying on Kubernetes (local kind cluster) using the Helm chart. Even with the most recent change, I am not able to access the Swagger UI (still getting the Django log in screen). But it does work fine on Docker Compose.

One other thing -- I didn't see a way to pass the token as a header, which I think we might need for users to authenticate against the gateway to run commands

image

  1. it might be the case that the ingress service need an additional path registered.

  2. yes. Good point. Let me figure that out 😀

@Tansito
Copy link
Member

Tansito commented Feb 6, 2024

how can I reproduce your checks?

Sorry for the delay @david-alber . In my case I use gunicorn main.wsgi:application --bind 0.0.0.0:8000 --workers=4 directly in the project after install the dependencies. Like we do in the docker-compose and I obtain the same result as @psschwei

@david-alber
Copy link
Contributor Author

@psschwei @Tansito with the latest commit I am addressing the following concerns:

  1. remove Djaong Login in Swagger UI as it is not needed
  2. Authentication for /api/v1/ routes: Klick on Authorize and type in the default mock token awesome_token. This will add the header: Authorization: awesome_token to every route, which authenticates the execution.
  3. include only /api/v1/ patterns in the swagger ui documentation (and remove /dj-rest-auth/ patterns from the UI as it is not needed). Note that the Base URL is thus: <host>:<port>/api/v1/

I have tested this by locally building the respective docker containers:

docker compose -f docker-compose-dev.yaml up postgres ray-head gateway -d --build

The Swagger UI is then available via:
localhost:8000/swagger/

Unfortunately, I am having issues with the deployment on K8s via a local kind cluster and thus cannot reproduce this test scenario. Maybe we can move this to another issue?

psschwei and others added 6 commits February 13, 2024 11:55
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
* add ModelView API for CatalogEntry

* additional apis for catalogentry

---------

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
* Removed main components from repository project

* Updated chart lock

* Remove repository references from the client
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
* use LocalProvider for notebook test

Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
* Update contribution guidelines

* Update contributing doc with supported python versions

* Remove Python 3.7 badge from README files

* Upgrade min python version in setup.py
@Tansito
Copy link
Member

Tansito commented Feb 16, 2024

Yep, I agree. Thank you @david-alber , I will take a look on it and let you know 🙏

@Tansito
Copy link
Member

Tansito commented Feb 22, 2024

Thank your for the update @david-alber I will try to take a look along this week 😄

Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @david-alber I see everything fine and from my part is working now. Thank you so much 🙏

@psschwei
Copy link
Collaborator

psschwei commented Feb 26, 2024

Oddly, I no longer see the Swagger UI, even when rolling back to the previous commit that did work, though I'm going to assume that's an issue on my end given that it's working for @Tansito

@Tansito
Copy link
Member

Tansito commented Feb 26, 2024

I tested it with the codespace feature + gunicorn. How did you try it, @psschwei ? I can test it locally again just to be sure (It should be the same though)

@psschwei
Copy link
Collaborator

Port forwarding the kubernetes service

@Tansito
Copy link
Member

Tansito commented Feb 26, 2024

@psschwei if you execute python manage.py collectstatic before deploy in kubernetes works for you?

@psschwei
Copy link
Collaborator

@psschwei if you execute python manage.py collectstatic before deploy in kubernetes works for you?

It did not (though that command did copy 69 files...), though that seems to be a step in the right direction

image

@psschwei
Copy link
Collaborator

image

seems like the javascript is getting blocked... though that could be a me issue

@Tansito
Copy link
Member

Tansito commented Feb 26, 2024

Locally for me it's working too (I just checked it right now). We can always continue improving this configuration later until it works for everyone.

@psschwei
Copy link
Collaborator

psschwei commented Feb 26, 2024

Well, not so much getting blocked as not existing...

image

edit: files exist in the pod, for example

I have no name!@gateway-796cfb4d5b-4tnn9:/usr/src/app$ ls static/drf-yasg/swagger-ui-dist/swagger-ui-bundle.js -l
-rw-r--r--. 1 1000 users 1046583 Feb 26 19:12 static/drf-yasg/swagger-ui-dist/swagger-ui-bundle.js

@psschwei
Copy link
Collaborator

Locally for me it's working too (I just checked it right now).

@Tansito how are you exposing the Kubernetes service? Port forwarding? Ingress? Door number 3?

@Tansito
Copy link
Member

Tansito commented Feb 26, 2024

Not using kubernetes at all, I just checked running locally with gunicorn command (maybe in kubernetes we will need more time to review it).

@psschwei
Copy link
Collaborator

Not using kubernetes at all

Ah, ok... it works fine for me too without kubernetes (docker compose in my case).

@Tansito
Copy link
Member

Tansito commented Feb 26, 2024

Yeah, I don't discard that we will need to review permissions or those kind of things. Is it something maybe that we can do in another PR? What do you think? (I don't have a strong opinion).

@psschwei
Copy link
Collaborator

I think either way is fine... as is, this PR adds the docs, and we just need to figure out how to expose it via Kubernetes (which is how end users will access it).

@psschwei
Copy link
Collaborator

running locally with gunicorn command

hmm... doesn't look like we're actually using gunicorn in kubernetes, whereas we are in docker compose...

@Tansito
Copy link
Member

Tansito commented Feb 26, 2024

Initially yes, or at least in our CI / CD:

https://github.com/Qiskit-Extensions/quantum-serverless/blob/main/charts/quantum-serverless/charts/gateway/templates/deployment.yaml#L68

This will run the command when it deploys the gateway if I'm not wrong, won't it @psschwei ?

@psschwei
Copy link
Collaborator

No, you're right... I was only looking at the dockerfile, not the deployment

@psschwei
Copy link
Collaborator

Ok, so I think I've narrowed this down to an issue with how Django serves static files... short version is in Kubernetes the static/ directory isn't being served, which means none of the javascript loads, which means no swagger ui.

Running locally, putting Django in debug mode and making a couple of tweaks to urls.py

$ git diff urls.py
diff --git a/gateway/main/urls.py b/gateway/main/urls.py
index 3180e81b..41cd6d03 100644
--- a/gateway/main/urls.py
+++ b/gateway/main/urls.py
@@ -22,6 +22,7 @@ from rest_framework import routers, permissions
 from drf_yasg.views import get_schema_view
 from drf_yasg import openapi
 import probes.views
+from django.contrib.staticfiles.urls import staticfiles_urlpatterns
 
 schema = get_schema_view(  # pylint: disable=invalid-name
     openapi.Info(
@@ -68,3 +69,4 @@ urlpatterns = [
 if settings.DEBUG:
     urlpatterns += static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT)
     urlpatterns += static(settings.STATIC_URL, document_root=settings.STATIC_ROOT)
+    urlpatterns += staticfiles_urlpatterns()

I was able to get the swagger ui working. But that's not an appropriate solution for production, so we'd need to do something more along the lines of the link above (s3?)

@david-alber
Copy link
Contributor Author

image

seems like the javascript is getting blocked... though that could be a me issue

A quick remark @psschwei
When developing the feature, I observed similar CSP-related errors.
The following CSP settings resolved these errors when running Gateway locally as a docker container:

gateway > main > settings.py

CSP_DEFAULT_SRC = "'none'"
CSP_SCRIPT_SRC = "'none'"
CSP_FRAME_ANCESTORS = "'self'"
CSP_OBJECT_SRC = "'self'"
CSP_IMG_SRC = ("'self'", "data:", "https://cdn.redoc.ly")
CSP_STYLE_SRC_ELEM = ("'self'", "'unsafe-inline'")
CSP_SCRIPT_SRC_ELEM = "'self'"
CSP_CONNECT_SRC = "'self'"
CSP_WORKER_SRC = ("'self'", "blob:")

@psschwei
Copy link
Collaborator

Thanks @david-alber -- I tried those, but they didn't solve the issue on Kubernetes (which seems to be an issue with the availability of the /static/ directory)... I'm inclined to say that's a separate problem that doesn't need to be tackled in this PR though...

@Tansito
Copy link
Member

Tansito commented Feb 27, 2024

I proceed to merge it then 😄

Thanks @david-alber for your help here!

@Tansito Tansito merged commit bd64e47 into Qiskit:main Feb 27, 2024
15 checks passed
@akihikokuroda
Copy link
Collaborator

I'm inclined to say that's a separate problem that doesn't need to be tackled in this PR though...

@psschwei Would you open a new issue? I would like to take a look at it. Thanks!

@david-alber
Copy link
Contributor Author

This was fun, as always. Thanks y'all for the support and guidance 🙏 ❤️ .

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

Successfully merging this pull request may close these issues.

Gateway: add swagger / openapi docs
7 participants