[#482] Fix containers resolving #482

mbiryukova · 2024-09-03T14:55:38Z

mbiryukova commented

2024-09-03 14:55:38 +00:00

Signed-off-by: Marina Biryukova m.biryukova@yadro.com

Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>

mbiryukova self-assigned this 2024-09-03 14:55:38 +00:00

mbiryukova added 1 commit 2024-09-03 14:55:42 +00:00

[#xxx] Fix containers resolving

/ DCO (pull_request) Failing after 1m41s

Details

/ Vulncheck (pull_request) Successful in 2m44s

Details

/ Builds (1.22) (pull_request) Successful in 2m48s

Details

/ Builds (1.23) (pull_request) Successful in 2m49s

Details

/ Lint (pull_request) Successful in 4m1s

Details

/ Tests (1.22) (pull_request) Successful in 2m51s

Details

/ Tests (1.23) (pull_request) Successful in 2m51s

Details

8ff19bfecb

Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>

mbiryukova force-pushed bugfix/containers_resolving from 8ff19bfecb to 01434ed747

2024-09-03 14:56:08 +00:00

Compare

mbiryukova changed title from ~~[#xxx] Fix containers resolving~~ to [#482] Fix containers resolving

2024-09-03 14:56:31 +00:00

mbiryukova requested review from storage-services-committers 2024-09-03 15:00:57 +00:00

mbiryukova requested review from storage-services-developers 2024-09-03 15:00:57 +00:00

dkirillov reviewed 2024-09-04 06:43:16 +00:00

api/resolver/resolver.go Outdated

					
				@ -213,3 +214,3 @@

					resolveFunc := func(ctx context.Context, name string) (cid.ID, error) {

						var d container.Domain

						d.SetName(name)

						if strings.Contains(name, ".") {

dkirillov commented

2024-09-04 06:43:16 +00:00

Actually bucket name can consist dot. So we cannot use such logic.
I suggest not to use bucket resolver to resolve containers from config on application startup.
Let's use separate function to resolve such containers

Actually bucket name can consist dot. So we cannot use such logic. I suggest not to use bucket resolver to resolve containers from config on application startup. Let's use separate function to resolve such containers

alexvanin commented

2024-09-04 07:56:41 +00:00

As an alternative, we can change Resolve(ctx context.Context, name string) signature by passing zone name explicitly. Both DNS and NNS resolver implementations take zone from context.

reqInfo := middleware.GetReqInfo(ctx)
zone, isDefault := settings.FormContainerZone(reqInfo.Namespace)

For S3 operations, pass zone from reqInfo. As for settings container resolving, use splitting by '.'

As an alternative, we can change `Resolve(ctx context.Context, name string)` signature by passing zone name explicitly. Both DNS and NNS resolver implementations take zone from context. ``` reqInfo := middleware.GetReqInfo(ctx) zone, isDefault := settings.FormContainerZone(reqInfo.Namespace) ``` For S3 operations, pass zone from `reqInfo`. As for settings container resolving, use splitting by '.'

dkirillov marked this conversation as resolved

mbiryukova force-pushed bugfix/containers_resolving from 01434ed747 to 7d59a1b1d3

2024-09-04 09:54:18 +00:00

Compare

dkirillov reviewed 2024-09-04 11:11:45 +00:00

api/resolver/resolver.go Outdated

					
				@ -171,3 +164,3 @@

					var dns ns.DNS

					resolveFunc := func(ctx context.Context, name string) (cid.ID, error) {

					resolveFunc := func(ctx context.Context, name, zone string, isDefault bool) (cid.ID, error) {

dkirillov commented

2024-09-04 11:11:45 +00:00

I don't like isDefault bool param. Maybe we can keep settings that has method to find out if provided zone is default?

Or just compare zone with v2container.SysAttributeZoneDefault inside this function

I don't like `isDefault bool` param. Maybe we can keep `settings` that has method to find out if provided zone is default? Or just compare zone with `v2container.SysAttributeZoneDefault ` inside this function

dkirillov marked this conversation as resolved

dkirillov reviewed 2024-09-04 11:15:57 +00:00

cmd/s3-gw/app.go Outdated

					
				@ -1063,3 +1063,3 @@

					var id cid.ID

					if err = id.DecodeString(containerString); err != nil {

						if id, err = a.bucketResolver.Resolve(ctx, containerString); err != nil {

						parts := strings.Split(containerString, ".")

dkirillov commented

2024-09-04 11:15:57 +00:00

It seems we should check if parts has length 2

dkirillov marked this conversation as resolved

mbiryukova force-pushed bugfix/containers_resolving from 7d59a1b1d3 to 8bc5ae47d0

2024-09-04 14:33:02 +00:00

Compare

dkirillov reviewed 2024-09-05 06:33:49 +00:00

api/layer/layer.go Outdated

					
				@ -329,3 +329,3 @@

					}

					containerID, err := n.ResolveBucket(ctx, name)

					containerID, err := n.ResolveBucket(ctx, name, zone)

dkirillov commented

2024-09-05 06:33:49 +00:00

Minor (non-blocking): It's a little odd to see:

n.cache.GetBucket(zone, name)
//...
n.ResolveBucket(ctx, name, zone)

in one case we use order zone,name, in other name,zone. Ideally it should be consistent (especially because they are close to each other)

Minor (non-blocking): It's a little odd to see: ```golang n.cache.GetBucket(zone, name) //... n.ResolveBucket(ctx, name, zone) ``` in one case we use order `zone`,`name`, in other `name`,`zone`. Ideally it should be consistent (especially because they are close to each other)

dkirillov approved these changes 2024-09-05 06:34:04 +00:00

Dismissed

mbiryukova force-pushed bugfix/containers_resolving from 8bc5ae47d0 to 6259caccb8

2024-09-05 08:20:33 +00:00

Compare

mbiryukova dismissed dkirillov's review 2024-09-05 08:20:33 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

alexvanin reviewed 2024-09-05 08:41:56 +00:00

cmd/s3-gw/app.go Outdated

					
				@ -1064,2 +1064,3 @@

					if err = id.DecodeString(containerString); err != nil {

						if id, err = a.bucketResolver.Resolve(ctx, containerString); err != nil {

						parts := strings.Split(containerString, ".")

						if len(parts) != 2 {

alexvanin commented

2024-09-05 08:41:56 +00:00

I am not sure this check is valid. I think it should be possible to define lifecycle.s3.reserved container, for example.

So we can rewrite it like that

i := strings.Index(containerString, ".")
if i < 0 {
    return nil, fmt.Errorf(...)
}
if id, err = a.bucketResolver.Resolve(ctx, containerString[:i], containerString[i+1:]); err != nil {

I am not sure this check is valid. I think it should be possible to define `lifecycle.s3.reserved` container, for example. So we can rewrite it like that ```go i := strings.Index(containerString, ".") if i < 0 { return nil, fmt.Errorf(...) } if id, err = a.bucketResolver.Resolve(ctx, containerString[:i], containerString[i+1:]); err != nil { ```

mbiryukova commented

2024-09-05 08:51:52 +00:00

In this case, zone should be s3.reserved?

In this case, zone should be `s3.reserved`?

alexvanin commented

2024-09-05 08:58:45 +00:00

I suppose so. The same as namespace zone which is <namespace-name>.ns.

And probably it doesn't matter for system container, because NNS resolver is going to glue domain name and zone anyway (link).

I suppose so. The same as namespace zone which is `<namespace-name>.ns`. And _probably_ it doesn't matter for system container, because NNS resolver is going to glue domain name and zone anyway ([link](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/branch/master/ns/nns.go#L97)).

👍 1