[STORY-3093] feat(dbng): add firewall rules commands by briceamen · Pull Request #1171 · Scalingo/cli

briceamen · 2025-12-31T10:52:09Z

Summary

Add firewall rules management commands for DBNG databases:

database-firewall-rules - List firewall rules
database-firewall-rules-add - Add custom CIDR or managed range rule
database-firewall-rules-remove - Remove a rule
database-firewall-managed-ranges - List available managed ranges

Commands support both positional arguments and --database flag, matching existing DBNG command patterns (database-info, database-destroy).

Usage examples

# Positional argument style
scalingo database-firewall-rules my-db-id
scalingo database-firewall-rules-add my-db-id --cidr 203.0.113.0/24 --label "Office"
scalingo database-firewall-rules-remove my-db-id rule-id

# --database flag style
scalingo --database my-db database-firewall-rules
scalingo --database my-db database-firewall-rules-add --managed-range man-osc-fr1-egress
scalingo --database my-db database-firewall-rules-remove rule-id

Changes

Added GetAddonIDFromDatabase helper in detect/app.go to resolve addon ID from database ID
Firewall commands now support positional args like other DBNG commands

Test plan

Tested on staging (osc-st-fr1) with database test-firewall-rules-db:

Positional argument style:

database-firewall-rules test-firewall-rules-db - List rules
database-firewall-managed-ranges test-firewall-rules-db - List managed ranges
database-firewall-rules-add test-firewall-rules-db --cidr 203.0.113.0/24 --label "Test office" - Add custom rule
database-firewall-rules-remove test-firewall-rules-db <rule-id> - Remove rule

--database flag style:

--database test-firewall-rules-db database-firewall-rules - List rules
--database test-firewall-rules-db database-firewall-rules-add --managed-range man-osc-st-fr1-egress - Add managed range
--database test-firewall-rules-db database-firewall-rules-remove <rule-id> - Remove rule

notion-workspace · 2025-12-31T10:52:16Z

🗒️ [FEAT] Implement Firewall Rules on CLI

…mands

sc-david-voisin

Looks good to me, however I have few questions/suggestions

sc-david-voisin · 2025-12-31T14:38:37Z

+	}
+
+	io.Statusf("Firewall rule '%s' has been added to database '%s'.\n", rule.ID, databaseID)
+	io.Warning("Firewall rules take time to be applied due to infrastructure provisioning.")


Maybe reformulate a bit. A proposition:

"Expect some delay for the firewall rules to be applied, due to infrastructure provisioning."

matthieu526-scalingo · 2026-01-02T10:49:46Z

+				return cli.ShowCommandHelp(ctx, c, "database-firewall-rules")
+			}
+
+			utils.CheckForConsent(ctx, databaseID, utils.ConsentTypeDBs)


question: is the consent required here?

IMO yes. But why not?

Because I am not sure how the consent should be used in that case. This is for a database setting, not accessing the data?

matthieu526-scalingo · 2026-01-02T10:56:40Z

+	}
+
+	io.Statusf("Firewall rule '%s' has been removed from database '%s'.\n", ruleID, databaseID)
+	io.Warning("Expect some delay for the firewall rules to be applied, due to infrastructure provisioning.")


nitpick: do we want to use applied here?

@briceamen up here. Is applied applicable for removal?

curzolapierre

Some remarks in addition of @matthieu526-scalingo's ones

Overall, LGTM

curzolapierre · 2026-01-05T14:05:10Z

+				return cli.ShowCommandHelp(ctx, c, "database-firewall-rules")
+			}
+
+			utils.CheckForConsent(ctx, databaseID, utils.ConsentTypeDBs)


IMO yes. But why not?

curzolapierre · 2026-01-05T15:08:06Z

