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

Use protobuf to parse the nsjail config and remove redundancies #88

Merged
merged 11 commits into from
Feb 3, 2021

Conversation

MarkKoz
Copy link
Member

@MarkKoz MarkKoz commented Jan 11, 2021

Fixes #74

Protobuf is used to parse the config because it's the proper way to do it and it's less maintenance than a custom parser. A Python module is generated from the protoc tool. There's a simple shell script that gets the config.proto from NsJail's GitHub and invokes protoc. The Python module only needs to be re-generated when NsJail is updated.

I realise more shell scripts go against #73, but I plan to address that issue in a separate PR. It'll also address the NsJail version currently being hard-coded in separate places (the Python script and the Dockerfile).

I managed to remove one shell script anyway since it was practically necessary to reduce redundancy. It required using the code in snekbox to invoke NsJail. This has the benefit of all the code that parses the config and sets up cgroups. I could have imported it from the shell script, but I didn't want to write Python code in-line in the .profile script.

I'll also write unit tests later, since there may be more refactoring down the road. It's not worth it right now if it's just gonna have to get changed again.

@MarkKoz MarkKoz added type: feature New feature or request area: nsjail Related to NsJail and its configuration labels Jan 11, 2021
@coveralls
Copy link

coveralls commented Jan 11, 2021

Coverage Status

Coverage decreased (-34.1%) to 59.322% when pulling aba5e0a on feat/nsjail/74/config into 7e6000e on master.

Copy link
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

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

This all looks good, and works locally.

One thing I'd note for development is maybe the usage of the script/protoc.sh and a note about how the config parsing actually works/should be modified.

Other than that, thumbs up from me!

@MarkKoz
Copy link
Member Author

MarkKoz commented Feb 3, 2021

One thing I'd note for development is maybe the usage of the script/protoc.sh

I plan to replace the script with on written in Python when I do #73. I'll document things better at that point.

and a note about how the config parsing actually works/should be modified.

Good point. I should document the config format, which is really just linking to the nsjail readme and maybe the protobuf file.

@MarkKoz MarkKoz merged commit c580809 into master Feb 3, 2021
@MarkKoz MarkKoz deleted the feat/nsjail/74/config branch February 3, 2021 00:04
@MarkKoz MarkKoz mentioned this pull request Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: nsjail Related to NsJail and its configuration priority: 2 - normal type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redundancy regarding NsJail config values
4 participants