Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to sdk rc10 #807

Merged
merged 12 commits into from
Aug 15, 2023
Merged

Upgrade to sdk rc10 #807

merged 12 commits into from
Aug 15, 2023

Conversation

smallhive
Copy link
Contributor

closes #806

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #807 (cb7d1e5) into master (d013f71) will decrease coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is 4.39%.

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
- Coverage   34.35%   34.33%   -0.02%     
==========================================
  Files          61       61              
  Lines       10483    10485       +2     
==========================================
- Hits         3601     3600       -1     
- Misses       6481     6484       +3     
  Partials      401      401              
Files Changed Coverage Δ
api/handler/multipart_upload.go 19.42% <0.00%> (-0.16%) ⬇️
api/handler/notifications.go 30.82% <0.00%> (ø)
authmate/authmate.go 0.00% <0.00%> (ø)
internal/neofs/neofs.go 1.43% <0.00%> (-0.03%) ⬇️
internal/neofs/tree.go 4.59% <0.00%> (ø)
api/layer/layer.go 37.71% <75.00%> (+0.53%) ⬆️
api/layer/neofs_mock.go 58.67% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmd/s3-gw/app.go Outdated Show resolved Hide resolved
cmd/s3-gw/app_metrics.go Outdated Show resolved Hide resolved
@smallhive smallhive force-pushed the 806-upgrade-to-sdk-rc10 branch 4 times, most recently from 250ad55 to 518fd38 Compare August 3, 2023 10:29
@smallhive smallhive marked this pull request as ready for review August 3, 2023 11:03
internal/neogo/sidechain/sidechain.go Outdated Show resolved Hide resolved
api/resolver/resolver.go Outdated Show resolved Hide resolved
@@ -85,7 +85,7 @@ func prepareHandlerContext(t *testing.T) *handlerContext {

layerCfg := &layer.Config{
Caches: layer.DefaultCachesConfigs(zap.NewExample()),
AnonKey: layer.AnonymousKey{Key: key},
GateKey: key,
Copy link
Member

Choose a reason for hiding this comment

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

It's supposed to be a feature, see #271. Although I don't quite understand it.

Copy link
Contributor Author

@smallhive smallhive Aug 4, 2023

Choose a reason for hiding this comment

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

On current master it doesn't work:

$ aws s3api create-bucket --bucket $BUCKET --endpoint http://localhost:9080 --no-sign-request

An error occurred (InternalError) when calling the CreateBucket operation (reached max retries: 0): We encountered an internal error, please try again.

In the gate logs:

2023-08-04T07:42:20.324+0400    error   handler/util.go:29      call method     {"status": 500, "request_id": "dc3da4ba-b5b0-42aa-800f-2dbb71b80ddc", "method": "CreateBucket", "bucket": "heh3", "object": "", "description": "couldn't get bearer token signature key", "error": "couldn't get box data from context"}

In any case, we have to sign requests. Many client calls requires Signer as an explicit parameter. I tend to think it doesn't matter what key it should be either a gate key or an anonymous one.
But the gate key we have to have because all our sessions were created for the gate key. And we need to sign requests by it

I also agreed with comment in #271 because we can't create container with random key, because containerPut is not a free operation. Signer is required and must not be nil. The account corresponding to the specified Signer will be charged for the operation.

Copy link
Member

Choose a reason for hiding this comment

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

It can be used for reads, I guess.

api/layer/layer.go Outdated Show resolved Hide resolved
cmd/s3-gw/app_metrics.go Outdated Show resolved Hide resolved
api/handler/multipart_upload.go Show resolved Hide resolved
@smallhive
Copy link
Contributor Author

Added anonymous key using for the containerList operation

@smallhive smallhive force-pushed the 806-upgrade-to-sdk-rc10 branch 3 times, most recently from 36d2d04 to b777c3c Compare August 7, 2023 04:29
@smallhive
Copy link
Contributor Author

smallhive commented Aug 7, 2023

Updated to the latest RC-10 SDK and neofs-contract versions

cmd/s3-gw/app.go Show resolved Hide resolved
api/resolver/resolver.go Outdated Show resolved Hide resolved
@@ -84,6 +84,11 @@ func (n *layer) containerList(ctx context.Context) ([]*data.BucketInfo, error) {
res []cid.ID
rid = api.GetRequestID(ctx)
)

if api.IsAnonymousRequest(ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

Why only this request? I think we can have the same in other ones like object get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this check to the old place, inside Owner func. After that it will work like previous implementation

Copy link
Member

Choose a reason for hiding this comment

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

But what about keys? I suppose we should use a different key when we're doing anonymous requests. Maybe layer should return user.Signer now to provide both at the same time (gw normally and anon if no auth)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I revised the code one more time.
We have the next component chain (gate component names):

  • layer -> neofs -> pool

layer - handles API requests, calls neofs component to prepare data and then it call pool to execute request in neofs node.

In fact, the layer even doesn't need to know something about a key or signer. We will drop the key from it.
The neofs component stores the same key (pool also stores it) and it is a good place to make magic with anonymous keys. We just need to pass isAnonymous flag to neofs component and anonymous signer will be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes in separate commits. At first look, I replaced all places when we cat override the signer. In other places, random keys are already in use by SDK client.

If these changes ok, even after few iterations, I will squash them to proper commit, if required

@smallhive smallhive force-pushed the 806-upgrade-to-sdk-rc10 branch 2 times, most recently from 5deac8b to 5934df7 Compare August 8, 2023 04:13
api/resolver/resolver.go Outdated Show resolved Hide resolved
cmd/s3-gw/app.go Show resolved Hide resolved
api/layer/anonymous.go Outdated Show resolved Hide resolved
api/layer/container.go Outdated Show resolved Hide resolved
@@ -84,6 +84,11 @@ func (n *layer) containerList(ctx context.Context) ([]*data.BucketInfo, error) {
res []cid.ID
rid = api.GetRequestID(ctx)
)

if api.IsAnonymousRequest(ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

But what about keys? I suppose we should use a different key when we're doing anonymous requests. Maybe layer should return user.Signer now to provide both at the same time (gw normally and anon if no auth)?

@smallhive smallhive force-pushed the 806-upgrade-to-sdk-rc10 branch 2 times, most recently from 6c1b743 to db8db45 Compare August 10, 2023 10:10
cmd/s3-gw/app.go Outdated Show resolved Hide resolved
internal/neofs/neofs.go Outdated Show resolved Hide resolved
internal/neofs/neofs.go Outdated Show resolved Hide resolved
api/layer/layer.go Show resolved Hide resolved
cmd/s3-gw/app.go Show resolved Hide resolved
Signed-off-by: Evgenii Baidakov <[email protected]>
The removing changes was made in 70957d7#diff-efcc85eef482b2cb183da5fd07ad20378963ee903b8876f59ae2b957ee684f67L393.
The bug leads to almost infinite waiting for the waiting the result of setEACL operation.
Fixing the bug may improve/fix #800 and nspcc-dev/neofs-testcases#591.

Signed-off-by: Evgenii Baidakov <[email protected]>
In some cases in may be useful. To take data from public bucket or check the access rules for object

Signed-off-by: Evgenii Baidakov <[email protected]>
Signed-off-by: Evgenii Baidakov <[email protected]>
Signed-off-by: Evgenii Baidakov <[email protected]>
Signed-off-by: Evgenii Baidakov <[email protected]>
Signed-off-by: Evgenii Baidakov <[email protected]>
@roman-khimov roman-khimov merged commit 5c083a9 into master Aug 15, 2023
10 of 12 checks passed
@roman-khimov roman-khimov deleted the 806-upgrade-to-sdk-rc10 branch August 15, 2023 13:45
@roman-khimov roman-khimov mentioned this pull request Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to SDK RC10
2 participants