add control flags to fdb remote networks by danovaro · Pull Request #267 · ecmwf/fdb

danovaro · 2026-04-15T12:10:11Z

Description

add ControlIdentifier to the remote FDB networs (to prevent specific operations from selected networks)

Contributor Declaration

By opening this pull request, I affirm the following:

All authors agree to the Contributor License Agreement.
The code follows the project's coding standards.
I have performed self-review and added comments where needed.
I have added or updated tests to verify that my changes are effective and functional.
I have run all existing tests and confirmed they pass.

🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/fdb/pull-requests/PR-267

Copilot

Pull request overview

This PR introduces per-network “control flags” for remote FDB connections, with the immediate goal of disabling wipe operations for selected client networks.

Changes:

Add parsing of control flags into ControlIdentifiers from configuration objects.
Wire network-derived ControlIdentifiers into the remote catalogue server and gate wipe-related control messages.
Refactor FDBBase initialisation to construct controlIdentifiers_ directly from config.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/fdb5/remote/server/CatalogueHandler.h`	Adds `ControlIdentifiers` state and a helper to check whether wipe is allowed.
`src/fdb5/remote/server/CatalogueHandler.cc`	Applies wipe gating to wipe-related messages and initialises control flags from matched network config.
`src/fdb5/api/helpers/ControlIterator.h`	Adds a `ControlIdentifiers(LocalConfiguration)` constructor declaration (and new include).
`src/fdb5/api/helpers/ControlIterator.cc`	Implements config-based control-flag parsing into the internal bitmask.
`src/fdb5/api/FDBFactory.cc`	Refactors `FDBBase` ctor to initialise `controlIdentifiers_` via the new constructor.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-15T12:17:34Z

            case Message::Wipe:  // Initial wipe request
-                wipe(clientID, requestID, std::move(payload));
-                return Handled::Yes;
+                if (isWipeEnabled(clientID, requestID)) {
+                    wipe(clientID, requestID, std::move(payload));
+                    return Handled::Yes;
+                }
+                return Handled::Replied;

            case Message::DoMaskIndexEntries:
-                // doit! We expect DoMaskIndexEntries, doWipeURIs, DoWipeUnknowns and doWipeEmptyDatabase in succession
-                doMaskIndexEntries(clientID, requestID, std::move(payload));
-                return Handled::Yes;
+                if (isWipeEnabled(clientID, requestID)) {
+                    // doit! We expect DoMaskIndexEntries, doWipeURIs, DoWipeUnknowns and doWipeEmptyDatabase in
+                    // succession
+                    doMaskIndexEntries(clientID, requestID, std::move(payload));
+                    return Handled::Yes;
+                }
+                return Handled::Replied;

            case Message::DoWipeURIs:  // Do the wipe on our currentWipeState
-                doWipeURIs(clientID, requestID, std::move(payload));
-                return Handled::Yes;
+                if (isWipeEnabled(clientID, requestID)) {
+                    doWipeURIs(clientID, requestID, std::move(payload));
+                    return Handled::Yes;
+                }
+                return Handled::Replied;

            case Message::DoWipeFinish:  // Finish wipe by deleting empty DBs
-                doWipeEmptyDatabase(clientID, requestID, std::move(payload));
-                return Handled::Yes;
+                if (isWipeEnabled(clientID, requestID)) {
+                    doWipeEmptyDatabase(clientID, requestID, std::move(payload));
+                    return Handled::Yes;
+                }
+                return Handled::Replied;

            case Message::DoWipeUnknowns:  // Wipe a set of unknown URIs
-                doWipeUnknowns(clientID, requestID, std::move(payload));
-                return Handled::Yes;
+                if (isWipeEnabled(clientID, requestID)) {
+                    doWipeUnknowns(clientID, requestID, std::move(payload));
+                    return Handled::Yes;
+                }


This new wipe-gating behaviour (based on per-network control flags) isn’t covered by the existing remote protocol tests. It would be good to add a remote test configuration that defines a networks entry matching the test client (e.g., localhost netmask) with wipe: false, and assert that FDB{}.wipe(...) fails with the expected error.

codecov-commenter · 2026-04-15T12:33:31Z

Codecov Report

❌ Patch coverage is 22.97297% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.05%. Comparing base (49d9ded) to head (e13ff30).

Files with missing lines	Patch %	Lines
src/fdb5/remote/server/CatalogueHandler.cc	20.00%	28 Missing ⚠️
src/fdb5/api/helpers/ControlIterator.cc	23.52%	26 Missing ⚠️
src/fdb5/remote/server/ServerConnection.h	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #267      +/-   ##
===========================================
- Coverage    71.17%   71.05%   -0.12%     
===========================================
  Files          376      376              
  Lines        23786    23834      +48     
  Branches      2480     2483       +3     
===========================================
+ Hits         16930    16936       +6     
- Misses        6856     6898      +42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ChrisspyB

Pending test.

I am still worried about negligent / AI wipe commands from within a "trusted" network.

ChrisspyB · 2026-04-16T20:48:12Z

+    if (!config.getBool("archive", writable)) {
+        value_ |= static_cast<value_type>(ControlIdentifier::Archive);
+    }
+    if (!config.getBool("wipe", writable)) {


So, to be clear, we set this bit to 1 if:

Wipe is set to false in the config.

Wipe is not specified, but writable is explicitly false in config.

So, I assume value_ is going to store the set of actions we cannot do.

And we set to 0 if:

Wipe is set to true.

Wipe unspecified but writable is true

If both wipe and writable not specified in config (i.e. by default)

And 0 means it is allowed i.e. wiping is by default enabled. Is this correct?

Worth adding a comment to this ctor that variables marked true in the config correspond to 0 in the ControlIdentifers, which is an aspect of the ControlIdentifiers I dislike.

After looking at the enabled function in the ControlIdentifiers class, I think Chris is right and we should at least leave a comment describing that behavior.

If this isn't true, we should definitely leave a comment, describing the behavior.

I would default to non-wipeable, too.

ChrisspyB · 2026-04-16T20:53:23Z

+    eckit::net::IPAddress clientIPaddress{controlSocket_.remoteAddr()};
+
+    clientNetwork_ = "";
+    if (config_.has("networks")) {


If I have a typo in my serverside config, e.g. I write network instead of networks, none of this logic is called, and (I think) everyone can wipe my data.

I'm inclined to say that for remote wipe it should default to being disabled.

tbkr

Would be good to documentation the behavior of the ControlIdentifiers and the mapping to configuration.

simondsmart

In addition to the above, I want the ability to (fully) disable the wipe function on certain interfaces of the Store.

simondsmart · 2026-04-17T11:09:17Z

    s >> value_;
 }

+ControlIdentifiers::ControlIdentifiers(const eckit::LocalConfiguration& config) : value_(0) {


I don't like this at all.

This ControlIdentifiers class is used in a number of places. It is used in the lock/unlock control functionality. And it is used here in the enable/disable logic. But those should both be logic which uses ControlIdentifiers rather than logic that is in ControlIdentifiers.

So, I'm really not at all convinced that the logic to extract enable/disable from config belongs inside this class. This is visible in the PR in that only some of the logic is here - the rest is in the CatalogueHandler.cc file. Yuck.

I also note Chris' comment below, such that the boolean logic is the inverse of what one would naively expect. We should probably fix that.

simondsmart · 2026-04-17T11:10:47Z


+void CatalogueHandler::checkIsEnabled(ControlIdentifier identifier) {
+    if (!controlIdentifiers_.enabled(identifier)) {
+        throw UnhandledOperationException(identifier);


This is not an unhandled operation (which draws to mind not-implemented, or similar). It is a disabled operation. A relative of PermissionDenied (the exception might derive from that)

simondsmart · 2026-04-17T11:12:28Z

                inspect(clientID, requestID, std::move(payload));
                return Handled::Yes;

            case Message::Stats:  // inspect request. Location are sent aynchronously over the data connection


Stats/Exist/Axes/List should also be considered for permissions.

simondsmart · 2026-04-17T11:12:44Z

 }

 void CatalogueHandler::wipe(uint32_t clientID, uint32_t requestID, eckit::Buffer&& payload) {
+    checkIsEnabled(ControlIdentifier::Wipe);


Is checkIsEnabled not called in handleControl()?

simondsmart · 2026-04-17T11:19:17Z

+                eckit::net::NetMask netmask{net.getString("netmask")};
+                if (netmask.contains(clientIPaddress)) {
+                    clientNetwork_ = net.getString("name");
+                    controlIdentifiers_ = ControlIdentifiers(net);


These apply in addition (?) to the logic in the contained FDB object?

danovaro requested review from ChrisspyB, Copilot and tbkr April 15, 2026 12:10

Copilot started reviewing on behalf of danovaro April 15, 2026 12:10 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

add control flags to fdb remote networks

c194460

danovaro force-pushed the feature/wipe-permission branch from 5cd054d to c194460 Compare April 15, 2026 12:36

FDB-663 generalised permission checking on remote actions

e13ff30

ChrisspyB reviewed Apr 16, 2026

View reviewed changes

tbkr requested changes Apr 17, 2026

View reviewed changes

simondsmart requested changes Apr 17, 2026

View reviewed changes

Conversation

danovaro commented Apr 15, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor Declaration

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ChrisspyB left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisspyB Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbkr Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisspyB Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbkr left a comment

Choose a reason for hiding this comment

Uh oh!

simondsmart left a comment

Choose a reason for hiding this comment

Uh oh!

simondsmart Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

simondsmart Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

simondsmart Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

simondsmart Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

simondsmart Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danovaro commented Apr 15, 2026 •

edited by github-actions Bot

Loading

codecov-commenter commented Apr 15, 2026 •

edited

Loading

ChrisspyB Apr 16, 2026 •

edited

Loading

tbkr Apr 17, 2026 •

edited

Loading

ChrisspyB Apr 16, 2026 •

edited

Loading