TrueCloudLab/frostfs-http-gw
17
1
Fork 18
Code Issues11 Pull requests Projects Releases16 Packages1 Activity Actions

[] Support CORS container for CORS settings #220

Merged
alexvanin merged 1 commit from mbiryukova/frostfs-http-gw:feature/cors_container into master 2025-03-12 06:33:58 +00:00
Conversation 0 Commits 1 Files changed 18 +1204 -203
mbiryukova commented 2025-02-27 09:29:32 +00:00
Member

Closes

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

Closes #212 Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>
mbiryukova self-assigned this 2025-02-27 09:29:33 +00:00
mbiryukova added 1 commit 2025-02-27 09:29:33 +00:00 
Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>
requested reviews from storage-services-developers, storage-services-committers 2025-02-27 09:29:33 +00:00
mbiryukova force-pushed feature/cors_container from 186578c264 to de5f362867 2025-02-27 10:04:05 +00:00 Compare
dkirillov reviewed 2025-02-28 09:08:48 +00:00
cmd/http-gw/app.go Outdated
@ -105,6 +110,7 @@ type (
bufferMaxSizeForPut uint64
namespaceHeader string
defaultNamespaces []string
corsSet bool
dkirillov commented 2025-02-28 08:47:44 +00:00
Member

Probably we can combine all cors setting in one struct and use one param in settings instead of corsSet, corsAllowOrigin etc ?

Probably we can combine all cors setting in one struct and use one param in settings instead of `corsSet`, `corsAllowOrigin` etc ?
dkirillov marked this conversation as resolved
cmd/http-gw/app.go Outdated
@ -262,0 +260,4 @@
func (a *app) initContainers(ctx context.Context) {
corsCnrInfo, err := a.fetchContainerInfo(ctx, cfgContainersCORS)
if err != nil {
a.log.Fatal(logs.CouldNotFetchCORSContainerInfo, zap.Error(err), logs.TagField(logs.TagApp))
dkirillov commented 2025-02-28 08:49:18 +00:00
Member

Do we need fatal here if cors.* config variables are set?

Do we need `fatal` here if `cors.*` config variables are set?
mbiryukova commented 2025-02-28 10:01:14 +00:00
Author
Member

Seems that no, but cors.* settings are SIGHUP reloadable and container is not

Seems that no, but `cors.*` settings are SIGHUP reloadable and container is not
👍 1
dkirillov marked this conversation as resolved
cmd/http-gw/app.go Outdated
@ -610,3 +626,3 @@
// Configure router.
a.configureRouter(handle)
a.configureRouter()
dkirillov commented 2025-02-28 08:53:12 +00:00
Member

Probably we can move invocation handler.New into a.configureRouter to be more explicit the fact of this two calls must be done in strict order?

Probably we can move invocation `handler.New` into `a.configureRouter` to be more explicit the fact of this two calls must be done in strict order?
dkirillov marked this conversation as resolved
cmd/http-gw/app.go Outdated
@ -1138,0 +1161,4 @@
return &data.BucketInfo{
CID: id,
HomomorphicHashDisabled: container.IsHomomorphicHashingDisabled(res),
dkirillov commented 2025-02-28 08:58:01 +00:00
Member

I'ms not sure if we really need this. Probably we can return just cid.ID. Yes in s3-gw this is used (but there we write to such container so we have to know IsHomomorphicHashingDisabled)

I'ms not sure if we really need this. Probably we can return just `cid.ID`. Yes in s3-gw this is used (but there we write to such container so we have to know `IsHomomorphicHashingDisabled`)
dkirillov marked this conversation as resolved
internal/handler/cors.go Outdated
@ -0,0 +54,4 @@
corsRule := h.config.CORS()
if corsRule != nil {
setCORSHeadersFromRule(c, corsRule)
dkirillov commented 2025-02-28 09:03:52 +00:00
Member

Should we use the same logic as below even when we use overrides from config?

Should we use the same logic as below even when we use overrides from config?
internal/handler/cors.go Outdated
@ -0,0 +246,4 @@
}
if headErr != nil {
return oid.ID{}, headErr
dkirillov commented 2025-02-28 09:08:40 +00:00
Member

If we want to return error when any head is failed we should return true on 238 line.
For now we return this error only if it occurred for last object in search, but this last object can be different.

If we want to return error when any head is failed we should `return true` on 238 line. For now we return this error only if it occurred for last object in search, but this last object can be different.
👍 1
dkirillov marked this conversation as resolved
internal/service/frostfs/frostfs.go Outdated
@ -70,7 +67,7 @@ func (x *FrostFS) CreateObject(ctx context.Context, prm handler.PrmObjectCreate)
prmPut.UseBearer(*prm.BearerToken)
}
idObj, err := x.pool.PutObject(qostagging.ContextWithIOTag(ctx, clientIOTag), prmPut)
dkirillov commented 2025-02-28 08:34:51 +00:00
Member

Why so?

Why so?
mbiryukova commented 2025-02-28 09:31:48 +00:00
Author
Member

client is the default value of tag, only internal needs to be explicitly set. Did the same as in other services

`client` is the default value of tag, only `internal` needs to be explicitly set. Did the same as in other services
dkirillov marked this conversation as resolved
mbiryukova force-pushed feature/cors_container from de5f362867 to 26aa71b074 2025-03-10 08:04:08 +00:00 Compare
mbiryukova force-pushed feature/cors_container from 26aa71b074 to 9ef6b06e91 2025-03-10 15:13:32 +00:00 Compare
dkirillov reviewed 2025-03-11 07:56:17 +00:00
internal/handler/cors.go
@ -0,0 +283,4 @@
return versionID1.EncodeToString() < versionID2.EncodeToString()
}
return unixTime1 < unixTime2
dkirillov commented 2025-03-11 07:56:17 +00:00
Member

Do we have guarantee that timestamp is always unix time (seconds, and not milliseconds)?

Do we have guarantee that timestamp is always unix time (seconds, and not milliseconds)?
dkirillov approved these changes 2025-03-11 07:57:08 +00:00
dkirillov left a comment
Member

LGTM, but see comments

LGTM, but see comments
alexvanin added this to the v0.33.0 milestone 2025-03-11 08:47:08 +00:00
alexvanin approved these changes 2025-03-12 06:33:54 +00:00
alexvanin merged commit 9ef6b06e91 into master 2025-03-12 06:33:58 +00:00
alexvanin deleted branch feature/cors_container 2025-03-12 06:34:01 +00:00
Sign in to join this conversation.
No reviewers
dkirillov
alexvanin
No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
v0.33.0
No project
No assignees
mbiryukova
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-http-gw#220
No description provided.
Delete branch "mbiryukova/frostfs-http-gw:feature/cors_container"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?