CLDSRV-584 Limit backbeat API versioning check to replication operations #5707

nicolas2bert · 2024-11-26T13:59:10Z

Context:

In Artesca, when creating a location (whether used for replication or not), you specify both the endpoint and the bucket name where data will be stored. At this stage, Zenko checks that the bucket has versioning enabled.

In S3C, it is a bit different, a “replication location” consists of a list of S3C endpoints (replicationEndpoints), and the destination bucket is specified later in the API replication rule (with put-bucket-replication API). Currently, S3C does not check if the destination bucket has versioning enabled. This means data could be replicated from a versioned source bucket to a non-versioned or suspended destination bucket, which can cause issues. e.g. https://scality.atlassian.net/browse/S3C-821

Current solution:

A validation step was added to the Backbeat API in cloudserver. This check rejects any requests made to buckets that either do not have versioning enabled or have versioning suspended. This check makes sure the Backbeat API only works with buckets that have versioning enabled. This prevents operations on destination buckets where versioning is disabled or suspended, hense maintaining proper version control.

On the source, we should be fine. Cloudserver prevents setting up replication on a bucket if its versioning is disabled or suspended, and it prevents changing a bucket’s versioning to suspended while replication is ongoing.

Issue:

The Backbeat API is also used for lifecycle, which works on both versioned and non-versioned buckets. The current versioning check affects the lifecycle operations, which should be allowed on non-versioned buckets.

Proposed solution:

Modify the Backbeat API to apply the bucket versioning check only to actions related to the replication destination buckets (such as PUT, POST, and DELETE requests).

Implement a new client header x-scal-versioning-required in the Backbeat client to make sure that replication targets only versioned buckets. When this header is set, Cloudserver will make sure that the destination bucket has versioning enabled, preventing replication to non-versioned buckets.

This change will ensure that replication only targets versioned buckets and allows lifecycle operations on non-versioned buckets.

bert-e · 2024-11-26T13:59:14Z

Hello nicolas2bert,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2024-11-26T13:59:23Z

Branches have diverged

This pull request's source branch bugfix/CLDSRV-584/backbeat-api has diverged from
development/8.8 by more than 50 commits.

To avoid any integration risks, please re-synchronize them using one of the
following solutions:

Merge origin/development/8.8 into bugfix/CLDSRV-584/backbeat-api
Rebase bugfix/CLDSRV-584/backbeat-api onto origin/development/8.8

Note: If you choose to rebase, you may have to ask me to rebuild
integration branches using the reset command.

bert-e · 2024-11-26T13:59:31Z

Branches have diverged

This pull request's source branch bugfix/CLDSRV-584/backbeat-api has diverged from
development/7.70 by more than 50 commits.

To avoid any integration risks, please re-synchronize them using one of the
following solutions:

Merge origin/development/7.70 into bugfix/CLDSRV-584/backbeat-api
Rebase bugfix/CLDSRV-584/backbeat-api onto origin/development/7.70

Note: If you choose to rebase, you may have to ask me to rebuild
integration branches using the reset command.

bert-e · 2024-11-26T14:01:35Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-584 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.10.51
7.70.59
8.6.29
8.7.50
8.8.38

Please check the Fix Version/s of CLDSRV-584, or the target
branch of this pull request.

nicolas2bert · 2024-11-26T14:01:49Z

@bert-e reset

bert-e · 2024-11-26T14:01:56Z

Reset complete

I have successfully deleted this pull request's integration branches.

bert-e · 2024-11-26T14:02:05Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-584 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.10.51
7.70.59
8.6.29
8.7.50
8.8.38

Please check the Fix Version/s of CLDSRV-584, or the target
branch of this pull request.

nicolas2bert · 2024-11-26T15:07:45Z

ping

bert-e · 2024-11-26T15:07:57Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

anurag4DSB

LGTM

francoisferrand · 2024-12-04T10:10:53Z

lib/routes/routeBackbeat.js

