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).

Allow GET actions like getMetadata and headLocation, which are used in workflows that don’t affect replication destination 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.

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.

francoisferrand · 2024-12-09T13:32:50Z

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')) {


Replication is supposed to work on any versioned bucket : so it must work as well on version-suspended buckets

Suggested change

if (request.method !== 'GET' && (!versioningConfig || versioningConfig.Status !== 'Enabled')) {

if (request.method !== 'GET' && (!versioningConfig || versioningConfig.Status === 'Disabled')) {

In CRR replication both source and replication have to be version enabled. I will keep enforcing it.

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.

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). Allow GET actions like getMetadata and headLocation, which are used in workflows that don’t affect replication destination 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

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

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

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

bert-e commented Dec 4, 2024

bert-e commented Dec 9, 2024

francoisferrand Dec 9, 2024

nicolas2bert Dec 10, 2024

bert-e commented Dec 10, 2024

	if (request.method !== 'GET' && (!versioningConfig \|\| versioningConfig.Status !== 'Enabled')) {
	if (request.method !== 'GET' && (!versioningConfig \|\| versioningConfig.Status === 'Disabled')) {

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

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

bert-e commented Dec 4, 2024

Incorrect fix version

bert-e commented Dec 9, 2024

Incorrect fix version

francoisferrand Dec 9, 2024

Choose a reason for hiding this comment

nicolas2bert Dec 10, 2024

Choose a reason for hiding this comment

bert-e commented Dec 10, 2024

Incorrect fix version

francoisferrand Dec 4, 2024 •

edited

Loading