fix(standalone): skip bridge gateway binding for rootless Docker#311
fix(standalone): skip bridge gateway binding for rootless Docker#311doringeman merged 1 commit intodocker:mainfrom
Conversation
Summary of ChangesHello @doringeman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request provides a fix for an issue related to Docker container port bindings, specifically when Docker is running in rootless mode. It introduces a new detection mechanism to identify rootless Docker environments and then uses this information to prevent the incorrect application of bridge gateway IP bindings. This change aims to enhance the stability and correctness of container deployments, ensuring that standalone containers are configured appropriately across different Docker operational modes. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider using the Info.Rootless boolean from the Docker API instead of scanning SecurityOptions with strings.Contains for more reliable detection.
- Defaulting to “not rootless” on Info errors may lead to unintended bridge bindings; consider returning the error or at least logging it when detection fails.
- Cache the result of the rootless check rather than calling dockerClient.Info on every container creation to avoid needless performance overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using the Info.Rootless boolean from the Docker API instead of scanning SecurityOptions with strings.Contains for more reliable detection.
- Defaulting to “not rootless” on Info errors may lead to unintended bridge bindings; consider returning the error or at least logging it when detection fails.
- Cache the result of the rootless check rather than calling dockerClient.Info on every container creation to avoid needless performance overhead.
## Individual Comments
### Comment 1
<location> `cmd/cli/pkg/standalone/containers.go:222` </location>
<code_context>
+// isNotRootless detects if Docker is NOT running in rootless mode.
+func isNotRootless(ctx context.Context, dockerClient *client.Client) bool {
+ info, err := dockerClient.Info(ctx)
+ if err != nil {
+ // Default to assuming not rootless if we can't determine.
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling Info API errors more explicitly.
Logging or propagating the error would help ensure issues like connectivity or permission failures are visible, rather than silently ignored.
Suggested implementation:
```golang
if err != nil {
// Log the error to ensure visibility of connectivity or permission failures.
log.Printf("Error retrieving Docker info: %v", err)
// Default to assuming not rootless if we can't determine.
return true
}
```
If the `log` package is not already imported in this file, you should add:
```go
import "log"
```
at the top of the file with the other imports.
</issue_to_address>
### Comment 2
<location> `cmd/cli/pkg/standalone/containers.go:227-231` </location>
<code_context>
+ // Default to assuming not rootless if we can't determine.
+ return true
+ }
+ for _, opt := range info.SecurityOptions {
+ if strings.Contains(opt, "rootless") {
+ return false
</code_context>
<issue_to_address>
**suggestion:** Check for case sensitivity in 'rootless' detection.
Consider using strings.EqualFold or converting both strings to lower case to make the 'rootless' check case-insensitive.
```suggestion
for _, opt := range info.SecurityOptions {
if strings.Contains(strings.ToLower(opt), "rootless") {
return false
}
}
```
</issue_to_address>
### Comment 3
<location> `cmd/cli/pkg/standalone/containers.go:282-283` </location>
<code_context>
- // Only add bridge gateway IP binding if host is 127.0.0.1
- if host == "127.0.0.1" {
+ // Only add bridge gateway IP binding if host is 127.0.0.1 and not in rootless mode
+ if host == "127.0.0.1" && isNotRootless(ctx, dockerClient) {
if bridgeGatewayIP, err := determineBridgeGatewayIP(ctx, dockerClient); err == nil && bridgeGatewayIP != "" {
portBindings = append(portBindings, nat.PortBinding{HostIP: bridgeGatewayIP, HostPort: portStr})
</code_context>
<issue_to_address>
**suggestion:** Consider documenting the rationale for skipping bridge IP binding in rootless mode.
If this change is based on a specific Docker limitation or bug, please add a reference to relevant documentation or issues for clarity.
```suggestion
// Only add bridge gateway IP binding if host is 127.0.0.1 and not in rootless mode.
// In Docker rootless mode, binding to the bridge gateway IP is not supported due to user namespace and network restrictions.
// See: https://docs.docker.com/engine/security/rootless/#networking and https://github.com/docker/for-linux/issues/101
if host == "127.0.0.1" && isNotRootless(ctx, dockerClient) {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for _, opt := range info.SecurityOptions { | ||
| if strings.Contains(opt, "rootless") { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: Check for case sensitivity in 'rootless' detection.
Consider using strings.EqualFold or converting both strings to lower case to make the 'rootless' check case-insensitive.
| for _, opt := range info.SecurityOptions { | |
| if strings.Contains(opt, "rootless") { | |
| return false | |
| } | |
| } | |
| for _, opt := range info.SecurityOptions { | |
| if strings.Contains(strings.ToLower(opt), "rootless") { | |
| return false | |
| } | |
| } |
cmd/cli/pkg/standalone/containers.go
Outdated
| // Only add bridge gateway IP binding if host is 127.0.0.1 and not in rootless mode | ||
| if host == "127.0.0.1" && isNotRootless(ctx, dockerClient) { |
There was a problem hiding this comment.
suggestion: Consider documenting the rationale for skipping bridge IP binding in rootless mode.
If this change is based on a specific Docker limitation or bug, please add a reference to relevant documentation or issues for clarity.
| // Only add bridge gateway IP binding if host is 127.0.0.1 and not in rootless mode | |
| if host == "127.0.0.1" && isNotRootless(ctx, dockerClient) { | |
| // Only add bridge gateway IP binding if host is 127.0.0.1 and not in rootless mode. | |
| // In Docker rootless mode, binding to the bridge gateway IP is not supported due to user namespace and network restrictions. | |
| // See: https://docs.docker.com/engine/security/rootless/#networking and https://github.com/docker/for-linux/issues/101 | |
| if host == "127.0.0.1" && isNotRootless(ctx, dockerClient) { |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with bridge gateway binding in rootless Docker environments by adding a check for rootless mode before attempting the binding. The overall approach is sound. My review includes suggestions to improve code readability and adhere to common Go naming conventions for the new helper function.
cmd/cli/pkg/standalone/containers.go
Outdated
| // isNotRootless detects if Docker is NOT running in rootless mode. | ||
| func isNotRootless(ctx context.Context, dockerClient *client.Client) bool { | ||
| info, err := dockerClient.Info(ctx) | ||
| if err != nil { | ||
| // Default to assuming not rootless if we can't determine. | ||
| return true | ||
| } | ||
| for _, opt := range info.SecurityOptions { | ||
| if strings.Contains(opt, "rootless") { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
For better readability and to follow common Go naming conventions, consider renaming this function to isRootless and inverting its return logic. Functions that return a boolean are often named with a positive sense (e.g., isSomething, not isNotSomething). This makes the function's purpose clearer. I'll suggest the corresponding change at the call site in a separate comment.
| // isNotRootless detects if Docker is NOT running in rootless mode. | |
| func isNotRootless(ctx context.Context, dockerClient *client.Client) bool { | |
| info, err := dockerClient.Info(ctx) | |
| if err != nil { | |
| // Default to assuming not rootless if we can't determine. | |
| return true | |
| } | |
| for _, opt := range info.SecurityOptions { | |
| if strings.Contains(opt, "rootless") { | |
| return false | |
| } | |
| } | |
| return true | |
| } | |
| // isRootless detects if Docker is running in rootless mode. | |
| func isRootless(ctx context.Context, dockerClient *client.Client) bool { | |
| info, err := dockerClient.Info(ctx) | |
| if err != nil { | |
| // If we can't get Docker info, assume it's not rootless to preserve old behavior. | |
| return false | |
| } | |
| for _, opt := range info.SecurityOptions { | |
| if strings.Contains(opt, "rootless") { | |
| return true | |
| } | |
| } | |
| return false | |
| } |
cmd/cli/pkg/standalone/containers.go
Outdated
| // Only add bridge gateway IP binding if host is 127.0.0.1 | ||
| if host == "127.0.0.1" { | ||
| // Only add bridge gateway IP binding if host is 127.0.0.1 and not in rootless mode | ||
| if host == "127.0.0.1" && isNotRootless(ctx, dockerClient) { |
There was a problem hiding this comment.
To accompany the suggested rename of isNotRootless to isRootless, this call should be updated to use negation, which improves clarity.
| if host == "127.0.0.1" && isNotRootless(ctx, dockerClient) { | |
| if host == "127.0.0.1" && !isRootless(ctx, dockerClient) { |
There was a problem hiding this comment.
If it was me I'd actually make it an:
isRootfull
function to avoid the double negative. But it's fine 😄
There was a problem hiding this comment.
I thought about that but I didn't like the fact that I was still checking for "rootless". 😅
Signed-off-by: Dorin Geman <dorin.geman@docker.com>
doringeman
force-pushed
the
docker-rootless
branch
from
356f095 to
d497a24
Compare
3 participants
Fixes #306.