fix(kafka): strip architecture suffix from Kafka image tags for semver parsing #3276
Conversation
Newer Kafka Docker images include architecture suffixes in their tags (e.g. 7.5.0.arm64), which are not valid semver. This change normalises the tag by removing the architecture suffix before passing it to the semver parser, ensuring compatibility with golang.org/x/mod/semver.
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
asahasrabuddhe
changed the title
asahasrabuddhe
changed the title
asahasrabuddhe
changed the title
asahasrabuddhe
changed the title
asahasrabuddhe
changed the title
There was a problem hiding this comment.
LGTM, thanks!
My only question would be if we should notify the Kafka folks about releasing multi-arch images to avoid breaking semver 🤔
@eddumelendez @kiview wdyt?
If not multi-arch images, they should try to fix their tagging to be semver compatible. |
|
|
||
| // remove the architecture suffix | ||
| if strings.HasSuffix(version, ".amd64") || strings.HasSuffix(version, ".arm64") { | ||
| version = version[:strings.LastIndex(version, ".")] |
There was a problem hiding this comment.
@mdelapenya How about something like this? But at this point, we might as well let them use the image by fixing the version.
| version = version[:strings.LastIndex(version, ".")] | |
| return fmt.Errorf("invalid image tag %q: architecture suffixes like .arm64 or .amd64 are not valid semver; please use a multi-architecture image instead", tag) |
There was a problem hiding this comment.
Given we are using semver libraries to get a valid version, then we should honor it. I think this is a good compromise, failing if the version is not following semver.
There was a problem hiding this comment.
@asahasrabuddhe let's apply that fix, so we can merge this PR
Thanks!
Summary by CodeRabbit
WalkthroughAdds architecture suffix stripping (".amd64"/".arm64") in validateKRaftVersion before semver comparison with "v7.4.0". Updates tests to include Confluent image tags with architecture suffixes for both amd64 and arm64, expecting no errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant V as validateKRaftVersion
C->>V: validateKRaftVersion(tag)
activate V
V->>V: Ensure "v" prefix
V->>V: If tag ends with .amd64/.arm64<br/>truncate at last "."
V->>V: Compare semver against v7.4.0
V-->>C: Return nil or error (unchanged criteria)
deactivate V
rect rgba(200,230,255,0.3)
note right of V: New step: strip architecture suffix<br/>before semver comparison
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/kafka/kafka.go (1)
222-225: Consider usingstrings.TrimSuffixfor clarity.The current implementation using
strings.LastIndexworks correctly, butstrings.TrimSuffixwould more explicitly convey the intent of removing a known suffix.Apply this diff for a more idiomatic approach:
- // remove the architecture suffix - if strings.HasSuffix(version, ".amd64") || strings.HasSuffix(version, ".arm64") { - version = version[:strings.LastIndex(version, ".")] - } + // remove the architecture suffix + version = strings.TrimSuffix(version, ".amd64") + version = strings.TrimSuffix(version, ".arm64")This approach:
- Eliminates the conditional check (TrimSuffix is a no-op if suffix not present)
- Makes the intent more explicit
- Handles both suffixes sequentially (though only one would be present in practice)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/kafka/kafka.go(1 hunks)modules/kafka/kafka_helpers_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Main pipeline
modules/kafka/kafka_helpers_test.go
[error] 103-103: golangci-lint: File is not properly formatted (gci)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
modules/kafka/kafka_helpers_test.go (1)
97-101: LGTM!The test case correctly validates that Kafka images with
.amd64architecture suffix are now supported after the version normalization invalidateKRaftVersion.
| { | ||
| name: "Official: valid, with the arm64 architecture suffix", | ||
| image: "confluentinc/confluent-local:7.5.9.arm64", | ||
| }, |
There was a problem hiding this comment.
Add missing wantErr field and fix formatting.
The test case is incomplete - it's missing the wantErr: false field. Additionally, there's a formatting issue flagged by the linter.
Apply this diff to fix both issues:
{
name: "Official: valid, with the arm64 architecture suffix",
image: "confluentinc/confluent-local:7.5.9.arm64",
+ wantErr: false,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| name: "Official: valid, with the arm64 architecture suffix", | |
| image: "confluentinc/confluent-local:7.5.9.arm64", | |
| }, | |
| { | |
| name: "Official: valid, with the arm64 architecture suffix", | |
| image: "confluentinc/confluent-local:7.5.9.arm64", | |
| wantErr: false, | |
| }, |
🧰 Tools
🪛 GitHub Actions: Main pipeline
[error] 103-103: golangci-lint: File is not properly formatted (gci)
🤖 Prompt for AI Agents
In modules/kafka/kafka_helpers_test.go around lines 102 to 105, the table-driven
test entry for "Official: valid, with the arm64 architecture suffix" is missing
the wantErr field and has a linter-formatting issue; update that test case to
include wantErr: false and adjust punctuation/indentation to match surrounding
cases (ensure fields are comma-separated, aligned, and the closing brace/comma
are correctly placed).
2 participants
What does this PR do?
This PR strips the architecture suffix (e.g. .arm64) from Kafka image tags.
Why is it important?
Newer Kafka Docker images have started including architecture suffixes in
their tags (e.g.
7.5.9-1-ubi8.arm64,7.5.9.arm64). These tags areproblematic because the
.arm64suffix is not valid according to theSemantic Versioning specification.
7.5.9-1-ubi8.arm64is semver-compatible because everything before.arm64(7.5.9-1-ubi8) is already a valid semver string with apre-release identifier (
-1-ubi8). The.arm64suffix, while not validsemver itself, is layered on top of an otherwise valid version.
7.5.9.arm64, on the other hand, is not semver-compatible becausethe
.arm64directly follows the patch version, making the whole stringinvalid under semver rules.
This change ensures compatibility with
golang.org/x/mod/semverbynormalising the tag — removing the trailing architecture suffix — before
parsing. That way, we consistently fall back to a valid semver core
(e.g.
7.5.9or7.5.9-1-ubi8), while still supporting images with suffixes.Related issues
Closes #2890
How to test this PR
We can try to use a newer Kafka image with a tag that contains the architecture suffix (e.g.
confluentinc/confluent-local:7.5.0.arm64orconfluentinc/confluent-local:7.5.9.arm64). We should no longer see theKRaft mode is only available since version 7.4.0error message.