Closed tdhooks closed 2 years ago
Hi @tdhooks , thanks for putting this MR together, that's great.
All LGTM so far. As you say, I believe it's only missing at least 3 tests:
1) A test for serverIsInsecure
2) An integration test checking the pulling from insecure registry fails
3) An integration test checking the pulling from insecure registry succeeds if properly configured
For 1, a unittest would suffice. test_CLI.cpp is a good source of inspiration.
For 2 and 3:
Integration tests are run through the script sarus-itest-standalone. They take the stadanlone sarus archive and use it inside a docker container to run the python-driven tests. The simplest solution that could be tried would be to run, in the same integration test container, a localhost insecure registry with Docker's registry image using sarus. sarus pull registry:2; sarus run registry:2 &
. That could be done as part of _run_cmd_in_container
for the integration tests functions. A more engineered and solid way to solve this would require several refactorings of the CI scripts and would be out of scope for this task IMHO.
Two things to consider:
sarus.json
used in the tests to add the insecure registry exception, test_environment_variables.py serves as an example (even if not the cleanest). Alternatively, as part of the _run_cmd_in_container
this insecure registry could be set for all integration tests at bash level. That would be even better, as the plan would be to use the local registry for most of the integration tests in the future.Wdyt?
Unit tests are in. I just added an insecure registry to the test config and followed the pattern in test_CLI.cpp
.
As for integration tests, simply running sarus pull registry:2; sarus run registry:2 &
before tests in _run_cmd_in_container
could be tricky, because there'd be no good way to push a test image from the container itself without doing something wacky like docker in docker.
What could work is exposing the container's port 5000 and pushing from the host, but I'm not sure how we'd handle that within the current test framework, because we'd need to run an image and execute the push concurrently, resolve the container network location, and keep dependent events in sync.
It also may make sense to host the registry in another container alongside the test container more as part of the test's environment than the test itself. That would look like, within sarus-itest-standalone
, starting and pushing to a registry container before running tests with _run_cmd_in_container
and taking it down afterward. The only complication there, besides just adding more moving parts in general, would be container networking, but I don't think it would be a big deal to have _run_cmd_in_container
run containers on a bridge network, or add another similar function that does and only use it when warranted.
Of course finding or hosting a registry image that has an image pre-loaded is ideal, but I haven't been able to find one, especially not from a trusted source. Overall I favor the two-container approach if this isn't an option.
Amazing! I suggested the "simpler" option to avoid you the trouble of dealing with yet another moving piece and the cross-container network issue, but clearly you've already seen that coming :)
The way I was thinking before going that path was trying to keep the images on the host (inside the caches
folder) and bind-mounting that in the registry (host -> docker -> sarus). Have a look at https://docs.docker.com/registry/deploying/#customize-the-storage-location. The hope is for this to be easier, but it may prove false, in which case we'd be left with the 2-container-based approach.
In any of the two cases, we'll need a preprocessing step for the integration tests that does the pull + tag + push to the local insecure registry of the images used in integration tests (typically alpine, ubuntu, etc.), from the docker container, ideally without leaving root-owned stuff on the host. Maybe pytest can help there with a session-level fixture or maybe easier with just an earlier bash function call. Your call! (no pun intended)
Thanks for your help btw
Integration tests are up. I was able to get the local (in container) registry set up using the cache directory without needing to write root owned files to the host. I think the separate container approach could probably be done without too much trouble using container network mode, but it probably isn't worth it for these few tests.
I wasn't able to get the 'from scratch' test pipeline running, but unless the test environment is appreciably different it should work the same. Worth double checking anyway, though.
Hi @tdhooks, thanks for all your work on this PR! We are having some issues with Travis CI lately, but I have run the tests on my laptop (also the builds from source on OpenSUSE and Ubuntu) and some stages of our internal pipeline and they work fine.
From my point of view everything's there to remove the Draft status and promote this to a proper PR.
I have a few observations which I'll comment directly on the code.
I've addressed and responded to comments and left them up to you all to resolve. All tests pass on my end.
LGTM
Hi @tdhooks, code is looking good from my side as well. As you might be aware from the contribution guidelines, we are required to ask you (as a first time contributor) to sign a Contributors License Agreement (CLA) with ETH Zurich. I have sent the form to the email address I found in your GitHub profile, could I ask if you have received it? Thanks a lot!
I have received the CLA form and will get to it soon.
CLA returned and conflicts resolved. Should be good to go.
I confirm reception of the CLA, and everything looking good in the code. Thanks again @tdhooks for the first external contribution! :tada:
Adds support for insecure registries via an
insecureRegistries
array insarus.json
. This field functions the same as theinsecure-registries
field in docker'sdaemon.json
. If the registry host for the given pull is listed in this configuration field, sarus will not use secure http by default and will not validate certificates. Security enforcement is enabled by default and will be carried out in all cases except when configured not to.This PR remains WIP until the following are resolved:
closes #23