+			var databaseID, addonID string
+			var err error
+
+			switch c.Args().Len() {
+			case 0:
+				// No positional arg, database from --database flag
+				databaseID, addonID = detect.GetCurrentDatabase(ctx, c)
+			case 1:
+				// database-id provided as positional arg
+				databaseID = c.Args().First()
+				addonID, err = detect.GetAddonIDFromDatabase(ctx, databaseID)
+				if err != nil {


Semantically we used database "name" instead of ID. Could you apply the change elsewhere in this file?

Or do I miss something?

Suggested change

var databaseID, addonID string

var err error

switch c.Args().Len() {

case 0:

// No positional arg, database from --database flag

databaseID, addonID = detect.GetCurrentDatabase(ctx, c)

case 1:

// database-id provided as positional arg

databaseID = c.Args().First()

addonID, err = detect.GetAddonIDFromDatabase(ctx, databaseID)

if err != nil {

var databaseName, addonID string

var err error

switch c.Args().Len() {

case 0:

// No positional arg, database from --database flag

databaseName, addonID = detect.GetCurrentDatabase(ctx, c)

case 1:

// database-name provided as positional arg

databaseName = c.Args().First()

addonID, err = detect.GetAddonIDFromDatabase(ctx, databaseName)

if err != nil {

curzolapierre · 2026-01-05T15:10:10Z

+	// A DBNG database is modeled as an app with a single addon (itself),
+	// whereas an application can have multiple addons (postgresql, redis, etc.).
+	// If multiple addons are returned, the ID is likely an application, not a database.
+	addons, err := client.AddonsList(ctx, databaseID)


Like before, this should be the databaseName

Suggested change

addons, err := client.AddonsList(ctx, databaseID)

addons, err := client.AddonsList(ctx, databaseName)

…omArgs helper

curzolapierre

I've remaining questions

curzolapierre · 2026-01-06T15:09:15Z

+			if err != nil {
+				if err == detect.ErrTooManyArguments {
+					io.Error(err)
+					return cli.ShowCommandHelp(ctx, c, "database-firewall-rules")
+				}
+				errorQuit(ctx, err)
+			}
+


Suggestion: Could be simplified by:

Suggested change

if err != nil {

if err == detect.ErrTooManyArguments {

io.Error(err)

return cli.ShowCommandHelp(ctx, c, "database-firewall-rules")

}

errorQuit(ctx, err)

}

if errors.Is(err, detect.ErrTooManyArguments) {

io.Error(err)

return cli.ShowCommandHelp(ctx, c, "database-firewall-rules")

}

if err != nil {

errorQuit(ctx, err)

}

In addition, this is a good practice to use errors.Is instead of direct comparison.

Much cleaner indeed. Thanks.

curzolapierre · 2026-01-06T15:16:08Z

+			if managedRange != "" && label != "" {
+				io.Warning("--label flag is ignored for managed ranges")
+			}


Question: are you sure we cannot add label to managed ranges?

Oups you're right FirewallRuleCreateParams has Label as a general field (not specific to custom ranges), and FirewallRule response also has Label for all types. Will change

curzolapierre · 2026-01-06T15:16:31Z

+			if err != nil {
+				if err == detect.ErrTooManyArguments {
+					io.Error(err)
+					return cli.ShowCommandHelp(ctx, c, "database-firewall-managed-ranges")
 				}
-			default:
-				io.Error("Too many arguments")
-				return cli.ShowCommandHelp(ctx, c, "database-firewall-managed-ranges")
+				errorQuit(ctx, err)
 			}


Same as above

curzolapierre · 2026-01-06T15:16:40Z

+			if err != nil {
+				if err == detect.ErrTooManyArguments {
+					io.Error(err)
+					return cli.ShowCommandHelp(ctx, c, "database-firewall-rules-add")
 				}
-			default:
-				io.Error("Too many arguments")
-				return cli.ShowCommandHelp(ctx, c, "database-firewall-rules-add")
+				errorQuit(ctx, err)


Same as above

curzolapierre

Thanks! LGTM

feat(dbng): add firewall rules commands for DBNG databases

b4cadcb

briceamen requested a review from Copilot December 31, 2025 10:53

This comment was marked as outdated.

Sign in to view

refactor: improve column header clarity in firewall rules list

9efbce0

briceamen self-assigned this Dec 31, 2025

feat(dbng): add positional arg support for firewall rules commands

c7a5475

briceamen requested a review from Copilot December 31, 2025 13:26

This comment was marked as outdated.

Sign in to view

fix: add validation for multiple addons in GetAddonIDFromDatabase

27dc5a3

briceamen marked this pull request as ready for review December 31, 2025 13:28

refactor: use switch statements for argument handling in firewall com…

ea3aaaa

…mands

briceamen force-pushed the feat/STORY-3093/firewall-rules branch from 10d0d9a to ea3aaaa Compare December 31, 2025 13:39

briceamen requested a review from Copilot December 31, 2025 13:40

This comment was marked as outdated.

Sign in to view

fix: improve error messages for remove command argument validation

66c53d4

briceamen requested review from curzolapierre, matthieu526-scalingo and sc-david-voisin December 31, 2025 14:02

sc-david-voisin reviewed Dec 31, 2025

View reviewed changes

briceamen added 2 commits December 31, 2025 15:50

docs: clarify addon list comment for DBNG databases

872f60e

fix: improve warning message wording for firewall rules

cf2bad7

briceamen requested a review from sc-david-voisin December 31, 2025 14:56

sc-david-voisin approved these changes Dec 31, 2025

View reviewed changes

Comment thread dbng/firewall_rules.go

Comment thread detect/app.go Outdated

matthieu526-scalingo reviewed Jan 2, 2026

View reviewed changes

curzolapierre reviewed Jan 5, 2026

View reviewed changes

briceamen added 3 commits January 6, 2026 14:23

refactor: rename databaseID to databaseName and extract GetDatabaseFr…

63fd066

…omArgs helper

refactor: use GetDatabaseFromArgs helper in firewall rules commands

f3301ab

fix: improve warning message wording for firewall rules

6ca3e6e

briceamen requested a review from curzolapierre January 6, 2026 13:35

briceamen requested a review from matthieu526-scalingo January 6, 2026 13:35

curzolapierre reviewed Jan 6, 2026

View reviewed changes

refactor: use errors.Is and support labels for managed ranges

e7e85e0

briceamen requested a review from curzolapierre January 6, 2026 16:23

curzolapierre approved these changes Jan 7, 2026

View reviewed changes

briceamen merged commit 4f3135a into master Jan 7, 2026
7 checks passed

briceamen deleted the feat/STORY-3093/firewall-rules branch January 7, 2026 10:24

	addons, err := client.AddonsList(ctx, databaseID)
	addons, err := client.AddonsList(ctx, databaseName)

Conversation

briceamen commented Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Usage examples

Changes

Test plan

Uh oh!

notion-workspace Bot commented Dec 31, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

sc-david-voisin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curzolapierre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curzolapierre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

curzolapierre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

briceamen commented Dec 31, 2025 •

edited

Loading