Skip to content
This repository was archived by the owner on Oct 28, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 147 additions & 0 deletions src/test/groovy/PushDockerImagesStepTests.groovy
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
// Licensed to Elasticsearch B.V. under one or more contributor
// license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright
// ownership. Elasticsearch B.V. licenses this file to you under
// the Apache License, Version 2.0 (the "License"); you may
// not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

import org.junit.Before
import org.junit.Test
import static org.junit.Assert.assertTrue

class PushDockerImagesStepTests extends ApmBasePipelineTest {

@Override
@Before
void setUp() throws Exception {
super.setUp()
script = loadScript('vars/pushDockerImages.groovy')
env.GIT_BASE_COMMIT = 'commit'
}

@Test
void test_windows() throws Exception {
testWindows() {
script.call()
}
}

@Test
void test_missing_registry() throws Exception {
testMissingArgument('registry') {
script.call()
}
}

@Test
void test_missing_secret() throws Exception {
testMissingArgument('secret') {
script.call(registry: 'foo')
}
}
@Test
void test_missing_targetNamespace() throws Exception {
testMissingArgument('targetNamespace') {
script.call(registry: 'foo', secret: 'bar')
}
}

@Test
void test_missing_version() throws Exception {
testMissingArgument('version') {
script.call(registry: 'foo', secret: 'bar', targetNamespace: 'target')
}
}

@Test
void test_missing_imageName() throws Exception {
testMissingArgument('imageName') {
script.call(registry: 'foo', secret: 'bar', targetNamespace: 'target', version: '1.2')
}
}

@Test
void test_calculateTags_pr() throws Exception {
helper.registerAllowedMethod('isPR', { return true })
env.CHANGE_ID = '1'
def ret = script.calculateTags('8.2-SNAPSHOT', '')
printCallStack()
assertTrue(ret.equals(['commit', 'pr-1']))
}

@Test
void test_calculateTags_branch() throws Exception {
helper.registerAllowedMethod('isPR', { return false })
def ret = script.calculateTags('8.2-SNAPSHOT', '8.2.0-SNAPSHOT')
printCallStack()
assertTrue(ret.equals(['commit', '8.2-SNAPSHOT', '8.2.0-SNAPSHOT']))

ret = script.calculateTags('8.2-SNAPSHOT', '')
printCallStack()
assertTrue(ret.equals(['commit', '8.2-SNAPSHOT']))
}

@Test
void test_doTagAndPush() throws Exception {
helper.registerAllowedMethod('sh', [Map.class], { return 0 })
script.doTagAndPush(registry: 'my-registry',
imageName: 'my-name',
variant: '-cloud',
sourceTag: '8.2-SNAPSHOT',
targetTag: 'commit',
sourceNamespace: 'beats',
targetNamespace: 'beats-ci')
printCallStack()
assertTrue(assertMethodCallContainsPattern('sh', '"my-registry/beats/my-name-cloud:8.2-SNAPSHOT" "my-registry/beats-ci/my-name-cloud:commit"'))
}

@Test
void test_with_snapshots() throws Exception {
helper.registerAllowedMethod('sh', [Map.class], { return 0 })
script.call(
secret: "my-secret",
registry: "my-registry",
arch: 'amd64',
version: '8.2.0',
snapshot: true,
imageName: 'filebeat',
variants: [
'' : 'beats',
'-cloud' : 'beats-ci'
],
targetNamespace: 'observability-ci'
)
printCallStack()
assertTrue(assertMethodCallOccurrences('sh', 12))
}

@Test
void test_without_snapshots() throws Exception {
helper.registerAllowedMethod('sh', [Map.class], { return 0 })
script.call(
secret: "my-secret",
registry: "my-registry",
arch: 'amd64',
version: '8.2.0',
snapshot: false,
imageName: 'filebeat',
variants: [
'' : 'beats',
'-cloud' : 'beats-ci'
],
targetNamespace: 'observability-ci'
)
printCallStack()
assertTrue(assertMethodCallOccurrences('sh', 8))
}
}
139 changes: 139 additions & 0 deletions vars/pushDockerImages.groovy
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
// Licensed to Elasticsearch B.V. under one or more contributor
// license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright
// ownership. Elasticsearch B.V. licenses this file to you under
// the Apache License, Version 2.0 (the "License"); you may
// not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

