[GitHub] [openwhisk] sven-lange-last commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images
sven-lange-last commented on a change in pull request #4503: Add optional
config for appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r325625604
##
File path:
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainerFactory.scala
##
@@ -60,10 +62,14 @@ class DockerContainerFactory(instance: InvokerInstanceId,
userProvidedImage: Boolean,
memory: ByteSize,
cpuShares: Int)(implicit config: WhiskConfig,
logging: Logging): Future[Container] = {
+val registryConfig =
+ ContainerFactory.resolveRegisterConfig(userProvidedImage,
runtimesRegistryConfig, userImagesRegistryConfig)
+val image =
+ if (userProvidedImage && registryConfig.url.isEmpty) Left(actionImage)
+ else Right(actionImage.localImageName(registryConfig.url))
Review comment:
Yes, since your PR does not provide a list with mappings for registry
domains but only a single user image repository domain, all user provided image
refs would be overwritten - regardless whether the user provided image ref
already contains a domain or not.
This current approach would lead to:
* User registry config empty, user-provided image ref `image:latest` =>
Docker container factory pulls `docker.io/library/image:latest`.
* User registry config empty, user-provided image ref
`docker.io/library/image:latest` => Docker container factory pulls
`docker.io/library/image:latest`.
* User registry config `uk.icr.io`, user-provided image ref `image:latest`
=> Docker container factory pulls `uk.icr.io/image:latest`.
The user would probably expect to pull `uk.icr.io/library/image:latest`
instead - but `docker pull` only inserts the "official" repository path
`library` if `docker.io` is specified as domain. See
https://github.com/docker/distribution/blob/14b96e55d84c367ebab6caf9f0086de0876eec72/reference/normalize.go#L88-L105.
* User registry config `uk.icr.io`, user-provided image ref
`path/image:latest` => Docker container factory pulls
`uk.icr.io/path/image:latest`.
* User registry config `uk.icr.io`, user-provided image ref
`docker.io/path/image:latest` => Docker container factory pulls
`uk.icr.io/path/image:latest`.
The user would certainly not expect this outcome. If you provide an image
ref with a domain, you would not expect that it is replaced.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] sven-lange-last commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images
sven-lange-last commented on a change in pull request #4503: Add optional
config for appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r325625604
##
File path:
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainerFactory.scala
##
@@ -60,10 +62,14 @@ class DockerContainerFactory(instance: InvokerInstanceId,
userProvidedImage: Boolean,
memory: ByteSize,
cpuShares: Int)(implicit config: WhiskConfig,
logging: Logging): Future[Container] = {
+val registryConfig =
+ ContainerFactory.resolveRegisterConfig(userProvidedImage,
runtimesRegistryConfig, userImagesRegistryConfig)
+val image =
+ if (userProvidedImage && registryConfig.url.isEmpty) Left(actionImage)
+ else Right(actionImage.localImageName(registryConfig.url))
Review comment:
Yes, since your PR does not provide a list with mappings for registry
domains but only a single user image repository domain, all user provided image
refs would be overwritten - regardless whether the user provided image ref
already contains a domain or not.
This current approach would lead to:
With a user registry config like `uk.icr.io`, this would lead to situations
like:
* User registry config empty, user-provided image ref `image:latest` =>
Docker container factory pulls `docker.io/library/image:latest`.
* User registry config empty, user-provided image ref
`docker.io/library/image:latest` => Docker container factory pulls
`docker.io/library/image:latest`.
* User registry config `uk.icr.io`, user-provided image ref `image:latest`
=> Docker container factory pulls `uk.icr.io/image:latest`.
The user would probably expect to pull `uk.icr.io/library/image:latest`
instead - but `docker pull` only inserts the "official" repository path
`library` if `docker.io` is specified as domain. See
https://github.com/docker/distribution/blob/14b96e55d84c367ebab6caf9f0086de0876eec72/reference/normalize.go#L88-L105.
* User registry config `uk.icr.io`, user-provided image ref
`path/image:latest` => Docker container factory pulls
`uk.icr.io/path/image:latest`.
* User registry config `uk.icr.io`, user-provided image ref
`docker.io/path/image:latest` => Docker container factory pulls
`uk.icr.io/path/image:latest`.
The user would certainly not expect this outcome. If you provide an image
ref with a domain, you would not expect that it is replaced.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] sven-lange-last commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images
sven-lange-last commented on a change in pull request #4503: Add optional
config for appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r325577206
##
File path:
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainerFactory.scala
##
@@ -60,10 +62,14 @@ class DockerContainerFactory(instance: InvokerInstanceId,
userProvidedImage: Boolean,
memory: ByteSize,
cpuShares: Int)(implicit config: WhiskConfig,
logging: Logging): Future[Container] = {
+val registryConfig =
+ ContainerFactory.resolveRegisterConfig(userProvidedImage,
runtimesRegistryConfig, userImagesRegistryConfig)
+val image =
+ if (userProvidedImage && registryConfig.url.isEmpty) Left(actionImage)
+ else Right(actionImage.localImageName(registryConfig.url))
Review comment:
My understanding is that the following behaviour is desired for blackbox
(i.e. user provided) images identified by a user provided image reference (see
https://github.com/docker/distribution/blob/master/reference/reference.go):
* If the image reference contains a domain, keep the image reference as is
to pull from the repository in the image reference.
* If the image reference contains NO domain and NO global user image
registry is configured (`registryConfig.url.isEmpty == true`), keep the image
reference as is. It's up to the used container factory to convert to a
canonical reference including a domain and path.
* If the image reference contains NO domain and a global user image registry
is configured (`registryConfig.url.isEmpty == false`), add the global user
image registry as domain to the image reference.
I think this is exactly the behaviour that following code implements:
```
val registryConfig =
ContainerFactory.resolveRegisterConfig(userProvidedImage,
runtimesRegistryConfig, userImagesRegistryConfig)
val imageRef = ContainerFactory.resolveImage(actionImage, registryConfig)
```
That's why I proposed this code because it already addresses the three cases
listed above.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] sven-lange-last commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images
sven-lange-last commented on a change in pull request #4503: Add optional
config for appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r325263353
##
File path:
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainerFactory.scala
##
@@ -60,10 +62,14 @@ class DockerContainerFactory(instance: InvokerInstanceId,
userProvidedImage: Boolean,
memory: ByteSize,
cpuShares: Int)(implicit config: WhiskConfig,
logging: Logging): Future[Container] = {
+val registryConfig =
+ ContainerFactory.resolveRegisterConfig(userProvidedImage,
runtimesRegistryConfig, userImagesRegistryConfig)
+val image =
+ if (userProvidedImage && registryConfig.url.isEmpty) Left(actionImage)
+ else Right(actionImage.localImageName(registryConfig.url))
Review comment:
Would this lead to the same result - as done in the Kube container factory
below?
```
val registryConfig =
ContainerFactory.resolveRegisterConfig(userProvidedImage,
runtimesRegistryConfig, userImagesRegistryConfig)
val imageRef = ContainerFactory.resolveImage(actionImage, registryConfig)
val image = if (userProvidedImage) Left(imageRef) else Right(imageRef)
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] sven-lange-last commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images
sven-lange-last commented on a change in pull request #4503: Add optional
config for appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r325259405
##
File path:
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainerFactory.scala
##
@@ -60,10 +62,14 @@ class DockerContainerFactory(instance: InvokerInstanceId,
userProvidedImage: Boolean,
memory: ByteSize,
cpuShares: Int)(implicit config: WhiskConfig,
logging: Logging): Future[Container] = {
+val registryConfig =
+ ContainerFactory.resolveRegisterConfig(userProvidedImage,
runtimesRegistryConfig, userImagesRegistryConfig)
+val image =
+ if (userProvidedImage && registryConfig.url.isEmpty) Left(actionImage)
+ else Right(actionImage.localImageName(registryConfig.url))
Review comment:
I have the expression that this code can be streamlined as follows to
simplify the condition in line 68 (`(userProvidedImage &&
registryConfig.url.isEmpty)`):
```
val registryConfig =
ContainerFactory.resolveRegisterConfig(userProvidedImage,
runtimesRegistryConfig, userImagesRegistryConfig)
val imageRef = actionImage.localImageName(registryConfig.url)
val image = if (userProvidedImage) Left(imageRef) else Right(imageRef)
```
My understanding is that `actionImage.localImageName()` would just leave out
the registry part if no registry urls are specified.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [openwhisk] sven-lange-last commented on a change in pull request #4503: Add optional config for appending custom registry to user provided images
sven-lange-last commented on a change in pull request #4503: Add optional
config for appending custom registry to user provided images
URL: https://github.com/apache/openwhisk/pull/4503#discussion_r325249961
##
File path:
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerFactory.scala
##
@@ -111,6 +111,20 @@ object ContainerFactory {
/** include the instance name, if specified and strip invalid chars before
attempting to use them in the container name */
def containerNamePrefix(instanceId: InvokerInstanceId): String =
s"wsk${instanceId.uniqueName.getOrElse("")}${instanceId.toInt}".filter(isAllowed)
+
+ def resolveRegisterConfig(userProvidedImage: Boolean,
+runtimesRegistryConfig: RuntimesRegistryConfig,
+userImagesRegistryConfig: RuntimesRegistryConfig):
RuntimesRegistryConfig = {
Review comment:
Wouldn't it make sense to name this function `resolveRegistryConfig()`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
