You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Budnik <ab...@mesosphere.com> on 2018/10/22 17:04:44 UTC
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/
-----------------------------------------------------------
(Updated Oct. 22, 2018, 5:04 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
Changes
-------
small refactoring & bug fixes & more comments
Bugs: MESOS-9105
https://issues.apache.org/jira/browse/MESOS-9105
Repository: mesos
Description
-------
Docker Seccomp config is a JSON file containing Seccomp filtering
rules. This patch introduces a parser for Docker Seccomp config format.
This parser accepts a JSON-string, parses and validates it, then
returns a prepared `ContainerSeccompProfile` message.
Diffs (updated)
-----
src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8
src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81
src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/68019/diff/3/
Changes: https://reviews.apache.org/r/68019/diff/2-3/
Testing
-------
Thanks,
Andrei Budnik
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/#review212164
-----------------------------------------------------------
src/linux/seccomp/seccomp_parser.cpp
Lines 84-89 (patched)
<https://reviews.apache.org/r/68019/#comment297829>
Nits, I would prefer:
```
return Error(
"Cannot determine 'defaultAction' for the Seccomp configuration: " +
(defaultAction.isError() ? defaultAction.error() : "Not found");
```
ditto to all others.
- Gilbert Song
On Nov. 8, 2018, 7:24 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 7:24 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
>
>
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf
> src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132
> src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
> src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68019/diff/14/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Dec. 27, 2018, 3:19 a.m., Qian Zhang wrote:
> > src/linux/seccomp/seccomp_parser.cpp
> > Lines 484 (patched)
> > <https://reviews.apache.org/r/68019/diff/7/?file=2109138#file2109138line484>
> >
> > Can you please elaborate on why we need to do the parsing manually rather than using `protobuf::parse`? Is it because we need to manually handle the `SCMP_` prefix? Any other reasons?
`SCMP_ARCH_X86` is a c-style macro. So, we need to avoid names starting with `SCMP_`.
>Is it because we need to manually handle the SCMP_ prefix?
Yes, it is.
>Any other reasons?
Nope.
> On Dec. 27, 2018, 3:19 a.m., Qian Zhang wrote:
> > src/linux/seccomp/seccomp_parser.cpp
> > Lines 504 (patched)
> > <https://reviews.apache.org/r/68019/diff/7/?file=2109138#file2109138line504>
> >
> > Just a question, this will not affect agent process since we do not call `seccompFilter.get()->load()`, right?
Yes, correct.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/#review211534
-----------------------------------------------------------
On Nov. 8, 2018, 3:24 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 3:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
>
>
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
> src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68019/diff/9/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/#review211534
-----------------------------------------------------------
src/linux/seccomp/seccomp_parser.hpp
Lines 22 (patched)
<https://reviews.apache.org/r/68019/#comment296855>
This is should be before `#include <stout/try.hpp>`.
src/linux/seccomp/seccomp_parser.hpp
Lines 30 (patched)
<https://reviews.apache.org/r/68019/#comment296854>
One more newline here.
src/linux/seccomp/seccomp_parser.cpp
Lines 39-40 (patched)
<https://reviews.apache.org/r/68019/#comment296856>
Can we change it to `constexpr char SCMP_PREFIX[] = "SCMP_`?
src/linux/seccomp/seccomp_parser.cpp
Lines 484 (patched)
<https://reviews.apache.org/r/68019/#comment296860>
Can you please elaborate on why we need to do the parsing manually rather than using `protobuf::parse`? Is it because we need to manually handle the `SCMP_` prefix? Any other reasons?
src/linux/seccomp/seccomp_parser.cpp
Lines 485 (patched)
<https://reviews.apache.org/r/68019/#comment296858>
Kill this newline, and ditto for the other two places below this code.
src/linux/seccomp/seccomp_parser.cpp
Lines 504 (patched)
<https://reviews.apache.org/r/68019/#comment296857>
Just a question, this will not affect agent process since we do not call `seccompFilter.get()->load()`, right?
- Qian Zhang
On Nov. 8, 2018, 11:24 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 11:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
>
>
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8
> src/Makefile.am 2d9c81b149a5764dc82593bef102f5568847daa2
> src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
> src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68019/diff/7/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/#review212369
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On Nov. 8, 2018, 7:24 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 7:24 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
>
>
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf
> src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132
> src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
> src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68019/diff/15/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Jan. 19, 2019, 1:05 a.m., Gilbert Song wrote:
> > src/linux/seccomp/seccomp_parser.cpp
> > Lines 164-167 (patched)
> > <https://reviews.apache.org/r/68019/diff/14/?file=2119630#file2119630line164>
> >
> > does the order of the `architecture/subarchitecture` matter? asking cause I am not sure if we need architecture before its corresponding subarchitecture in the repeated field
> does the order of the architecture/subarchitecture matter?
No, it doesn't.
> On Jan. 19, 2019, 1:05 a.m., Gilbert Song wrote:
> > src/linux/seccomp/seccomp_parser.cpp
> > Lines 490-494 (patched)
> > <https://reviews.apache.org/r/68019/diff/14/?file=2119630#file2119630line490>
> >
> > Since we somehow have to create the profile in launch.cpp, could we avoid create the `ctx` object twice?
> >
> > Instead, for the default profile, I think we could add this verification in isolator::create method after we parse it
I'm totally against moving verification step outside of this function. Such a tiny optimization does not worth it.
The benefit of verifying Seccomp profile in-place is that:
1) We do not duplicate code for calling verification for callers of this function. We need to always verify the Seccomp profile after parsing it.
2) If `libseccomp` returns an error (e.g., when it does not recognize a syscall), this function returns the error, so it will be written in agent logs. That is very helpful for debugging. Otherwise, an error will be detected in `launch.cpp` - it won't be printed into logs.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/#review212151
-----------------------------------------------------------
On Nov. 8, 2018, 3:24 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 3:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
>
>
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf
> src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132
> src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
> src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68019/diff/14/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/#review212151
-----------------------------------------------------------
Fix it, then Ship it!
src/linux/seccomp/seccomp_parser.cpp
Lines 82 (patched)
<https://reviews.apache.org/r/68019/#comment297809>
Not a big issue but seems like in our code base we usually pass in a pointer instead of reference.
The positive side of passing a pointer is that it would be more explicit for code reader when they read the caller side of this method, they know the protobuf object will be mutated.
You can regard `Architecture_Parse()` as an example.
cc @bmahler
src/linux/seccomp/seccomp_parser.cpp
Lines 104 (patched)
<https://reviews.apache.org/r/68019/#comment297812>
ditto.
src/linux/seccomp/seccomp_parser.cpp
Lines 164-167 (patched)
<https://reviews.apache.org/r/68019/#comment297816>
does the order of the `architecture/subarchitecture` matter? asking cause I am not sure if we need architecture before its corresponding subarchitecture in the repeated field
src/linux/seccomp/seccomp_parser.cpp
Lines 176 (patched)
<https://reviews.apache.org/r/68019/#comment297813>
ditto.
src/linux/seccomp/seccomp_parser.cpp
Lines 263 (patched)
<https://reviews.apache.org/r/68019/#comment297815>
ditto
src/linux/seccomp/seccomp_parser.cpp
Lines 283 (patched)
<https://reviews.apache.org/r/68019/#comment297817>
index->as<uint32_t>()?
src/linux/seccomp/seccomp_parser.cpp
Lines 290 (patched)
<https://reviews.apache.org/r/68019/#comment297818>
ditto
src/linux/seccomp/seccomp_parser.cpp
Lines 297 (patched)
<https://reviews.apache.org/r/68019/#comment297819>
ditto
src/linux/seccomp/seccomp_parser.cpp
Lines 325 (patched)
<https://reviews.apache.org/r/68019/#comment297814>
ditto.
src/linux/seccomp/seccomp_parser.cpp
Lines 490-494 (patched)
<https://reviews.apache.org/r/68019/#comment297827>
Since we somehow have to create the profile in launch.cpp, could we avoid create the `ctx` object twice?
Instead, for the default profile, I think we could add this verification in isolator::create method after we parse it
- Gilbert Song
On Nov. 8, 2018, 7:24 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 7:24 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
>
>
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf
> src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132
> src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
> src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68019/diff/14/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Jan. 14, 2019, 1:24 p.m., Qian Zhang wrote:
> > src/linux/seccomp/seccomp_parser.cpp
> > Lines 363-365 (patched)
> > <https://reviews.apache.org/r/68019/diff/13/?file=2117427#file2117427line363>
> >
> > So `includes` is a **required** field in a seccomp profile, at least we should have `"includes": {}` in a seccomp profile, right?
Yes, correct. See https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/api/types/seccomp.go#L91See - `Includes` doesn't have `omitempty` flag.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/#review211947
-----------------------------------------------------------
On Nov. 8, 2018, 3:24 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 3:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
>
>
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf
> src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132
> src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
> src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68019/diff/14/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/#review211947
-----------------------------------------------------------
Fix it, then Ship it!
src/linux/seccomp/seccomp_parser.cpp
Lines 87 (patched)
<https://reviews.apache.org/r/68019/#comment297519>
s/configuration /configuration/
Ditto for all other messages starting with `Cannot determine`.
src/linux/seccomp/seccomp_parser.cpp
Lines 160 (patched)
<https://reviews.apache.org/r/68019/#comment297521>
Kill this empty line.
I see a couple of empty lines like this, please remove them as well.
src/linux/seccomp/seccomp_parser.cpp
Lines 216-217 (patched)
<https://reviews.apache.org/r/68019/#comment297525>
A newline between.
src/linux/seccomp/seccomp_parser.cpp
Lines 363-365 (patched)
<https://reviews.apache.org/r/68019/#comment297527>
So `includes` is a **required** field in a seccomp profile, at least we should have `"includes": {}` in a seccomp profile, right?
src/linux/seccomp/seccomp_parser.cpp
Lines 516 (patched)
<https://reviews.apache.org/r/68019/#comment297518>
s/Error reading/Failed to read/
And I think we do not need the word `file` in this message just like the message below.
- Qian Zhang
On Nov. 8, 2018, 11:24 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 11:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
>
>
> Bugs: MESOS-9105
> https://issues.apache.org/jira/browse/MESOS-9105
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
> src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68019/diff/13/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68019: Added a parser for the Docker Seccomp config
format.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68019/
-----------------------------------------------------------
(Updated Nov. 8, 2018, 3:24 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
Bugs: MESOS-9105
https://issues.apache.org/jira/browse/MESOS-9105
Repository: mesos
Description
-------
Docker Seccomp config is a JSON file containing Seccomp filtering
rules. This patch introduces a parser for Docker Seccomp config format.
This parser accepts a JSON-string, parses and validates it, then
returns a prepared `ContainerSeccompProfile` message.
Diffs (updated)
-----
src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8
src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190
src/linux/seccomp/seccomp_parser.hpp PRE-CREATION
src/linux/seccomp/seccomp_parser.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/68019/diff/4/
Changes: https://reviews.apache.org/r/68019/diff/3-4/
Testing
-------
Thanks,
Andrei Budnik