Skip to content

Add Info.FirewallBackend#49761

Merged
vvoland merged 1 commit intomoby:masterfrom
robmry:add_info_firewallbackend
Apr 7, 2025
Merged

Add Info.FirewallBackend#49761
vvoland merged 1 commit intomoby:masterfrom
robmry:add_info_firewallbackend

Conversation

@robmry
Copy link
Contributor

@robmry robmry commented Apr 7, 2025

- What I did

Report FirewallBackend in "docker info".

It's currently "iptables" or "iptables+firewalld" on Linux, and omitted on Windows.

- How I did it

- How to verify it

New integration test.

- Human readable description for the release notes

`GET /info` now returns a `FirewallBackend` containing information about the daemon's firewalling configuration.

@robmry robmry added area/api API kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny impact/changelog area/networking/firewalling Networking labels Apr 7, 2025
@robmry robmry added this to the 28.1.0 milestone Apr 7, 2025
@robmry robmry self-assigned this Apr 7, 2025
@robmry robmry requested a review from thaJeztah April 7, 2025 09:10
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Some minor nits, but LGTM otherwise

Comment on lines +6953 to +6956
type: "string"
example: "nftables"
Copy link
Member

Choose a reason for hiding this comment

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

Not a real blocker, but we could add an enum to this to indicate values it can return; I guess nftables is not yet possible with this PR, correct?

Suggested change
type: "string"
example: "nftables"
type: "string"
enum: ["iptables", "iptables+firewalld"]
example: "iptables"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change has a life of its own, but it was only intended to be something we can see in docker info in bug reports (once added to the CLI), and check in our integration tests because the firewalld tests were running without firewalld enabled for some months.

I think it's best not to try to lock it down or treat new values as API changes, or it'll just get out of step or force us into otherwise-unnecessary API version bumps?

You suggested aligning earlier incarnations of the change with reporting of storage drivers - can we stick with that?

moby/api/swagger.yaml

Lines 6451 to 6454 in ecb03c4

Driver:
description: "Name of the storage driver in use."
type: "string"
example: "overlay2"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(And, right - no nftables yet. But, soon, it should become the common case.)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense. Mostly was wondering if it would help describing what to expect (not 100% sure if we must consider adding new options to the enum to require an API version bump, but good call)

api/swagger.yaml Outdated
items:
type: "string"
example:
- ["ReloadedAt", "2025-01-01 00:00:00"]
Copy link
Member

Choose a reason for hiding this comment

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

Per the other PR; if we decide to use RFC3339 (or ..Nano), we could update the example to match that format;

Suggested change
- ["ReloadedAt", "2025-01-01 00:00:00"]
- ["ReloadedAt", "2025-01-01T00:00:00Z"]
Suggested change
- ["ReloadedAt", "2025-01-01 00:00:00"]
- ["ReloadedAt", "2025-01-01T00:00:00.000000001Z"]

The Z depends on whether we'll use UTC, otherwise it may show something like +02:00 or Z (depending on the hosts's config)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@robmry robmry force-pushed the add_info_firewallbackend branch from 3c6045a to b2eea03 Compare April 7, 2025 10:21
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@robmry robmry force-pushed the add_info_firewallbackend branch from b2eea03 to effcffd Compare April 7, 2025 10:33
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@robmry robmry requested a review from vvoland April 7, 2025 10:53
@thaJeztah
Copy link
Member

A, derp; needs a rebase for the version-history.md - we can merge once that's done (probably don't need a full cycle of CI after that)

@robmry robmry force-pushed the add_info_firewallbackend branch from effcffd to 1c1fae4 Compare April 7, 2025 15:11
Report FirewallBackend in "docker info".

It's currently "iptables" or "iptables+firewalld" on Linux, and
omitted on Windows.

Signed-off-by: Rob Murray <rob.murray@docker.com>
@robmry robmry force-pushed the add_info_firewallbackend branch from 1c1fae4 to a0a86d0 Compare April 7, 2025 15:57
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

still LGTM

@vvoland if you could give this one a quick peek

Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM

@vvoland vvoland merged commit ebc6c06 into moby:master Apr 7, 2025
154 checks passed
@robmry robmry deleted the add_info_firewallbackend branch April 7, 2025 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api API area/networking/firewalling Networking impact/api impact/changelog kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants