Replace shiplift with bollard for Docker 25+ compatibility #595
Conversation
meerd
force-pushed
the
docker_fix
branch
9 times, most recently
from
e5f2c2f
to
10939fe
Compare
The `shiplift` library which is currently used to interact with the docker API is unmaintained. Switch to `bollard` which is an actively maintained and fairly popular library. This fixes aws#537 Signed-off-by: Erdem Meydanli <[email protected]>
Add additional simple tests to docker.rs for better test coverage. Signed-off-by: Erdem Meydanli <[email protected]>
Extract inspect method in docker.rs to eliminate repetitive code. Signed-off-by: Erdem Meydanli <[email protected]>
Extract parse_docker_host method from get_credentials for reusability. Signed-off-by: Erdem Meydanli <[email protected]>
Extract build_tarball method from build_image in docker.rs for reusability. Signed-off-by: Erdem Meydanli <[email protected]>
Create Runtime instances at the beginning of method blocks in docker.rs for consistency. Signed-off-by: Erdem Meydanli <[email protected]>
Extract stream output handling code into handle_stream_output function and move to utils.rs Signed-off-by: Erdem Meydanli <[email protected]>
|
Update: Testing is in progress. This PR will be merged soon. |
There was a problem hiding this comment.
I have done some testing around this to see if this works as expected on current and older versions of docker. I used different base AMIs to get some older versions of docker to run. The results are summarized in the table below.
| OS | Docker version | Working without PR | Working with PR |
|---|---|---|---|
| Ubuntu 18 | 20.10.21 | ✅ | ✅ |
| Ubuntu 22 | 24.0.5 | ✅ | ✅ |
| AL2 | 17.12.11 | ✅ | ✅ |
| AL2 | 20.10.25 | ✅ | ✅ |
| AL2023 | 25.0.3 | ❌ | ✅ |
I did not spend the extra time to go even further back in Docker version history, given that Docker 17.12 moved into EOL in February 2018.
Overall, I think the results show that the update works will with older Docker versions, while also enabling the use of nitro-cli with newer versions.
Footnotes
-
For the testcase with Docker 17 on AL2, I had to downgrade the docker package and some other conflicting packages, so this test case is somewhat flaky. But it shows that we are backwards compatible until then. ↩
meerd
changed the title
| // if docker daemon address needs to be substituted. | ||
| // By default, it tries to connect to 'unix:///var/run/docker.sock' | ||
| let docker = Docker::connect_with_defaults().map_err(|e| { | ||
| error!("{:?}", e); |
There was a problem hiding this comment.
nit: The error should be encapsulated into the ConnectionError and printed when it is handled.
| let credentials = match self.get_credentials() { | ||
| Ok(auth) => Some(auth), | ||
| // It is not mandatory to have the credentials set, but this is | ||
| // the most likely reason for failure when pulling, so log the | ||
| // error. | ||
| Err(err) => { | ||
| debug!("WARNING!! Credential could not be set {:?}", err); | ||
| None | ||
| } | ||
| }; |
There was a problem hiding this comment.
nit: Looks like this is replicating existing methods of Result:
let credentials = self.get_credentials()
.inspect_err(|err| debug!("WARNING!! Credential could not be set {:?}", err))
.ok();
enclave_build/src/docker.rs
Outdated
| Ok(image) => Ok(( | ||
| image.config.entrypoint.unwrap(), | ||
| image.config.env.ok_or_else(Vec::<String>::new).unwrap(), | ||
| image.config.clone().unwrap().entrypoint.unwrap(), |
There was a problem hiding this comment.
nit: Should have a comment why unwrap is safe, e.g. because it was checked before. Or even better would be to restructure the code to use the checked value from above.
| Ok(image) => Ok(image), | ||
| Err(e) => { | ||
| error!("{:?}", e); | ||
| Err(DockerError::InspectError) | ||
| } |
There was a problem hiding this comment.
nit: This could be handled by map_err.
| Some(host) => host, | ||
| None => return Err(CredentialsError("Invalid docker image URI!".to_string())), |
There was a problem hiding this comment.
nit: This match could be handled using .ok_or_else(|| CredentialsError(...))?
The
shipliftcrate is outdated and incompatible with Docker 25. This PR replaces the it withbollard, allowingnitro-clito work with Docker version 25 and newer.Special thanks to @jalaziz for their contribution.