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

feat: validate server options #529

Merged
merged 10 commits into from
Feb 3, 2021
Merged

feat: validate server options #529

merged 10 commits into from
Feb 3, 2021

Conversation

olevski
Copy link
Member

@olevski olevski commented Jan 27, 2021

closes #71

So I implemented evaluation on the helm chart values but only for the server options portion. I will add a new issue to add it for the whole values.yaml file.

I also added more detailed validation (in line with the values.yaml validation) for the API.

I did some digging online to see if marshmallow can parse jsonschema and convert to marshmallow type schemas and it cannot unfortunately. There is a library that can convert a marshmallow schema to jsonschema but it cannot include the custom validation we need for some of the server options stuff. So I think we unfortunately have to have this kind of duplication.

This is deployed at https://tasko.dev.renku.ch/

@olevski
Copy link
Member Author

olevski commented Jan 27, 2021

issue for adding validation for the whole values.yaml file #530

Copy link
Contributor

@ableuler ableuler left a comment

Choose a reason for hiding this comment

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

Thanks for the careful work on validating that the server options conform to the desired schema. I agree that the duplication is a bit annoying, but we can live with it.

The really important point still missing here (you might consider this simply a bug of the initial implementation of those server options) is an actual check if the server options which come as part of the POST body are actually elements of the options that were offered for selection in the first place. Basically, we have to prevent this

image

when the dialog only offers this

image.

I simply replayed the POST request from the browser while giving myself a bit more CPU for this.

renku_notebooks/api/custom_fields.py Show resolved Hide resolved
renku_notebooks/api/custom_fields.py Show resolved Hide resolved
@olevski
Copy link
Member Author

olevski commented Feb 2, 2021

@ableuler let me know what you think about my responses. Also I added validation for the server options in the post request for starting a server as you suggested. I am sorry I missed that. I also added a test to ensure that trying to start a server with bad options will fail.

@olevski olevski requested a review from ableuler February 2, 2021 21:32
renku_notebooks/api/schemas.py Outdated Show resolved Hide resolved
@olevski
Copy link
Member Author

olevski commented Feb 3, 2021

Ok great thanks. Also ran the acceptance tests locally with docker and they passed.

@olevski olevski merged commit 03d20e2 into master Feb 3, 2021
@olevski olevski deleted the feat-validate-server-options branch February 3, 2021 15:26
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.

validate server_options
2 participants