+            // The following makes sure that only replication destination-related operations (non-GET requests)
+            // target buckets with versioning enabled.
+            // This allows lifecycle operations (which use GET requests) to work on non-versioned buckets.
+            if (request.method !== 'GET' && (!versioningConfig || versioningConfig.Status !== 'Enabled')) {


lifecycle does not only perform GET operations : it also deletes objects (expiration), deletes data associated with objects (transition and restored-object-expiration), and puts metadata of objects...

so i fear such broad filtering in the router may introduce other issues.

→ Is there a way instead to identify the few "actions" performed by CRR replication (e.g. putObjectVersion, putObjectMetadata), maybe with some additional condition on the parameters/fields, to confirm? Then the check cna be performed direclty in each of these action handlers.
→ Otherwise, may be better to "isolate" the calls with specific CRR/lifecycle routes, or maybe simply add an extra parameter to enforce the check (IfVersioned maybe ?)
→ Alternatively, instead of making it a "core" feature in the code, could it be achieve through policies? e.g. the replication user will not have permissions to write to non-versioned bucket? (need to check if this is a kind of policy we support or even can define, but would be an elegant and simple solution I think)

S3C Lifecycle expiration (transition is not supported yet) only uses backbeat client GET operation: getMetadata. Delete object/version is done by the S3 client itself that uses the s3 route.

Is there a way instead to identify the few "actions" performed by CRR replication (e.g. putObjectVersion, putObjectMetadata)

Replication uses putData, putMetadata and batchDelete 's backebatAPI routes. Limiting the versioning check to those actions makes sense and is more explicit. However putMetadata is also used by lifecycle transition so it will not solve the lifecycle transition future implementation.

This PR is a fix, so I am not comfortable introducing new CRR/lifeycle routes for it. But it will be needed once we introduce lifecycle transition in S3C.

the replication user will not have permissions to write to non-versioned bucket?

I thought about it, but it does not exist.

batchDelete route is used for lifecycle as well.

This PR is a fix, so I am not comfortable introducing new CRR/lifeycle routes for it. But it will be needed once we introduce lifecycle transition in S3C.

Fine with me ; however the code will waterflow and in the not-so-long future we should converge Cloudserver: so whatever you change will break lifecycle in Zenko.

So I'd rather make the check conditional anyway, and let the "caller" decide if the bucket must be versionned: e..g. simply add an extra parameter to enforce the check (IfVersioned maybe ?) : then backbeat can be updated to required versioning if and if required, without introducing backbeat logic/constraints in cloudserver.

Yes, I am aligned with it.

bert-e · 2024-12-04T10:11:38Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-584 contains:

7.10.51
7.70.59
8.6.29
8.7.50
8.8.39

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.10.51
7.70.59
8.8.39

Please check the Fix Version/s of CLDSRV-584, or the target
branch of this pull request.

bert-e · 2024-12-09T13:31:20Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-584 contains:

7.10.51
7.70.59
8.6.29
8.7.50
8.8.39

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.10.51
7.70.59
8.8.40

Please check the Fix Version/s of CLDSRV-584, or the target
branch of this pull request.

lib/routes/routeBackbeat.js

bert-e · 2024-12-10T15:42:00Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-584 contains:

7.10.51
7.70.59
8.6.29
8.7.50
8.8.40

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.10.51
7.70.59
8.8.40

Please check the Fix Version/s of CLDSRV-584, or the target
branch of this pull request.

bert-e · 2024-12-12T11:04:06Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-584 contains:

7.10.51
7.70.59
8.6.29
8.7.50
8.8.40

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.10.51
7.70.59
8.8.40
8.9.0

Please check the Fix Version/s of CLDSRV-584, or the target
branch of this pull request.

francoisferrand · 2024-12-12T11:09:56Z

lib/routes/routeBackbeat.js

+            // The following makes sure that only replication destination-related operations
+            // target buckets with versioning enabled.
+            // This allows lifecycle expiration operations (getMetadata) to work on non-versioned buckets.
+            const isCRRDestinationRequest = request.method === 'PUT' &&
+                (request.resourceType === 'data' || request.resourceType === 'metadata');
+            if (isCRRDestinationRequest && (!versioningConfig || versioningConfig.Status !== 'Enabled')) {


To reduce coupling, I'd rather make this generic and let the caller (i.e. backbeat, who has full context!) specify if versioning is required, without introducing replication-related logic (which could actually change!) here.

something like:

Suggested change

// The following makes sure that only replication destination-related operations

// target buckets with versioning enabled.

// This allows lifecycle expiration operations (getMetadata) to work on non-versioned buckets.

const isCRRDestinationRequest = request.method === 'PUT' &&

(request.resourceType === 'data' || request.resourceType === 'metadata');

if (isCRRDestinationRequest && (!versioningConfig || versioningConfig.Status !== 'Enabled')) {

if (request.headers['x-scal-if-versionning-enabled'] && (!versioningConfig || versioningConfig.Status !== 'Enabled')) {

francoisferrand · 2024-12-13T10:42:35Z

lib/routes/routeBackbeat.js

-            if (!versioningConfig || versioningConfig.Status !== 'Enabled') {
+            // The following makes sure that only replication destination-related operations
+            // target buckets with versioning enabled.
+            const isVersioningEnabled = request.headers['x-scal-versioning-enabled'] === 'true';


nit: isVersioningEnabled should be something like checkVersioningEnabled

similarly, i wonder if the header should be renamed to something more explicit, like x-scal-versioning-required or x-scal-if-versioning-enabled ?

bert-e · 2024-12-13T10:42:49Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-584 contains:

7.10.51
7.70.59
8.6.29
8.7.50
8.8.40

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.10.51
7.70.59
8.8.40
9.0.0

Please check the Fix Version/s of CLDSRV-584, or the target
branch of this pull request.

francoisferrand · 2024-12-13T10:43:03Z

tests/functional/raw-node/test/routes/routeBackbeat.js

@@ -1513,13 +1514,16 @@ describeSkipIfAWS('backbeat routes', () => {
            done();
        }));


should add tests to verify versioning is not checked when x-scal-versioning-enabled is not defined or false.
("should not refuse PUT data when bucket is not versioned if x-scal-versioning-enabled is not set")

francoisferrand

LGTM, just some small cleanup.

In addition, I wonder if we should be more strict and fail the request if this header is passed to other routes (which do not support it) ?

nicolas2bert · 2024-12-20T07:59:22Z

@bert-e approve

bert-e · 2024-12-20T07:59:35Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.8/bugfix/CLDSRV-584/backbeat-api with contents from w/7.70/bugfix/CLDSRV-584/backbeat-api
and development/8.8.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.8/bugfix/CLDSRV-584/backbeat-api origin/development/8.8
 $ git merge origin/w/7.70/bugfix/CLDSRV-584/backbeat-api
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.8/bugfix/CLDSRV-584/backbeat-api

The following options are set: approve

bert-e · 2024-12-20T08:39:46Z

History mismatch

Merge commit #0ec3584cbe3dd3ed61cc0f305ec4d6704b72b177 on the integration branch
w/7.70/bugfix/CLDSRV-584/backbeat-api is merging a branch which is neither the current
branch bugfix/CLDSRV-584/backbeat-api nor the development branch
development/7.70.

It is likely due to a rebase of the branch bugfix/CLDSRV-584/backbeat-api and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

The following options are set: approve

Context: In Artesca, when creating a location (whether used for replication or not), you specify both the endpoint and the bucket name where data will be stored. At this stage, Zenko checks that the bucket has versioning enabled. In S3C, it is a bit different, a “replication location” consists of a list of S3C endpoints (replicationEndpoints), and the destination bucket is specified later in the API replication rule (with put-bucket-replication API). Currently, S3C does not check if the destination bucket has versioning enabled. This means data could be replicated from a versioned source bucket to a non-versioned or suspended destination bucket, which can cause issues. e.g. https://scality.atlassian.net/browse/S3C-821 Current solution: A validation step was added to the Backbeat API in cloudserver. This check rejects any requests made to buckets that either do not have versioning enabled or have versioning suspended. This check makes sure the Backbeat API only works with buckets that have versioning enabled. This prevents operations on destination buckets where versioning is disabled or suspended, hense maintaining proper version control. On the source, we should be fine. Cloudserver prevents setting up replication on a bucket if its versioning is disabled or suspended, and it prevents changing a bucket’s versioning to suspended while replication is ongoing. Issue: The Backbeat API is also used for lifecycle, which works on both versioned and non-versioned buckets. The current versioning check affects the lifecycle operations, which should be allowed on non-versioned buckets. Proposed solution: Modify the Backbeat API to apply the bucket versioning check only to actions related to the replication destination buckets (such as PUT, POST, and DELETE requests). Implement a new client header x-scal-versioning-required in the Backbeat client to make sure that replication targets only versioned buckets. When this header is set, Cloudserver will make sure that the destination bucket has versioning enabled, preventing replication to non-versioned buckets. This change will ensure that replication only targets versioned buckets and allows lifecycle operations on non-versioned buckets.

nicolas2bert changed the base branch from development/8.8 to development/7.70 November 26, 2024 13:59

nicolas2bert force-pushed the bugfix/CLDSRV-584/backbeat-api branch from 130300a to 87a5fbf Compare November 26, 2024 14:01

nicolas2bert changed the base branch from development/7.70 to development/7.10 November 26, 2024 14:01

anurag4DSB approved these changes Nov 26, 2024

View reviewed changes

francoisferrand self-assigned this Nov 29, 2024

francoisferrand reviewed Dec 4, 2024

View reviewed changes

francoisferrand requested a review from Kerkesni December 9, 2024 13:30

francoisferrand reviewed Dec 9, 2024

View reviewed changes

lib/routes/routeBackbeat.js Outdated Show resolved Hide resolved

nicolas2bert force-pushed the bugfix/CLDSRV-584/backbeat-api branch from 87a5fbf to b5160b9 Compare December 10, 2024 17:13

francoisferrand reviewed Dec 12, 2024

View reviewed changes

nicolas2bert force-pushed the bugfix/CLDSRV-584/backbeat-api branch 3 times, most recently from 887d250 to b3f446a Compare December 12, 2024 21:09

francoisferrand reviewed Dec 13, 2024

View reviewed changes

francoisferrand approved these changes Dec 13, 2024

View reviewed changes

francoisferrand mentioned this pull request Dec 13, 2024

BB-640 Add VersioningEnabled header to Backbeat client scality/backbeat#2601

Merged

nicolas2bert force-pushed the bugfix/CLDSRV-584/backbeat-api branch from b3f446a to 0ec3584 Compare December 13, 2024 13:36

nicolas2bert force-pushed the bugfix/CLDSRV-584/backbeat-api branch from 0ec3584 to b762233 Compare December 20, 2024 08:39

nicolas2bert force-pushed the bugfix/CLDSRV-584/backbeat-api branch from b762233 to 1bd502a Compare December 20, 2024 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLDSRV-584 Limit backbeat API versioning check to replication operations #5707

CLDSRV-584 Limit backbeat API versioning check to replication operations #5707

nicolas2bert commented Nov 26, 2024 •

edited

Loading

bert-e commented Nov 26, 2024

bert-e commented Nov 26, 2024

bert-e commented Nov 26, 2024

bert-e commented Nov 26, 2024

nicolas2bert commented Nov 26, 2024

bert-e commented Nov 26, 2024

bert-e commented Nov 26, 2024

nicolas2bert commented Nov 26, 2024

bert-e commented Nov 26, 2024

anurag4DSB left a comment

francoisferrand Dec 4, 2024 •

edited

Loading

nicolas2bert Dec 10, 2024

francoisferrand Dec 12, 2024

nicolas2bert Dec 12, 2024

bert-e commented Dec 4, 2024

bert-e commented Dec 9, 2024

bert-e commented Dec 10, 2024

bert-e commented Dec 12, 2024

francoisferrand Dec 12, 2024 •

edited

Loading

nicolas2bert Dec 12, 2024

francoisferrand Dec 13, 2024

bert-e commented Dec 13, 2024

francoisferrand Dec 13, 2024 •

edited

Loading

francoisferrand left a comment

nicolas2bert commented Dec 20, 2024

bert-e commented Dec 20, 2024

bert-e commented Dec 20, 2024

		@@ -1513,13 +1514,16 @@ describeSkipIfAWS('backbeat routes', () => {
		done();
		}));

CLDSRV-584 Limit backbeat API versioning check to replication operations #5707

Are you sure you want to change the base?

CLDSRV-584 Limit backbeat API versioning check to replication operations #5707

Conversation

nicolas2bert commented Nov 26, 2024 • edited Loading

bert-e commented Nov 26, 2024

Hello nicolas2bert,

bert-e commented Nov 26, 2024

Branches have diverged

bert-e commented Nov 26, 2024

Branches have diverged

bert-e commented Nov 26, 2024

Incorrect fix version

nicolas2bert commented Nov 26, 2024

bert-e commented Nov 26, 2024

Reset complete

bert-e commented Nov 26, 2024

Incorrect fix version

nicolas2bert commented Nov 26, 2024

bert-e commented Nov 26, 2024

Request integration branches

anurag4DSB left a comment

Choose a reason for hiding this comment

francoisferrand Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

nicolas2bert Dec 10, 2024

Choose a reason for hiding this comment

francoisferrand Dec 12, 2024

Choose a reason for hiding this comment

nicolas2bert Dec 12, 2024

Choose a reason for hiding this comment

bert-e commented Dec 4, 2024

Incorrect fix version

bert-e commented Dec 9, 2024

Incorrect fix version

bert-e commented Dec 10, 2024

Incorrect fix version

bert-e commented Dec 12, 2024

Incorrect fix version

francoisferrand Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

nicolas2bert Dec 12, 2024

Choose a reason for hiding this comment

francoisferrand Dec 13, 2024

Choose a reason for hiding this comment

bert-e commented Dec 13, 2024

Incorrect fix version

francoisferrand Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

francoisferrand left a comment

Choose a reason for hiding this comment

nicolas2bert commented Dec 20, 2024

bert-e commented Dec 20, 2024

Conflict

bert-e commented Dec 20, 2024

History mismatch

nicolas2bert commented Nov 26, 2024 •

edited

Loading

francoisferrand Dec 4, 2024 •

edited

Loading

francoisferrand Dec 12, 2024 •

edited

Loading

francoisferrand Dec 13, 2024 •

edited

Loading