fix (buildx) : Always pass --config option for latest versions of Doc…

…ker CLI (fabric8io#1701) + Always pass --config option in case of latest docker + Change default value of --node parameter to null Signed-off-by: Rohan Kumar <[email protected]>
rohanKanojia · Aug 17, 2023 · 2755579 · 2755579
1 parent d730680
commit 2755579
Show file tree

Hide file tree

Showing 7 changed files with 173 additions and 46 deletions.
diff --git a/doc/changelog.md b/doc/changelog.md
@@ -1,5 +1,6 @@
 # ChangeLog
 * **0.44-SNAPSHOT**:
+  - Always pass `--config` option for latest versions of Docker CLI ([1701](https://github.com/fabric8io/docker-maven-plugin/issues/1701))
 
 * **0.43.3** (2023-08-13):
   - Only add `--config` to buildx command string when authentication credentials are coming from outside sources

diff --git a/src/main/asciidoc/inc/build/_buildx.adoc b/src/main/asciidoc/inc/build/_buildx.adoc
@@ -32,7 +32,7 @@ The `<buildx>` element within `<build>` defines how to build multi-architecture
 The builder manages the build cache.
 
 | *nodeName*
-| Specify the name of the node to be created or modified. If none is specified, it is the name of the builder it belongs to, with an index `0` number suffix.
+| Specify the name of the node to be created or modified.
 
 | *configFile*
 | Configuration file for builder.  Non-absolute files are relative to the maven project directory.  If configFile starts with

diff --git a/src/main/java/io/fabric8/maven/docker/service/BuildXService.java b/src/main/java/io/fabric8/maven/docker/service/BuildXService.java
@@ -2,6 +2,7 @@
 
 import io.fabric8.maven.docker.access.AuthConfigList;
 import io.fabric8.maven.docker.access.DockerAccess;
+import io.fabric8.maven.docker.access.util.ExternalCommand;
 import io.fabric8.maven.docker.assembly.BuildDirs;
 import io.fabric8.maven.docker.assembly.DockerAssemblyManager;
 import io.fabric8.maven.docker.config.AttestationConfiguration;
@@ -12,6 +13,7 @@
 import io.fabric8.maven.docker.util.ImageName;
 import io.fabric8.maven.docker.util.Logger;
 import io.fabric8.maven.docker.util.ProjectPaths;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.maven.plugin.MojoExecutionException;
 
 import java.io.BufferedReader;
@@ -68,8 +70,7 @@ protected <C> void useBuilder(ProjectPaths projectPaths, ImageConfiguration imag
         Path configPath = getDockerStateDir(imageConfig.getBuildConfiguration(),  buildDirs);
         List<String> buildX = new ArrayList<>();
         buildX.add("docker");
-        if (authConfig != null && !authConfig.isEmpty() &&
-            !hasAuthForRegistryInDockerConfig(logger, configuredRegistry, authConfig)) {
+        if (isDockerCLINotLegacy() || shouldAddConfigInLegacyDockerCLI(authConfig, configuredRegistry)) {
             buildX.add("--config");
             buildX.add(configPath.toString());
         }
@@ -85,6 +86,31 @@ protected <C> void useBuilder(ProjectPaths projectPaths, ImageConfiguration imag
         }
     }
 
+    private boolean shouldAddConfigInLegacyDockerCLI(AuthConfigList authConfigList, String configuredRegistry) throws MojoExecutionException {
+        return authConfigList != null && !authConfigList.isEmpty() &&
+            !hasAuthForRegistryInDockerConfig(logger, configuredRegistry, authConfigList);
+    }
+
+    private boolean isDockerCLINotLegacy(){
+        DockerVersionExternalCommand dockerVersionExternalCommand = new DockerVersionExternalCommand(logger);
+        try {
+            dockerVersionExternalCommand.execute();
+        } catch (IOException e) {
+            logger.info("Failure in getting docker CLI version", e);
+        }
+        String version = dockerVersionExternalCommand.getVersion();
+        if (StringUtils.isNotBlank(version)) {
+            version = version.replaceAll("(^')|('$)", "");
+            String[] versionParts = version.split("\\.");
+            logger.info("Using Docker CLI " + version);
+            if (versionParts.length >= 3) {
+                int cliMajorVersion = Integer.parseInt(versionParts[0]);
+                return cliMajorVersion >= 23;
+            }
+        }
+        return false;
+    }
+
     protected void createConfigJson(Path configJson, AuthConfigList authConfig) throws MojoExecutionException {
         try (BufferedWriter bufferedWriter = Files.newBufferedWriter(configJson, StandardCharsets.UTF_8,
             StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)
@@ -219,11 +245,14 @@ protected void createDirectory(Path cachePath) {
     protected String createBuilder(Path configPath, List<String> buildX, ImageConfiguration imageConfig, BuildDirs buildDirs) throws MojoExecutionException {
         BuildXConfiguration buildXConfiguration = imageConfig.getBuildConfiguration().getBuildX();
         String builderName = Optional.ofNullable(buildXConfiguration.getBuilderName()).orElse("maven");
-        String nodeName = Optional.ofNullable(buildXConfiguration.getNodeName()).orElse(builderName + "0");
+        String nodeName = buildXConfiguration.getNodeName();
         Path builderPath = configPath.resolve(Paths.get("buildx", "instances", builderName));
         if(Files.notExists(builderPath)) {
             List<String> cmds = new ArrayList<>(buildX);
-            append(cmds, "create", "--driver", "docker-container", "--name", builderName, "--node", nodeName);
+            append(cmds, "create", "--driver", "docker-container", "--name", builderName);
+            if (nodeName != null) {
+                append(cmds, "--node", nodeName);
+            }
             String buildConfig = buildXConfiguration.getConfigFile();
             if(buildConfig != null) {
                 append(cmds, "--config",
@@ -285,4 +314,27 @@ private void pumpStream(InputStream is) {
             });
         }
     }
+
+    public static class DockerVersionExternalCommand extends ExternalCommand {
+        private final StringBuilder outputBuilder;
+        public DockerVersionExternalCommand(Logger logger) {
+            super(logger);
+            outputBuilder = new StringBuilder();
+        }
+
+        @Override
+        protected String[] getArgs() {
+            return new String[] {"docker", "version", "--format", "'{{.Client.Version}}'"};
+        }
+        @Override
+        protected void processLine(String line) {
+            if (StringUtils.isNotBlank(line)) {
+                outputBuilder.append(line);
+            }
+        }
+
+        public String getVersion() {
+            return outputBuilder.toString();
+        }
+    }
 }
diff --git a/src/test/java/io/fabric8/maven/docker/BuildMojoTest.java b/src/test/java/io/fabric8/maven/docker/BuildMojoTest.java
@@ -12,13 +12,16 @@
 import io.fabric8.maven.docker.service.BuildXService;
 import io.fabric8.maven.docker.service.ImagePullManager;
 import org.apache.maven.plugin.MojoExecutionException;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.io.TempDir;
 import org.mockito.ArgumentCaptor;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
+import org.mockito.MockedConstruction;
 import org.mockito.Mockito;
 import org.mockito.junit.jupiter.MockitoExtension;
 
@@ -51,11 +54,22 @@ class BuildMojoTest extends MojoTestBase {
 
     @Mock
     private BuildXService.Exec exec;
+    private MockedConstruction<BuildXService.DockerVersionExternalCommand> dockerVersionExternalCommandMockedConstruction;
 
     private static String getOsDependentBuild(Path buildPath, String docker) {
         return buildPath.resolve(docker).toString().replace('/', File.separatorChar);
     }
 
+    @BeforeEach
+    void setUp() {
+        dockerVersionExternalCommandMockedConstruction = Mockito.mockConstruction(BuildXService.DockerVersionExternalCommand.class);
+    }
+
+    @AfterEach
+    void tearDown() {
+        dockerVersionExternalCommandMockedConstruction.close();
+    }
+
     @Test
     void skipWhenPom() throws IOException, MojoExecutionException {
 
@@ -336,6 +350,7 @@ private void whenMojoExecutes() throws IOException, MojoExecutionException {
 
     private BuildXConfiguration.Builder getBuildXPlatforms(String... platforms) {
         return new BuildXConfiguration.Builder()
+            .nodeName("maven0")
             .platforms(Arrays.asList(platforms));
     }
 

diff --git a/src/test/java/io/fabric8/maven/docker/PushMojoBuildXTest.java b/src/test/java/io/fabric8/maven/docker/PushMojoBuildXTest.java
@@ -23,6 +23,8 @@
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.MockedConstruction;
 import org.mockito.MockedStatic;
 import org.sonatype.plexus.components.sec.dispatcher.SecDispatcher;
@@ -57,6 +59,8 @@ class PushMojoBuildXTest {
   private File expectedDockerStateConfigDir;
   private Settings mockedMavenSettings;
   private MockedConstruction<BuildXService.DefaultExec> defaultExecMockedConstruction;
+  private MockedConstruction<BuildXService.DockerVersionExternalCommand> dockerVersionExternalCommandMockedConstruction;
+  private String dockerClIVersion;
 
   @BeforeEach
   void setup() throws MojoExecutionException, MojoFailureException, IOException, ComponentLookupException, SecDispatcherException {
@@ -84,6 +88,9 @@ void setup() throws MojoExecutionException, MojoFailureException, IOException, C
     when(mockedSecDispatcher.decrypt(anyString())).thenReturn("testpassword");
     Map<String, Object> pluginContext = new HashMap<>();
     defaultExecMockedConstruction = mockConstruction(BuildXService.DefaultExec.class);
+    dockerVersionExternalCommandMockedConstruction = mockConstruction(BuildXService.DockerVersionExternalCommand.class, (mock, ctx) -> {
+      when(mock.getVersion()).thenReturn(dockerClIVersion);
+    });
     this.pushMojo = new PushMojo();
     this.pushMojo.setPluginContext(pluginContext);
     pushMojo.verbose = "true";
@@ -100,31 +107,41 @@ void setup() throws MojoExecutionException, MojoFailureException, IOException, C
   @AfterEach
   void tearDown() {
     defaultExecMockedConstruction.close();
+    dockerVersionExternalCommandMockedConstruction.close();
   }
 
   @Test
-  @DisplayName("docker:push buildx, no auth, then don't add --config option to buildx")
-  void execute_whenNoAuthConfig_thenRunBuildXCommandWithAddedConfig() throws MojoExecutionException, MojoFailureException {
+  @DisplayName("legacy docker CLI, docker:push buildx, no auth, then don't add --config option to buildx")
+  void execute_whenNoAuthConfig_thenRunBuildXCommandWithNoConfig() throws MojoExecutionException, MojoFailureException {
     // Given
+    givenDockerCLIVersion("20.0.14");
+
     // When
     pushMojo.execute();
 
     // Then
-    assertEquals(1, defaultExecMockedConstruction.constructed().size());
-    BuildXService.DefaultExec defaultExec = defaultExecMockedConstruction.constructed().get(0);
-    verify(defaultExec).process(Arrays.asList("docker", "buildx", "create",
-        "--driver", "docker-container", "--name", "testbuilder", "--node", "testnode"));
-    verify(defaultExec).process(Arrays.asList("docker", "buildx", "build",
-        "--progress=plain", "--builder", "testbuilder", "--platform", "linux/amd64,linux/arm64",
-        "--tag", "test.example.org/testuser/sample-test-image:latest",
-        expectedDockerStateDir.resolve("build").toFile().getAbsolutePath(), "--push"));
+    verifyNoDockerConfigAddedToBuildX();
+  }
+
+  @Test
+  @DisplayName("docker CLI 23.0.6, docker:push buildx, no auth, then add --config option to buildx")
+  void execute_whenNoAuthConfigAndDockerCLIPost23_thenRunBuildXCommandWithAddedConfig() throws MojoExecutionException, MojoFailureException {
+    // Given
+    givenDockerCLIVersion("23.0.6+azure-2");
+
+    // When
+    pushMojo.execute();
+
+    // Then
+    verifyDockerConfigOptionAddedToBuildX();
   }
 
   @Test
-  @DisplayName("docker:push buildx, auth from ~/.docker/config.json, then don't add --config option to buildx")
-  void execute_whenAuthConfigFromLocalDockerConfig_thenDoNotAddConfigToDockerBuildXCommand() throws MojoExecutionException, MojoFailureException {
+  @DisplayName("legacy docker CLI, docker:push buildx, auth from ~/.docker/config.json, then don't add --config option to buildx")
+  void execute_whenLegacyDockerCLIAndAuthConfigFromLocalDockerConfig_thenDoNotAddConfigToDockerBuildXCommand() throws MojoExecutionException, MojoFailureException {
     try (MockedStatic<DockerFileUtil> dockerFileUtilMockedStatic = mockStatic(DockerFileUtil.class)) {
       // Given
+      givenDockerCLIVersion("20.0.14");
       AuthConfig authConfig = new AuthConfig("testuser", "testpassword", null, null, null);
       authConfig.setRegistry("test.example.org");
       dockerFileUtilMockedStatic.when(DockerFileUtil::readDockerConfig)
@@ -134,21 +151,34 @@ void execute_whenAuthConfigFromLocalDockerConfig_thenDoNotAddConfigToDockerBuild
       pushMojo.execute();
 
       // Then
-      assertEquals(1, defaultExecMockedConstruction.constructed().size());
-      BuildXService.DefaultExec defaultExec = defaultExecMockedConstruction.constructed().get(0);
-      verify(defaultExec).process(Arrays.asList("docker", "buildx", "create",
-          "--driver", "docker-container", "--name", "testbuilder", "--node", "testnode"));
-      verify(defaultExec).process(Arrays.asList("docker", "buildx", "build",
-          "--progress=plain", "--builder", "testbuilder", "--platform", "linux/amd64,linux/arm64",
-          "--tag", "test.example.org/testuser/sample-test-image:latest",
-          expectedDockerStateDir.resolve("build").toFile().getAbsolutePath(), "--push"));
+      verifyNoDockerConfigAddedToBuildX();
     }
   }
 
   @Test
-  @DisplayName("docker:push buildx, auth from ~/.m2/settings.xml, then add --config option to buildx")
-  void execute_whenAuthConfigFromMavenSettings_thenAddConfigToDockerBuildXCommand() throws MojoExecutionException, MojoFailureException {
+  @DisplayName("23.0.6+azure-2 docker CLI, docker:push buildx, auth from ~/.docker/config.json, then don't add --config option to buildx")
+  void execute_whenAuthConfigFromLocalDockerConfig_thenAddConfigToDockerBuildXCommand() throws MojoExecutionException, MojoFailureException {
+    try (MockedStatic<DockerFileUtil> dockerFileUtilMockedStatic = mockStatic(DockerFileUtil.class)) {
+      // Given
+      givenDockerCLIVersion("'23.0.6+azure-2'");
+      AuthConfig authConfig = new AuthConfig("testuser", "testpassword", null, null, null);
+      authConfig.setRegistry("test.example.org");
+      dockerFileUtilMockedStatic.when(DockerFileUtil::readDockerConfig)
+          .thenReturn(authConfig.toJsonObject());
+
+      // When
+      pushMojo.execute();
+
+      // Then
+      verifyDockerConfigOptionAddedToBuildX();
+    }
+  }
+
+  @ParameterizedTest(name = "docker CLI {0} docker:push buildx, auth from ~/.m2/settings.xml, then add --config option to buildx")
+  @ValueSource(strings = {"20.0.14", "'23.0.6+azure-2'"})
+  void execute_whenAuthConfigFromMavenSettings_thenAddConfigToDockerBuildXCommand(String dockerClIVersion) throws MojoExecutionException, MojoFailureException {
     // Given
+    givenDockerCLIVersion(dockerClIVersion);
     Server server = new Server();
     server.setId("test.example.org");
     server.setUsername("testuser");
@@ -159,42 +189,51 @@ void execute_whenAuthConfigFromMavenSettings_thenAddConfigToDockerBuildXCommand(
     pushMojo.execute();
 
     // Then
-    assertEquals(1, defaultExecMockedConstruction.constructed().size());
-    BuildXService.DefaultExec defaultExec = defaultExecMockedConstruction.constructed().get(0);
-    verify(defaultExec).process(Arrays.asList("docker", "--config", expectedDockerStateConfigDir.getAbsolutePath(), "buildx", "create",
-        "--driver", "docker-container", "--name", "testbuilder", "--node", "testnode"));
-    verify(defaultExec).process(Arrays.asList("docker", "--config", expectedDockerStateConfigDir.getAbsolutePath(), "buildx", "build",
-        "--progress=plain", "--builder", "testbuilder", "--platform", "linux/amd64,linux/arm64",
-        "--tag", "test.example.org/testuser/sample-test-image:latest",
-        expectedDockerStateDir.resolve("build").toFile().getAbsolutePath(), "--push"));
+    verifyDockerConfigOptionAddedToBuildX();
   }
 
-  @Test
-  @DisplayName("docker:push buildx, auth from properties, then add --config option to buildx")
-  void execute_whenAuthConfigFromProperties_thenAddConfigOptionToBuildXCommand() throws MojoExecutionException, MojoFailureException {
+  @ParameterizedTest(name = "docker CLI {0} docker:push buildx, auth from properties, then add --config option to buildx")
+  @ValueSource(strings = {"20.0.14", "'23.0.6+azure-2'"})
+  void execute_whenAuthConfigFromProperties_thenAddConfigOptionToBuildXCommand(String dockerClIVersion) throws MojoExecutionException, MojoFailureException {
     try {
       // Given
+      givenDockerCLIVersion(dockerClIVersion);
       System.setProperty("docker.push.username", "testuser");
       System.setProperty("docker.push.password", "testpassword");
 
       // When
       pushMojo.execute();
 
       // Then
-      assertEquals(1, defaultExecMockedConstruction.constructed().size());
-      BuildXService.DefaultExec defaultExec = defaultExecMockedConstruction.constructed().get(0);
-      verify(defaultExec).process(Arrays.asList("docker", "--config", expectedDockerStateConfigDir.getAbsolutePath(), "buildx", "create",
-          "--driver", "docker-container", "--name", "testbuilder", "--node", "testnode"));
-      verify(defaultExec).process(Arrays.asList("docker", "--config", expectedDockerStateConfigDir.getAbsolutePath(), "buildx", "build",
-          "--progress=plain", "--builder", "testbuilder", "--platform", "linux/amd64,linux/arm64",
-          "--tag", "test.example.org/testuser/sample-test-image:latest",
-          expectedDockerStateDir.resolve("build").toFile().getAbsolutePath(), "--push"));
+      verifyDockerConfigOptionAddedToBuildX();
     } finally {
       System.clearProperty("docker.push.username");
       System.clearProperty("docker.push.password");
     }
   }
 
+  private void verifyNoDockerConfigAddedToBuildX() throws MojoExecutionException {
+    assertEquals(1, defaultExecMockedConstruction.constructed().size());
+    BuildXService.DefaultExec defaultExec = defaultExecMockedConstruction.constructed().get(0);
+    verify(defaultExec).process(Arrays.asList("docker", "buildx", "create",
+        "--driver", "docker-container", "--name", "testbuilder", "--node", "testnode"));
+    verify(defaultExec).process(Arrays.asList("docker", "buildx", "build",
+        "--progress=plain", "--builder", "testbuilder", "--platform", "linux/amd64,linux/arm64",
+        "--tag", "test.example.org/testuser/sample-test-image:latest",
+        expectedDockerStateDir.resolve("build").toFile().getAbsolutePath(), "--push"));
+  }
+
+  private void verifyDockerConfigOptionAddedToBuildX() throws MojoExecutionException {
+    assertEquals(1, defaultExecMockedConstruction.constructed().size());
+    BuildXService.DefaultExec defaultExec = defaultExecMockedConstruction.constructed().get(0);
+    verify(defaultExec).process(Arrays.asList("docker", "--config", expectedDockerStateConfigDir.getAbsolutePath(), "buildx", "create",
+        "--driver", "docker-container", "--name", "testbuilder", "--node", "testnode"));
+    verify(defaultExec).process(Arrays.asList("docker", "--config", expectedDockerStateConfigDir.getAbsolutePath(), "buildx", "build",
+        "--progress=plain", "--builder", "testbuilder", "--platform", "linux/amd64,linux/arm64",
+        "--tag", "test.example.org/testuser/sample-test-image:latest",
+        expectedDockerStateDir.resolve("build").toFile().getAbsolutePath(), "--push"));
+  }
+
   private ImageConfiguration createImageConfiguration() {
     return new ImageConfiguration.Builder()
         .name("test.example.org/testuser/sample-test-image:latest")
@@ -208,4 +247,8 @@ private ImageConfiguration createImageConfiguration() {
             .build())
         .build();
   }
+
+  private void givenDockerCLIVersion(String version) {
+    dockerClIVersion = version;
+  }
 }