/**

Publish docker images in the given docker registry. For such, it
retags the existing docker images and publish them in the given
docker namespace.

It also allows to transform the version and customise private docker
namespaces/registries.

pushDockerImages(
secret: "my-secret",
registry: "my-registry",
arch: 'amd64',
version: '8.2.0',
snapshot: true,
imageName : 'filebeat',
variants: [
'' : 'beats',
'ubi8' : 'beats',
'cloud' : 'beats-ci',
'complete' : 'beats',
],
targetNamespace: 'observability-ci'
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think version and arch are redundant, the variants cover the need to push a matrix of Docker images. If we need to specialize the push we can do it in steps designed to implement that layer.

It is more straightforward to have in variants the source tag, the destination tag, and the namespace. even though you have to repeat some configuration values the code will be much simpler and easy to configure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the configuration is the one we need to agree, and what do you propose?


*/
def call(Map args = [:]) {
if(!isUnix()){
error('publishToCDN: windows is not supported yet.')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
error('publishToCDN: windows is not supported yet.')
error('pushDockerImages: windows is not supported yet.')

}
def registry = args.containsKey('registry') ? args.registry : error('pushDockerImages: registry parameter is required')
def secret = args.containsKey('secret') ? args.secret : error('pushDockerImages: secret parameter is required')
def targetNamespace = args.containsKey('targetNamespace') ? args.targetNamespace : error('pushDockerImages: targetNamespace parameter is required')
def version = args.containsKey('version') ? args.version : error('pushDockerImages: version parameter is required')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is stack-oriented variable, it limits the use of the step

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

def imageName = args.containsKey('imageName') ? args.imageName : error('pushDockerImages: imageName parameter is required')
def arch = args.get('arch', 'amd64')
def variants = args.get('variants', [:])
def snapshot = args.get('snapshot', true)

// Transform version in a snapshot.
def sourceTag = version
def aliasVersion = ""
if (snapshot) {
// remove third number in version
aliasVersion = version.substring(0, version.lastIndexOf(".")) + "-SNAPSHOT"
sourceTag += "-SNAPSHOT"
}
Comment on lines +51 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this logic should not be here, it is related to process aliases, but not related to pushes Docker images, also, it is not needed if you pass the correct tag.

def version = '8.0.0'

aliasVersion -> 8.0-SNAPSHOT

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bear in mind, this implementation did nothing but copy the existing one in the packaging.groovy file, so I didn't want to change the contract


// What docker tags are gonna be used
def tags = calculateTags(sourceTag, aliasVersion)

println tags
dockerLogin(secret: "${secret}", registry: "${registry}")
variants?.each { variant, sourceNamespace ->
tags.each { tag ->
// TODO:
// For backward compatibility let's ensure we tag only for amd64, then E2E can benefit from until
// they support the versioning with the architecture
if ("${arch}" == "amd64") {
doTagAndPush(registry: registry, imageName: imageName, variant: variant, sourceTag: sourceTag, targetTag: "${tag}", sourceNamespace: sourceNamespace, targetNamespace: targetNamespace)
}
doTagAndPush(registry: registry, imageName: imageName, variant: variant, sourceTag: sourceTag, targetTag: "${tag}-${arch}", sourceNamespace: sourceNamespace, targetNamespace: targetNamespace)
}
}
}

