You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bm...@apache.org on 2015/10/16 20:31:54 UTC
mesos git commit: Fixed and added tests for docker image name parsing.
Repository: mesos
Updated Branches:
refs/heads/master ebde436d0 -> 2332fd5b7
Fixed and added tests for docker image name parsing.
Review: https://reviews.apache.org/r/39353
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/2332fd5b
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/2332fd5b
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/2332fd5b
Branch: refs/heads/master
Commit: 2332fd5b7ec228161eafe7dd45d6b6023b844297
Parents: ebde436
Author: Benjamin Mahler <be...@gmail.com>
Authored: Thu Oct 15 11:38:04 2015 -0700
Committer: Benjamin Mahler <be...@gmail.com>
Committed: Fri Oct 16 10:58:23 2015 -0700
----------------------------------------------------------------------
include/mesos/mesos.proto | 5 +-
.../provisioner/docker/message.hpp | 71 ++++++++++++++++----
.../provisioner/docker/message.proto | 2 +
.../containerizer/provisioner/docker/store.cpp | 9 +--
.../containerizer/provisioner_docker_tests.cpp | 59 ++++++++++++++++
5 files changed, 126 insertions(+), 20 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/2332fd5b/include/mesos/mesos.proto
----------------------------------------------------------------------
diff --git a/include/mesos/mesos.proto b/include/mesos/mesos.proto
index f2ea4fc..9400434 100644
--- a/include/mesos/mesos.proto
+++ b/include/mesos/mesos.proto
@@ -1313,7 +1313,10 @@ message Image {
}
message Docker {
- // The name of the image. Expected in format repository[:tag].
+ // The name of the image. Expected format:
+ // [REGISTRY_HOST[:REGISTRY_PORT]/]REPOSITORY[:TAG|@TYPE:DIGEST]
+ //
+ // See: https://docs.docker.com/reference/commandline/pull/
required string name = 1;
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/2332fd5b/src/slave/containerizer/provisioner/docker/message.hpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/provisioner/docker/message.hpp b/src/slave/containerizer/provisioner/docker/message.hpp
index 6368bf4..466e6f8 100644
--- a/src/slave/containerizer/provisioner/docker/message.hpp
+++ b/src/slave/containerizer/provisioner/docker/message.hpp
@@ -29,25 +29,70 @@ namespace internal {
namespace slave {
namespace docker {
-inline Image::Name parseName(const std::string& value)
+// Docker expects the image to be specified on the command line as:
+// [REGISTRY_HOST[:REGISTRY_PORT]/]REPOSITORY[:TAG|@TYPE:DIGEST]
+//
+// This format is inherently ambiguous when dealing with repository
+// names that include forward slashes. To disambiguate, the docker
+// code looks for '.', or ':', or 'localhost' to decide if the
+// first component is a registry or a respository name. For more
+// detail, drill into the implementation of docker pull.
+//
+// TODO(bmahler): We currently store the digest as a tag, does
+// that makes sense?
+//
+// TODO(bmahler): Validate based on docker's validation logic
+// and return a Try here.
+inline Image::Name parseImageName(std::string s)
{
- Image::Name imageName;
- Option<std::string> registry = None();
- std::vector<std::string> components = strings::split(value, "/");
- if (components.size() > 2) {
- imageName.set_registry(value.substr(0, value.find_last_of("/")));
+ Image::Name name;
+
+ // Extract the digest.
+ if (strings::contains(s, "@")) {
+ std::vector<std::string> split = strings::split(s, "@");
+
+ s = split[0];
+ name.set_tag(split[1]);
}
- std::size_t found = components.back().find_last_of(':');
- if (found == std::string::npos) {
- imageName.set_repository(components.back());
- imageName.set_tag("latest");
+ // Remove the tag. We need to watch out for a
+ // host:port registry, which also contains ':'.
+ if (strings::contains(s, ":")) {
+ std::vector<std::string> split = strings::split(s, ":");
+
+ // The tag must be the last component. If a slash is
+ // present there is a registry port and no tag.
+ if (!strings::contains(split.back(), "/")) {
+ name.set_tag(split.back());
+ split.pop_back();
+
+ s = strings::join(":", split);
+ }
+ }
+
+ // Default to the 'latest' tag when omitted.
+ if (name.tag().empty()) {
+ name.set_tag("latest");
+ }
+
+ // Extract the registry and repository. The first component can
+ // either be the registry, or the first part of the repository!
+ // We resolve this ambiguity using the same hacks used in the
+ // docker code ('.', ':', 'localhost' indicate a registry).
+ std::vector<std::string> split = strings::split(s, "/", 2);
+
+ if (split.size() == 1) {
+ name.set_repository(s);
+ } else if (strings::contains(split[0], ".") ||
+ strings::contains(split[0], ":") ||
+ split[0] == "localhost") {
+ name.set_registry(split[0]);
+ name.set_repository(split[1]);
} else {
- imageName.set_repository(components.back().substr(0, found));
- imageName.set_tag(components.back().substr(found + 1));
+ name.set_repository(s);
}
- return imageName;
+ return name;
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/2332fd5b/src/slave/containerizer/provisioner/docker/message.proto
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/provisioner/docker/message.proto b/src/slave/containerizer/provisioner/docker/message.proto
index edbb1a8..c33e0c5 100644
--- a/src/slave/containerizer/provisioner/docker/message.proto
+++ b/src/slave/containerizer/provisioner/docker/message.proto
@@ -29,6 +29,8 @@ message Image {
message Name {
optional string registry = 1;
required string repository = 2;
+
+ // TODO(bmahler): This may hold a tag or a digest, split these?
required string tag = 3;
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/2332fd5b/src/slave/containerizer/provisioner/docker/store.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/provisioner/docker/store.cpp b/src/slave/containerizer/provisioner/docker/store.cpp
index 637c97c..5034013 100644
--- a/src/slave/containerizer/provisioner/docker/store.cpp
+++ b/src/slave/containerizer/provisioner/docker/store.cpp
@@ -157,13 +157,10 @@ Future<vector<string>> StoreProcess::get(const mesos::Image& image)
return Failure("Docker provisioner store only supports Docker images");
}
- Try<Image::Name> imageName = parseName(image.docker().name());
- if (imageName.isError()) {
- return Failure("Unable to parse docker image name: " + imageName.error());
- }
+ Image::Name imageName = parseImageName(image.docker().name());
- return metadataManager->get(imageName.get())
- .then(defer(self(), &Self::_get, imageName.get(), lambda::_1))
+ return metadataManager->get(imageName)
+ .then(defer(self(), &Self::_get, imageName, lambda::_1))
.then(defer(self(), &Self::__get, lambda::_1));
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/2332fd5b/src/tests/containerizer/provisioner_docker_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer/provisioner_docker_tests.cpp b/src/tests/containerizer/provisioner_docker_tests.cpp
index 0436413..7475b63 100644
--- a/src/tests/containerizer/provisioner_docker_tests.cpp
+++ b/src/tests/containerizer/provisioner_docker_tests.cpp
@@ -70,6 +70,65 @@ namespace mesos {
namespace internal {
namespace tests {
+
+TEST(DockerUtilsTest, ParseImageName)
+{
+ slave::docker::Image::Name name;
+
+ name = parseImageName("library/busybox");
+ EXPECT_FALSE(name.has_registry());
+ EXPECT_EQ("library/busybox", name.repository());
+ EXPECT_EQ("latest", name.tag());
+
+ name = parseImageName("busybox");
+ EXPECT_FALSE(name.has_registry());
+ EXPECT_EQ("busybox", name.repository());
+ EXPECT_EQ("latest", name.tag());
+
+ name = parseImageName("library/busybox:tag");
+ EXPECT_FALSE(name.has_registry());
+ EXPECT_EQ("library/busybox", name.repository());
+ EXPECT_EQ("tag", name.tag());
+
+ // Note that the digest is stored as a tag.
+ name = parseImageName(
+ "library/busybox"
+ "@sha256:bc8813ea7b3603864987522f02a7"
+ "6101c17ad122e1c46d790efc0fca78ca7bfb");
+ EXPECT_FALSE(name.has_registry());
+ EXPECT_EQ("library/busybox", name.repository());
+ EXPECT_EQ("sha256:bc8813ea7b3603864987522f02a7"
+ "6101c17ad122e1c46d790efc0fca78ca7bfb",
+ name.tag());
+
+ name = parseImageName("registry.io/library/busybox");
+ EXPECT_EQ("registry.io", name.registry());
+ EXPECT_EQ("library/busybox", name.repository());
+ EXPECT_EQ("latest", name.tag());
+
+ name = parseImageName("registry.io/library/busybox:tag");
+ EXPECT_EQ("registry.io", name.registry());
+ EXPECT_EQ("library/busybox", name.repository());
+ EXPECT_EQ("tag", name.tag());
+
+ name = parseImageName("registry.io:80/library/busybox:tag");
+ EXPECT_EQ("registry.io:80", name.registry());
+ EXPECT_EQ("library/busybox", name.repository());
+ EXPECT_EQ("tag", name.tag());
+
+ // Note that the digest is stored as a tag.
+ name = parseImageName(
+ "registry.io:80/library/busybox"
+ "@sha256:bc8813ea7b3603864987522f02a7"
+ "6101c17ad122e1c46d790efc0fca78ca7bfb");
+ EXPECT_EQ("registry.io:80", name.registry());
+ EXPECT_EQ("library/busybox", name.repository());
+ EXPECT_EQ("sha256:bc8813ea7b3603864987522f02a7"
+ "6101c17ad122e1c46d790efc0fca78ca7bfb",
+ name.tag());
+}
+
+
/**
* Provides token operations and defaults.
*/