Copilot commented on code in PR #3964:
URL: https://github.com/apache/solr/pull/3964#discussion_r2628917383
##########
solr/docker/build.gradle:
##########
@@ -234,19 +294,64 @@ task dockerPush(dependsOn: tasks.dockerTag) {
// Ensure that the docker image is re-pushed if the image ID or tag changes
inputs.properties([
dockerImageName: dockerImageName,
+ dockerImagePlatforms: dockerImagePlatforms
])
inputs.file(imageIdFile)
// We don't want to push a docker image unless the tests have passed
mustRunAfter tasks.testDocker
doLast {
- exec {
- commandLine "docker", "push", dockerImageName
- }
+ def platformValue = inputs.properties.dockerImagePlatforms
+
+ if (isMultiPlatform()) {
+ // Multi-platform push requires buildx and rebuilding with --push
+
+ // Get builder info and validate it exists
+ def builderInfo = getBuildxBuilderInfo()
+
+ // Verify the builder supports the requested platforms (fail fast)
+ def requestedPlatforms = platformValue.split(',').collect { it.trim() }
+ def missingPlatforms = []
+ requestedPlatforms.each { platform ->
+ // Check for exact platform match - use delimiters instead of word
boundaries
+ // Match platform at start/end or surrounded by commas/spaces
+ def platformPattern = "(?:^|[,\\s])" +
java.util.regex.Pattern.quote(platform) + "(?:[,\\s]|\$)"
+ if (!(builderInfo =~ platformPattern).find()) {
+ missingPlatforms.add(platform)
+ }
+ }
- // Print information on the image after it has been created
- project.logger.lifecycle("Solr Docker Image Pushed: \t$dockerImageName")
+ if (!missingPlatforms.isEmpty()) {
+ // Extract supported platforms from builder info using matcher
+ def supportedPlatforms = (builderInfo =~
/linux\/\w+(?:\/\w+)?/).collect().join(', ')
+ throw new GradleException("Current builder does not support all
requested platforms: ${missingPlatforms}\n" +
+ "Supported platforms: ${supportedPlatforms}\n" +
+ "To add platform support, create a builder with QEMU:\n" +
+ " docker run --privileged --rm tonistiigi/binfmt --install
all\n" +
+ " docker buildx create --name solr-builder --use --platform
${platformValue}")
+ }
+
+ def solrTgzConfiguration = isImageSlim() ? configurations.solrSlimTgz :
configurations.solrFullTgz
Review Comment:
The solr tarball configuration is retrieved again here, but it was already
computed at the task level (line 192). This duplicates logic and could lead to
inconsistencies if the configuration selection logic changes. Consider
accessing this value from the task inputs or storing it in a variable that can
be reused in both places.
```suggestion
```
##########
solr/docker/build.gradle:
##########
@@ -234,19 +294,64 @@ task dockerPush(dependsOn: tasks.dockerTag) {
// Ensure that the docker image is re-pushed if the image ID or tag changes
inputs.properties([
dockerImageName: dockerImageName,
+ dockerImagePlatforms: dockerImagePlatforms
])
inputs.file(imageIdFile)
// We don't want to push a docker image unless the tests have passed
mustRunAfter tasks.testDocker
doLast {
- exec {
- commandLine "docker", "push", dockerImageName
- }
+ def platformValue = inputs.properties.dockerImagePlatforms
+
+ if (isMultiPlatform()) {
+ // Multi-platform push requires buildx and rebuilding with --push
+
+ // Get builder info and validate it exists
+ def builderInfo = getBuildxBuilderInfo()
+
+ // Verify the builder supports the requested platforms (fail fast)
+ def requestedPlatforms = platformValue.split(',').collect { it.trim() }
+ def missingPlatforms = []
+ requestedPlatforms.each { platform ->
+ // Check for exact platform match - use delimiters instead of word
boundaries
+ // Match platform at start/end or surrounded by commas/spaces
+ def platformPattern = "(?:^|[,\\s])" +
java.util.regex.Pattern.quote(platform) + "(?:[,\\s]|\$)"
+ if (!(builderInfo =~ platformPattern).find()) {
+ missingPlatforms.add(platform)
+ }
+ }
- // Print information on the image after it has been created
- project.logger.lifecycle("Solr Docker Image Pushed: \t$dockerImageName")
+ if (!missingPlatforms.isEmpty()) {
+ // Extract supported platforms from builder info using matcher
+ def supportedPlatforms = (builderInfo =~
/linux\/\w+(?:\/\w+)?/).collect().join(', ')
+ throw new GradleException("Current builder does not support all
requested platforms: ${missingPlatforms}\n" +
+ "Supported platforms: ${supportedPlatforms}\n" +
+ "To add platform support, create a builder with QEMU:\n" +
+ " docker run --privileged --rm tonistiigi/binfmt --install
all\n" +
+ " docker buildx create --name solr-builder --use --platform
${platformValue}")
+ }
+
+ def solrTgzConfiguration = isImageSlim() ? configurations.solrSlimTgz :
configurations.solrFullTgz
+ exec {
+ standardInput = solrTgzConfiguration.singleFile.newDataInputStream()
+ commandLine "docker", "buildx", "build",
+ "-f", "solr-${ -> project.version
}${dockerImageDistSuffix}/docker/Dockerfile",
+ "--platform", platformValue,
Review Comment:
The platform values from user input are not validated before being passed to
shell commands. While the Pattern.quote() method is used for regex matching on
line 319, the raw platformValue is passed directly to the docker command on
line 340. A malicious or malformed platform string could potentially inject
additional command arguments. Consider validating that platform values match
the expected format (e.g., `linux/[a-z0-9]+(/[a-z0-9]+)?`) before use.
##########
solr/docker/build.gradle:
##########
@@ -134,6 +136,32 @@ def checksum = { file ->
return new DigestUtils(DigestUtils.sha512Digest).digestAsHex(file).trim()
}
+// Helper method to get Docker Buildx builder info, throws exception if not
available
+def getBuildxBuilderInfo = {
+ def builderCheckOutput = new ByteArrayOutputStream()
+ def builderCheckResult = exec {
+ ignoreExitValue = true
+ standardOutput = builderCheckOutput
+ errorOutput = new ByteArrayOutputStream()
+ commandLine "docker", "buildx", "inspect"
+ }
+
+ if (builderCheckResult.exitValue != 0) {
+ throw new GradleException("Docker Buildx is not available or no builder is
configured.\n" +
+ "Please ensure Docker Buildx is installed and create a builder:\n"
+
+ " docker buildx create --name solr-builder --use\n" +
+ "Or use an existing builder:\n" +
+ " docker buildx use <builder-name>")
+ }
+
+ return builderCheckOutput.toString()
+}
+
+// Helper method to validate Docker Buildx builder is available
Review Comment:
The `validateBuildxBuilder` function is a thin wrapper that just calls
`getBuildxBuilderInfo()` and discards the result. This adds an unnecessary
layer of indirection. Consider either removing this wrapper and calling
`getBuildxBuilderInfo()` directly where validation is needed, or if the wrapper
serves a semantic purpose, add a comment explaining why it exists separately
from `getBuildxBuilderInfo()`.
```suggestion
// Helper method to validate Docker Buildx builder is available.
// This is a semantic wrapper for use in Gradle tasks that only need to
assert
// the presence of a configured Buildx builder (and do not require its info).
```
##########
solr/docker/build.gradle:
##########
@@ -234,19 +294,64 @@ task dockerPush(dependsOn: tasks.dockerTag) {
// Ensure that the docker image is re-pushed if the image ID or tag changes
inputs.properties([
dockerImageName: dockerImageName,
+ dockerImagePlatforms: dockerImagePlatforms
])
inputs.file(imageIdFile)
// We don't want to push a docker image unless the tests have passed
mustRunAfter tasks.testDocker
doLast {
- exec {
- commandLine "docker", "push", dockerImageName
- }
+ def platformValue = inputs.properties.dockerImagePlatforms
+
+ if (isMultiPlatform()) {
+ // Multi-platform push requires buildx and rebuilding with --push
+
+ // Get builder info and validate it exists
+ def builderInfo = getBuildxBuilderInfo()
+
+ // Verify the builder supports the requested platforms (fail fast)
+ def requestedPlatforms = platformValue.split(',').collect { it.trim() }
+ def missingPlatforms = []
+ requestedPlatforms.each { platform ->
+ // Check for exact platform match - use delimiters instead of word
boundaries
+ // Match platform at start/end or surrounded by commas/spaces
+ def platformPattern = "(?:^|[,\\s])" +
java.util.regex.Pattern.quote(platform) + "(?:[,\\s]|\$)"
Review Comment:
The regex pattern uses `\w+` which only matches word characters (letters,
digits, underscore). However, Docker platform names can include hyphens (e.g.,
`linux/arm/v7`). The current pattern would fail to match platforms with hyphens
in their variants. Consider using a more permissive character class like
`[^,\s]+` or `[\w\-]+` to handle all valid platform identifiers.
##########
solr/docker/build.gradle:
##########
@@ -158,32 +186,64 @@ task dockerBuild() {
// Ensure that the docker image is rebuilt on build-arg changes or changes
in the docker context
inputs.properties([
- baseDockerImage: baseDockerImage
+ baseDockerImage: baseDockerImage,
+ dockerImagePlatforms: dockerImagePlatforms
])
var solrTgzConfiguration = isImageSlim() ? configurations.solrSlimTgz :
configurations.solrFullTgz
inputs.files(solrTgzConfiguration)
inputs.property("isSlimImage", isImageSlim())
+ inputs.property("isMultiPlatform", isMultiPlatform())
dependsOn(solrTgzConfiguration)
doLast {
- exec {
- standardInput = solrTgzConfiguration.singleFile.newDataInputStream()
- commandLine "docker", "build",
- "-f", "solr-${ -> project.version
}${dockerImageDistSuffix}/docker/Dockerfile",
- "--iidfile", imageIdFile,
- "--build-arg", "BASE_IMAGE=${ ->
inputs.properties.baseDockerImage}",
- "-"
+ def platformValue = inputs.properties.dockerImagePlatforms
Review Comment:
The local variable `platformValue` duplicates information already available
in `inputs.properties.dockerImagePlatforms`. Since this value is accessed
multiple times in the method, using the local variable improves readability,
but it creates inconsistency with the closure-based approach used elsewhere
(like line 244). Consider either consistently using the local variable
throughout the doLast block, or accessing it from inputs.properties
consistently to maintain uniform code style.
##########
solr/docker/build.gradle:
##########
@@ -234,19 +294,64 @@ task dockerPush(dependsOn: tasks.dockerTag) {
// Ensure that the docker image is re-pushed if the image ID or tag changes
inputs.properties([
dockerImageName: dockerImageName,
+ dockerImagePlatforms: dockerImagePlatforms
])
inputs.file(imageIdFile)
// We don't want to push a docker image unless the tests have passed
mustRunAfter tasks.testDocker
doLast {
- exec {
- commandLine "docker", "push", dockerImageName
- }
+ def platformValue = inputs.properties.dockerImagePlatforms
+
+ if (isMultiPlatform()) {
+ // Multi-platform push requires buildx and rebuilding with --push
+
+ // Get builder info and validate it exists
+ def builderInfo = getBuildxBuilderInfo()
+
+ // Verify the builder supports the requested platforms (fail fast)
+ def requestedPlatforms = platformValue.split(',').collect { it.trim() }
Review Comment:
The platform string is split and trimmed for validation, but the original
`platformValue` (which may contain spaces around commas) is passed to the
docker buildx command on line 340. While Docker likely tolerates extra
whitespace, it would be more robust to use the trimmed, normalized platform
list consistently. Consider storing the cleaned platform list and using it for
both validation and the command invocation.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