/**
* Tag and push the source docker image. It retries to add resilience.
*
* @param imageName of the docker image
* @param variant name of the variant used to build the docker image name
* @param sourceNamespace namespace to be used as source for the docker tag command
* @param targetNamespace namespace to be used as target for the docker tag command
* @param sourceTag tag to be used as source for the docker tag command, usually under the 'beats' namespace
* @param targetTag tag to be used as target for the docker tag command, usually under the 'observability-ci' namespace
*/
def doTagAndPush(Map args = [:]) {
def name = args.imageName
def variant = args.variant
def sourceTag = args.sourceTag
def targetTag = args.targetTag
def sourceNamespace = args.sourceNamespace
def targetNamespace = args.targetNamespace
def registry = args.registry

def sourceName = "${registry}/${sourceNamespace}/${name}${variant}:${sourceTag}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure is correct

def registry = 'docker.elastic.co'
def sourceNamespace = 'observability-ci'
def name = 'metricbeat'
variant = 'cloud'
sourceTag = '8.0.0-SNAPSHOT'

docker.elastic.co/observability-ci/metricbeatcloud:8.0.0-SNPASHOT

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your question, sourceNamespace is never observability-ci but the one the consumer decides. That's exactly the same implementation as used to be -> https://github.com/elastic/beats/pull/30414/files#diff-5edb5ab93fc95960129eecb2ee3d8763a9c5e02b56e5b80126e0c5a52296ee8fL352

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha! variant should contain -cloud rather than cloud

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the values are examples to point you to the separator (-) issue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's an issue:

"${registry}/${sourceNamespace}/${name}${variant}:${sourceTag}" then variable names should be defined with the format needed, otherwise what will you use?

def targetName = "${registry}/${targetNamespace}/${name}${variant}:${targetTag}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure is correct

def registry = 'docker.elastic.co'
def sourceNamespace = 'observability-ci'
def name = 'metricbeat'
variant = 'cloud'
sourceTag = '8.0.0-SNAPSHOT'

docker.elastic.co/observability-ci/metricbeatcloud:8.0.0-SNPASHOT

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer hiding the - prefix, and let it as an implementation detail:

variants: [
      '' : 'beats',
      'ubi8' : 'beats',
      'cloud' : 'beats-ci',
      'complete' : 'beats',
    ],

and then evaluate it internally when building the image name: if the variant key is not empty, prepend the -. Otherwise the consumer could be adding whatever they like as variant separator, like ~cloud or even worst :cloud. I know this is weird, but I'd see it better if we are controlling the inputs. We could even sanitise them, removing :,\/; etc...

Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concern, but I'd rather prefer the freedom in case there is a corner case to support something else. No strong opinion though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker on my side either. We have it already working in the old system and this PR is simply moving it to another shareable place 😄 I'm totally OK with keeping it as is.

def iterations = 0

waitUntil {
iterations++
def status = sh(label: "Change tag and push ${targetName}",
script: """#!/bin/bash
set -e
echo "source: '${sourceName}' target: '${targetName}'"
if docker image inspect "${sourceName}" &> /dev/null ; then
docker tag "${sourceName}" "${targetName}"
docker push "${targetName}"
else
echo "docker image ${sourceName} does not exist"
fi""",
returnStatus: true)
if (status > 0 && iterations < 3) {
sleep 5 * iterations
return false
} else if (status > 0) {
return false
} else {
return true
}
}
}

def calculateTags(sourceTag, aliasVersion) {
def tags = [ env.GIT_BASE_COMMIT,
isPR() ? "pr-${env.CHANGE_ID}" : sourceTag ]

if (!isPR() && aliasVersion.trim()) {
tags << aliasVersion
}
return tags
}
38 changes: 38 additions & 0 deletions vars/pushDockerImages.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
Publish docker images in the given docker registry. For such, it
retags the existing docker images and publish them in the given
docker namespace.

It also allows to transform the version and customise private docker
namespaces/registries.

```
// Given the filebeat project, and its generated docker
// images for the 8.2.0-SNAPSHOT and 4 different variants
// then publish them to observability-ci
pushDockerImages(
secret: "my-secret",
registry: "my-registry",
arch: 'amd64',
version: '8.2.0',
snapshot: true,
imageName: 'filebeat',
variants: [
'' : 'beats',
'ubi8' : 'beats',
'cloud' : 'beats-ci',
'complete' : 'beats',
],
targetNamespace: 'observability-ci'
)
```

* secret: the docker secret
* registry: the docker registry
* arch: the supported arch (amd64 or arm64)
* version: what version
* snapshot: snapshot support
* imageName: the docker image name.
* variants: variants and docker namespace to copy from.
* targetNamespace: what docker namespace to publish to.
Copy link
Contributor

@kuisathaverat kuisathaverat Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version, arch and variant are complicated, it is not obvious what will be the tag, maybe it worth a few examples to understand how it works

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above example is not enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think so, you do not know which Docker image is the source and which will be the destination if you do not check the code.


__NOTE__: It requires *Nix where to run it from.