[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