Strict APE check for EC & fix sign EC part put requests #1451
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1451
Loading…
Reference in a new issue
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:fix/ec_ape_strict"
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?
2a3bc3a2c8
to196bb755c6
@ -123,3 +126,3 @@
}
}
header = c.fillHeaderWithECParent(ctx, prm, header)
header, err := c.fillHeaderWithECParent(ctx, prm, header)
It looks even too strict if you look at the line above:
WIP: Strict APE check for EC & fix sign EC part put requeststo Strict APE check for EC & fix sign EC part put requestsStrict APE check for EC & fix sign EC part put requeststo WIP: Strict APE check for EC & fix sign EC part put requests@ -61,2 +63,4 @@
}
restoreTokens := e.CommonPrm.ForgetTokens()
defer restoreTokens()
Restore tokens as in case of complex object use session token to store linking object (as it was before)
WIP: Strict APE check for EC & fix sign EC part put requeststo Strict APE check for EC & fix sign EC part put requestsWhat happens now if we PUT EC object via non-container node?
196bb755c6
to5c5dfaaf35
New commits pushed, approval review dismissed automatically according to repository settings
It will fail. Fixed.
Also found a bug in traverser:
Create traverser for container without object ID. Container node cache is empty, so netmap requested, stored in cache and returned to caller:
cn, err := b.containerCache.ContainerNodes(nm, cnr, p)
BuildPlacement
returns original slice (from cache):return cnrNodes, nil
Traverser
changest.vectors
which links to cache's slice:t.vectors[0] = t.vectors[0][count:]
Create one more traverser for the same container but with object id. Container node cache returns empty slice, so traverser returns no placement nodes.
Also added check if all EC parts were processed by writer.
5c5dfaaf35
to37645a18b4
37645a18b4
to5ea53276c5
Strict APE check for EC & fix sign EC part put requeststo WIP: Strict APE check for EC & fix sign EC part put requests5ea53276c5
to94c2b36ae0
94c2b36ae0
to9014cba4cd
WIP: Strict APE check for EC & fix sign EC part put requeststo Strict APE check for EC & fix sign EC part put requests9014cba4cd
to84bab017b8
84bab017b8
to177843aeb0
@ -695,6 +695,7 @@ func TestPutECChunk(t *testing.T) {
nm := &netmapStub{
currentEpoch: 100,
netmaps: map[uint64]*netmapSDK.NetMap{
99: netmap,
Why this change is necessary?
previously, checking that the node is a container node ignored the error that netmap is not defined for previous epoch
@ -156,3 +162,3 @@
}
func (c *checkerImpl) fillHeaderWithECParent(ctx context.Context, prm Prm, header *objectV2.Header) *objectV2.Header {
func (c *checkerImpl) fillHeaderWithECParent(ctx context.Context, prm Prm, header *objectV2.Header) (*objectV2.Header, error) {
I do not completely understand this function.
In what circumstances do we return
header
and not parent header and, more importantly, why?If ape skips checks for in-container requests, why do we call this function at all?
Oh, mistake. It must be:
Then container node tries to collect headers if request is done be owner or other user.
Again, if APE skips check, why do we call this function?
It doesn't seem we signify the "skip" part with our return value in any way.
Removed pt.2 as it is checker before this function call.
177843aeb0
to5afbb6202b
New commits pushed, approval review dismissed automatically according to repository settings
New commits pushed, approval review dismissed automatically according to repository settings
@ -63,0 +69,4 @@
restoreTokens := e.CommonPrm.ForgetTokens()
defer restoreTokens()
// As request executed on container node, so sign request with container key.
e.remoteRequestSignKey, err = e.Config.KeyStorage.GetKey(nil)
Let's consider a situation:
ec-container: s01, s03, s04
. We're sending a request signed bywallet.json
. We pass through APE-check fors01
, resign the request bys01
key and sendwriteECPart
tos03, s04
.Imagine that at this time interval (30sec before) a local override has been added to
s03
ors04
to restrict putting object to this container - the local override is totally ignored due to this actor role change.I don't consider a situation when an ape chain is present within
policy
contract because it'd reject the request before this point and that's OKI think it is ok as frostfs-storage is not strongly consistent system.
Also it depends on local override: if local override restricts put objects with owner, it will still work.
It seems to be so
We won't get to the check at all: look at these lines
Anyway, currently I am not requesting to change this. We just need to keep this in mind
Oh, right.
But as we discussed with @fyrchik we should try to check user's request once (on first container node now) to reduce unnecessary operations count.
OK. I'll keep this comment open to not forget about this point in the future if we refer back to this PR
5afbb6202b
to78858c629e
@ -69,0 +71,4 @@
func (c *ContainerNodesCache) cloneResult(nodes [][]netmapSDK.NodeInfo) [][]netmapSDK.NodeInfo {
result := make([][]netmapSDK.NodeInfo, len(nodes))
for repIdx := range nodes {
result[repIdx] = make([]netmapSDK.NodeInfo, 0, len(nodes[repIdx]))
slices.Clone
?fixed
78858c629e
to765c81ad2a
765c81ad2a
tob4adf43557