From 5325f2b101ee15e2fc80257f7b62d516bc751c51 Mon Sep 17 00:00:00 2001 From: Paolo Di Tommaso Date: Mon, 29 Apr 2024 20:41:35 +0200 Subject: [PATCH] Improve default container name handling Signed-off-by: Paolo Di Tommaso --- .../controller/ContainerController.groovy | 15 +--- .../seqera/wave/util/ContainerHelper.groovy | 17 ++++- .../controller/ContainerControllerTest.groovy | 45 ++--------- .../service/builder/BuildRequestTest.groovy | 8 +- .../wave/util/ContainerHelperTest.groovy | 76 ++++++++++++------- 5 files changed, 78 insertions(+), 83 deletions(-) diff --git a/src/main/groovy/io/seqera/wave/controller/ContainerController.groovy b/src/main/groovy/io/seqera/wave/controller/ContainerController.groovy index 35d26ab9a..a6983ce65 100644 --- a/src/main/groovy/io/seqera/wave/controller/ContainerController.groovy +++ b/src/main/groovy/io/seqera/wave/controller/ContainerController.groovy @@ -69,7 +69,6 @@ import io.seqera.wave.tower.User import io.seqera.wave.tower.auth.JwtAuthStore import io.seqera.wave.util.DataTimeUtils import io.seqera.wave.util.LongRndKey -import io.seqera.wave.util.StringUtils import jakarta.inject.Inject import jakarta.inject.Named import static io.micronaut.http.HttpHeaders.WWW_AUTHENTICATE @@ -77,10 +76,10 @@ import static io.seqera.wave.WaveDefault.TOWER import static io.seqera.wave.service.builder.BuildFormat.DOCKER import static io.seqera.wave.service.builder.BuildFormat.SINGULARITY import static io.seqera.wave.util.ContainerHelper.checkContainerSpec -import static io.seqera.wave.util.ContainerHelper.makeContainerId import static io.seqera.wave.util.ContainerHelper.condaFileFromRequest import static io.seqera.wave.util.ContainerHelper.containerFileFromPackages import static io.seqera.wave.util.ContainerHelper.decodeBase64OrFail +import static io.seqera.wave.util.ContainerHelper.makeContainerId import static io.seqera.wave.util.ContainerHelper.makeResponseV1 import static io.seqera.wave.util.ContainerHelper.makeResponseV2 import static io.seqera.wave.util.ContainerHelper.makeTargetImage @@ -247,16 +246,6 @@ class ContainerController { } } - protected String publicRepo(SubmitContainerTokenRequest req) { - if( !buildConfig.defaultPublicRepository ) - return null - if( buildConfig.defaultPublicRepository.contains('/') ) - return buildConfig.defaultPublicRepository - return !req.nameStrategy || req.nameStrategy==ImageNameStrategy.imageSuffix - ? StringUtils.pathConcat(buildConfig.defaultPublicRepository, 'library') - : StringUtils.pathConcat(buildConfig.defaultPublicRepository, 'library/build') - } - BuildRequest makeBuildRequest(SubmitContainerTokenRequest req, PlatformId identity, String ip) { if( !req.containerFile ) throw new BadRequestException("Missing dockerfile content") @@ -270,7 +259,7 @@ class ContainerController { final spackContent = spackFileFromRequest(req) final format = req.formatSingularity() ? SINGULARITY : DOCKER final platform = ContainerPlatform.of(req.containerPlatform) - final buildRepository = req.buildRepository ?: (req.freeze && publicRepo(req) ? publicRepo(req) : buildConfig.defaultBuildRepository) + final buildRepository = req.buildRepository ?: (req.freeze && buildConfig.defaultPublicRepository ? buildConfig.defaultPublicRepository : buildConfig.defaultBuildRepository) final cacheRepository = req.cacheRepository ?: buildConfig.defaultCacheRepository final configJson = dockerAuthService.credentialsConfigJson(containerSpec, buildRepository, cacheRepository, identity) final containerConfig = req.freeze ? req.containerConfig : null diff --git a/src/main/groovy/io/seqera/wave/util/ContainerHelper.groovy b/src/main/groovy/io/seqera/wave/util/ContainerHelper.groovy index 9a0603b07..7308d0f2d 100644 --- a/src/main/groovy/io/seqera/wave/util/ContainerHelper.groovy +++ b/src/main/groovy/io/seqera/wave/util/ContainerHelper.groovy @@ -290,7 +290,6 @@ class ContainerHelper { static String makeTargetImage(BuildFormat format, String repo, String id, @Nullable String condaFile, @Nullable String spackFile, @Nullable ImageNameStrategy nameStrategy) { assert id, "Argument 'id' cannot be null or empty" assert repo, "Argument 'repo' cannot be null or empty" - assert repo.contains('/'), "Argument 'repo' is not a valid container repository name" assert format, "Argument 'format' cannot be null" NameVersionPair tools @@ -319,7 +318,7 @@ class ContainerHelper { throw new BadRequestException("Unsupported image naming strategy: '${nameStrategy}'") } - format==SINGULARITY ? "oras://${repo}:${tag}" : "${repo}:${tag}" + format==SINGULARITY ? "oras://${normaliseRepo(repo)}:${tag}" : "${normaliseRepo(repo)}:${tag}" } static protected String normalise0(String tag, int maxLength, String pattern) { @@ -358,6 +357,20 @@ class ContainerHelper { value ? normalise0(value.toLowerCase(), maxLength, /[^a-z0-9_.\-\/]/) : null } + static protected String normaliseRepo(String value) { + if( !value ) + return value + final parts = value.tokenize('/') + if( parts.size()>2 ) + return value + if( parts.size()==1 ) { + return parts[0] + '/library/build' + } + else { + return parts[0] + '/library/' + parts[1] + } + } + static String makeContainerId(String containerFile, String condaFile, String spackFile, ContainerPlatform platform, String repository, BuildContext buildContext) { final attrs = new LinkedHashMap(10) attrs.containerFile = containerFile diff --git a/src/test/groovy/io/seqera/wave/controller/ContainerControllerTest.groovy b/src/test/groovy/io/seqera/wave/controller/ContainerControllerTest.groovy index 9625cad24..e6fe2fe50 100644 --- a/src/test/groovy/io/seqera/wave/controller/ContainerControllerTest.groovy +++ b/src/test/groovy/io/seqera/wave/controller/ContainerControllerTest.groovy @@ -19,7 +19,6 @@ package io.seqera.wave.controller import spock.lang.Specification -import spock.lang.Unroll import java.time.Instant import java.time.temporal.ChronoUnit @@ -32,7 +31,6 @@ import io.micronaut.http.client.annotation.Client import io.micronaut.http.server.util.HttpClientAddressResolver import io.micronaut.test.extensions.spock.annotation.MicronautTest import io.seqera.wave.api.ContainerConfig -import io.seqera.wave.api.ImageNameStrategy import io.seqera.wave.api.PackagesSpec import io.seqera.wave.api.SubmitContainerTokenRequest import io.seqera.wave.api.SubmitContainerTokenResponse @@ -195,7 +193,7 @@ class ContainerControllerTest extends Specification { and: data.containerFile == DOCKER data.identity.userId == 100 - data.containerImage == 'wave/build:be9ee6ac1eeff4b5' + data.containerImage == 'wave/library/build:be9ee6ac1eeff4b5' data.containerConfig == cfg data.platform.toString() == 'linux/arm64' } @@ -224,7 +222,7 @@ class ContainerControllerTest extends Specification { and: data.containerFile == DOCKER data.identity.userId == 100 - data.containerImage == 'wave/build:be9ee6ac1eeff4b5' + data.containerImage == 'wave/library/build:be9ee6ac1eeff4b5' data.containerConfig == cfg data.platform.toString() == 'linux/arm64' } @@ -240,7 +238,7 @@ class ContainerControllerTest extends Specification { then: build.containerId =~ /7efaa2ed59c58a16/ build.containerFile == 'FROM foo' - build.targetImage == 'wave/build:7efaa2ed59c58a16' + build.targetImage == 'wave/library/build:7efaa2ed59c58a16' build.platform == ContainerPlatform.of('amd64') when: @@ -249,7 +247,7 @@ class ContainerControllerTest extends Specification { then: build.containerId =~ /7efaa2ed59c58a16/ build.containerFile == 'FROM foo' - build.targetImage == 'wave/build:7efaa2ed59c58a16' + build.targetImage == 'wave/library/build:7efaa2ed59c58a16' build.platform == ContainerPlatform.of('amd64') // using 'arm' platform changes the id @@ -259,7 +257,7 @@ class ContainerControllerTest extends Specification { then: build.containerId =~ /be9ee6ac1eeff4b5/ build.containerFile == 'FROM foo' - build.targetImage == 'wave/build:be9ee6ac1eeff4b5' + build.targetImage == 'wave/library/build:be9ee6ac1eeff4b5' build.platform == ContainerPlatform.of('arm64') when: @@ -269,7 +267,7 @@ class ContainerControllerTest extends Specification { build.containerId =~ /c6dac2e544419f71/ build.containerFile == 'FROM foo' build.condaFile == 'some::conda-recipe' - build.targetImage == 'wave/build:c6dac2e544419f71' + build.targetImage == 'wave/library/build:c6dac2e544419f71' build.platform == ContainerPlatform.of('arm64') when: @@ -281,7 +279,7 @@ class ContainerControllerTest extends Specification { build.containerFile.startsWith('# Builder image\n') build.condaFile == null build.spackFile == 'some::spack-recipe' - build.targetImage == 'wave/build:b7d730d274d1e057' + build.targetImage == 'wave/library/build:b7d730d274d1e057' build.platform == ContainerPlatform.of('arm64') } @@ -559,33 +557,4 @@ class ContainerControllerTest extends Specification { e.message == "Attribute `packages` is not allowed" } - @Unroll - def 'should return default public repo' () { - given: - def controller = new ContainerController( - inclusionService: Mock(ContainerInclusionService), - allowAnonymous: false, - buildConfig: new BuildConfig(defaultPublicRepository: REPO) - ) - - expect: - controller.publicRepo(new SubmitContainerTokenRequest(nameStrategy: STRATEGY ? ImageNameStrategy.valueOf(STRATEGY) : null)) == EXPECTED - - where: - REPO | STRATEGY | EXPECTED - null | 'none' | null - null | null | null - null | 'tagPrefix' | null - null | 'imageSuffix' | null - and: - 'foo.com' | null | 'foo.com/library' - 'foo.com' | 'none' | 'foo.com/library/build' - 'foo.com' | 'tagPrefix' | 'foo.com/library/build' - 'foo.com' | 'imageSuffix' | 'foo.com/library' - and: - 'foo.com/bar' | null | 'foo.com/bar' - 'foo.com/bar' | 'none' | 'foo.com/bar' - 'foo.com/bar' | 'tagPrefix' | 'foo.com/bar' - 'foo.com/bar' | 'imageSuffix' | 'foo.com/bar' - } } diff --git a/src/test/groovy/io/seqera/wave/service/builder/BuildRequestTest.groovy b/src/test/groovy/io/seqera/wave/service/builder/BuildRequestTest.groovy index bee4d8468..5d769f254 100644 --- a/src/test/groovy/io/seqera/wave/service/builder/BuildRequestTest.groovy +++ b/src/test/groovy/io/seqera/wave/service/builder/BuildRequestTest.groovy @@ -75,7 +75,7 @@ class BuildRequestTest extends Specification { then: req.containerId == '181ec22b26ae6d04' - req.targetImage == "docker.io/wave:${req.containerId}" + req.targetImage == "docker.io/library/wave:${req.containerId}" req.containerFile == CONTENT req.identity == USER req.configJson == '{"config":"json"}' @@ -122,7 +122,7 @@ class BuildRequestTest extends Specification { ) then: req.containerId == '8026e3a63b5c863f' - req.targetImage == 'docker.io/wave:samtools-1.0--8026e3a63b5c863f' + req.targetImage == 'docker.io/library/wave:samtools-1.0--8026e3a63b5c863f' req.condaFile == CONDA_RECIPE req.spackFile == null and: @@ -157,7 +157,7 @@ class BuildRequestTest extends Specification { ) then: req.containerId == '8726782b1d9bb8fb' - req.targetImage == 'docker.io/wave:bwa-0.7.15--8726782b1d9bb8fb' + req.targetImage == 'docker.io/library/wave:bwa-0.7.15--8726782b1d9bb8fb' req.spackFile == SPACK_RECIPE req.condaFile == null and: @@ -201,7 +201,7 @@ class BuildRequestTest extends Specification { ) then: req.containerId == 'd78ba9cb01188668' - req.targetImage == "oras://docker.io/wave:${req.containerId}" + req.targetImage == "oras://docker.io/library/wave:${req.containerId}" req.containerFile == CONTENT req.identity == USER req.configJson == '{"config":"json"}' diff --git a/src/test/groovy/io/seqera/wave/util/ContainerHelperTest.groovy b/src/test/groovy/io/seqera/wave/util/ContainerHelperTest.groovy index a9d3746d6..688282094 100644 --- a/src/test/groovy/io/seqera/wave/util/ContainerHelperTest.groovy +++ b/src/test/groovy/io/seqera/wave/util/ContainerHelperTest.groovy @@ -545,6 +545,21 @@ class ContainerHelperTest extends Specification { '._-xyz._-' | 'xyz' } + @Unroll + def 'should normalise repo' () { + expect: + ContainerHelper.normaliseRepo(REPO) == EXPECTED + where: + REPO | EXPECTED + null | null + 'docker.io/a/b' | 'docker.io/a/b' + 'docker.io/a/b/c' | 'docker.io/a/b/c' + 'docker.io/foo' | 'docker.io/library/foo' + 'docker.io/foo/' | 'docker.io/library/foo' + 'docker.io' | 'docker.io/library/build' + 'docker.io/' | 'docker.io/library/build' + } + def 'should make request target' () { expect: ContainerHelper.makeTargetImage(BuildFormat.DOCKER, 'quay.io/org/name', '12345', null, null, null) @@ -606,42 +621,51 @@ class ContainerHelperTest extends Specification { where: FORMAT | REPO | ID | CONDA | SPACK | STRATEGY | EXPECTED - 'DOCKER' | 'foo.com/build' | '123' | null | null | null | 'foo.com/build:123' - 'DOCKER' | 'foo.com/build' | '123' | null | null | 'none' | 'foo.com/build:123' - 'DOCKER' | 'foo.com/build' | '123' | null | null | 'tagPrefix' | 'foo.com/build:123' - 'DOCKER' | 'foo.com/build' | '123' | null | null | 'imageSuffix' | 'foo.com/build:123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | null | null | 'foo.com/a/b:123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | null | 'none' | 'foo.com/a/b:123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | null | 'tagPrefix' | 'foo.com/a/b:123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | null | 'imageSuffix' | 'foo.com/a/b:123' + 'DOCKER' | 'foo.com' | '123' | null | null | 'imageSuffix' | 'foo.com/library/build:123' and: - 'SINGULARITY' | 'foo.com/build' | '123' | null | null | null | 'oras://foo.com/build:123' - 'SINGULARITY' | 'foo.com/build' | '123' | null | null | 'none' | 'oras://foo.com/build:123' - 'SINGULARITY' | 'foo.com/build' | '123' | null | null | 'tagPrefix' | 'oras://foo.com/build:123' - 'SINGULARITY' | 'foo.com/build' | '123' | null | null | 'imageSuffix' | 'oras://foo.com/build:123' + 'SINGULARITY' | 'foo.com/a/b' | '123' | null | null | null | 'oras://foo.com/a/b:123' + 'SINGULARITY' | 'foo.com/a/b' | '123' | null | null | 'none' | 'oras://foo.com/a/b:123' + 'SINGULARITY' | 'foo.com/a/b' | '123' | null | null | 'tagPrefix' | 'oras://foo.com/a/b:123' + 'SINGULARITY' | 'foo.com/a/b' | '123' | null | null | 'imageSuffix' | 'oras://foo.com/a/b:123' + 'SINGULARITY' | 'foo.com' | '123' | null | null | 'imageSuffix' | 'oras://foo.com/library/build:123' and: - 'DOCKER' | 'foo.com/build' | '123' | CONDA1| null | null | 'foo.com/build:samtools-1.0--123' - 'DOCKER' | 'foo.com/build' | '123' | CONDA1| null | 'none' | 'foo.com/build:123' - 'DOCKER' | 'foo.com/build' | '123' | CONDA1| null | 'tagPrefix' | 'foo.com/build:samtools-1.0--123' + 'DOCKER' | 'foo.com/a/b' | '123' | CONDA1| null | null | 'foo.com/a/b:samtools-1.0--123' + 'DOCKER' | 'foo.com/a/b' | '123' | CONDA1| null | 'none' | 'foo.com/a/b:123' + 'DOCKER' | 'foo.com/a/b' | '123' | CONDA1| null | 'tagPrefix' | 'foo.com/a/b:samtools-1.0--123' + 'DOCKER' | 'foo.com/a/b' | '123' | CONDA1| null | 'imageSuffix' | 'foo.com/a/b/samtools:1.0--123' + 'DOCKER' | 'foo.com' | '123' | CONDA1| null | 'imageSuffix' | 'foo.com/library/samtools:1.0--123' + 'DOCKER' | 'foo.com/library' | '123' | CONDA1| null | 'imageSuffix' | 'foo.com/library/samtools:1.0--123' 'DOCKER' | 'foo.com/build' | '123' | CONDA1| null | 'imageSuffix' | 'foo.com/build/samtools:1.0--123' + and: - 'SINGULARITY' | 'foo.com/build' | '123' | CONDA1| null | null | 'oras://foo.com/build:samtools-1.0--123' - 'SINGULARITY' | 'foo.com/build' | '123' | CONDA1| null | 'none' | 'oras://foo.com/build:123' - 'SINGULARITY' | 'foo.com/build' | '123' | CONDA1| null | 'tagPrefix' | 'oras://foo.com/build:samtools-1.0--123' + 'SINGULARITY' | 'foo.com/a/b' | '123' | CONDA1| null | null | 'oras://foo.com/a/b:samtools-1.0--123' + 'SINGULARITY' | 'foo.com/a/b' | '123' | CONDA1| null | 'none' | 'oras://foo.com/a/b:123' + 'SINGULARITY' | 'foo.com/a/b' | '123' | CONDA1| null | 'tagPrefix' | 'oras://foo.com/a/b:samtools-1.0--123' + 'SINGULARITY' | 'foo.com/a/b' | '123' | CONDA1| null | 'imageSuffix' | 'oras://foo.com/a/b/samtools:1.0--123' + 'SINGULARITY' | 'foo.com/library' | '123' | CONDA1| null | 'imageSuffix' | 'oras://foo.com/library/samtools:1.0--123' 'SINGULARITY' | 'foo.com/build' | '123' | CONDA1| null | 'imageSuffix' | 'oras://foo.com/build/samtools:1.0--123' + and: - 'DOCKER' | 'foo.com/build' | '123' | CONDA2| null | null | 'foo.com/build:samtools-1.0_bamtools-2.0_multiqc-1.15--123' - 'DOCKER' | 'foo.com/build' | '123' | CONDA2| null | 'none' | 'foo.com/build:123' - 'DOCKER' | 'foo.com/build' | '123' | CONDA2| null | 'tagPrefix' | 'foo.com/build:samtools-1.0_bamtools-2.0_multiqc-1.15--123' - 'DOCKER' | 'foo.com/build' | '123' | CONDA2| null | 'imageSuffix' | 'foo.com/build/samtools_bamtools_multiqc:123' + 'DOCKER' | 'foo.com/a/b' | '123' | CONDA2| null | null | 'foo.com/a/b:samtools-1.0_bamtools-2.0_multiqc-1.15--123' + 'DOCKER' | 'foo.com/a/b' | '123' | CONDA2| null | 'none' | 'foo.com/a/b:123' + 'DOCKER' | 'foo.com/a/b' | '123' | CONDA2| null | 'tagPrefix' | 'foo.com/a/b:samtools-1.0_bamtools-2.0_multiqc-1.15--123' + 'DOCKER' | 'foo.com/a/b' | '123' | CONDA2| null | 'imageSuffix' | 'foo.com/a/b/samtools_bamtools_multiqc:123' and: - 'DOCKER' | 'foo.com/build' | '123' | null | SPACK1| null | 'foo.com/build:bwa-0.7.15--123' - 'DOCKER' | 'foo.com/build' | '123' | null | SPACK1| 'none' | 'foo.com/build:123' - 'DOCKER' | 'foo.com/build' | '123' | null | SPACK1| 'tagPrefix' | 'foo.com/build:bwa-0.7.15--123' - 'DOCKER' | 'foo.com/build' | '123' | null | SPACK1| 'imageSuffix' | 'foo.com/build/bwa:0.7.15--123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | SPACK1| null | 'foo.com/a/b:bwa-0.7.15--123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | SPACK1| 'none' | 'foo.com/a/b:123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | SPACK1| 'tagPrefix' | 'foo.com/a/b:bwa-0.7.15--123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | SPACK1| 'imageSuffix' | 'foo.com/a/b/bwa:0.7.15--123' and: - 'DOCKER' | 'foo.com/build' | '123' | null | SPACK2| null | 'foo.com/build:bwa-0.7.15_salmon-1.1.1--123' - 'DOCKER' | 'foo.com/build' | '123' | null | SPACK2| 'none' | 'foo.com/build:123' - 'DOCKER' | 'foo.com/build' | '123' | null | SPACK2| 'tagPrefix' | 'foo.com/build:bwa-0.7.15_salmon-1.1.1--123' - 'DOCKER' | 'foo.com/build' | '123' | null | SPACK2| 'imageSuffix' | 'foo.com/build/bwa_salmon:123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | SPACK2| null | 'foo.com/a/b:bwa-0.7.15_salmon-1.1.1--123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | SPACK2| 'none' | 'foo.com/a/b:123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | SPACK2| 'tagPrefix' | 'foo.com/a/b:bwa-0.7.15_salmon-1.1.1--123' + 'DOCKER' | 'foo.com/a/b' | '123' | null | SPACK2| 'imageSuffix' | 'foo.com/a/b/bwa_salmon:123' } def 'should validate containerfile' () {