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

[batch] Openapi doc for batch service #14777

Merged
merged 26 commits into from
Jan 14, 2025

Conversation

cjllanwarne
Copy link
Collaborator

@cjllanwarne cjllanwarne commented Jan 8, 2025

Change Description

Adds two endpoints in the batch service hosting an openapi specification and a swagger page.

Note to reviewers: The API specification documentat itself is nice to get right, but relatively low risk. I'd prioritize time making sure the framework to host it is correct, and the openapi setup sections before scanning through the details of every individual API call.

Security Assessment

Delete all except the correct answer:

  • This change has a high security impact
    • Required: The impact has been assessed and approved by appsec

Impact Description

We are adding two new endpoints in batch. Mitigating factors:

  • There are no user parameters for these endpoints
  • The pages being hosted are mostly static, apart from the "base url" which the API documentation and swagger page targets, which is generated in the backend.

(Reviewers: please confirm the security impact before approving)

@cjllanwarne cjllanwarne requested a review from sarahgibs January 9, 2025 19:49
Copy link

@sarahgibs sarahgibs left a comment

Choose a reason for hiding this comment

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

I've checked that ZAP will ingest this version of the openapi spec, and doesn't have any problems with it. Thanks for adding these endpoints.

Copy link
Collaborator

@patrick-schultz patrick-schultz left a comment

Choose a reason for hiding this comment

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

I can't really comment on the endpoint docs themselves, but I do have one suggestion about the metadata setup.

@@ -0,0 +1,1449 @@
openapi: 3.1.0
info:
version: 0.2.133
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be a bit of a pain to keep updated. It's also not technically correct, since changes to the service api are deployed continuously, not when we release a new version. Does this actually matter much?

Actually, I think we could template this. front_end.py already imports version, which is the full hail version, including the commit hash. That would be most accurate. But if you think we'd rather have just the release version, you could also import pip_version from hailtop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good catch. This is not really necessary, at least for the purposes of this POC. I think if we were publishing multiple versions of our API spec we might want to be able to switch between or identify which one we're looking at, but that's definitely not a now requirement. For now I'll just remove this field since it's already out of date.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, it's a required field. I'll just template in the version

@hail-ci-robot hail-ci-robot merged commit e907c22 into hail-is:main Jan 14, 2025
3 checks passed
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.

4 participants