Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

api,client,http,main,prod,storage: Add query param for publisher and solve flooding in recovery #2202

Merged
merged 19 commits into from
Jun 16, 2020
Merged

api,client,http,main,prod,storage: Add query param for publisher and solve flooding in recovery #2202

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
540187d
api/client,api:http,cmd/swarm,prod,storage: added flag for publisher …
santicomp2014 Jun 11, 2020
0463f7a
storage: changed recovery only when publisher is set
santicomp2014 Jun 11, 2020
0e8b18e
prod,netstore: added logs for flood test
santicomp2014 Jun 11, 2020
ca488fd
storage: remove publisher after recovery
santicomp2014 Jun 11, 2020
f5236ec
storage: removed test publisher removal
santicomp2014 Jun 11, 2020
754739b
prod: added log for publisher in getPinners
santicomp2014 Jun 11, 2020
0787628
api,api/client,apit/http,cmd/swarm,cmd/swarm-smoke: removed publisher…
santicomp2014 Jun 11, 2020
f1b0fbf
cmd/swarm: remove unused log in download path
santicomp2014 Jun 12, 2020
e25597e
api/client,api/http,cmd/swarm: added publisher to bzz-list for recove…
santicomp2014 Jun 12, 2020
6a9a30a
api/client,api/http,cmd/swarm,prod,storage: removed unused comments a…
santicomp2014 Jun 12, 2020
d5d3de9
Merge branch 'global-pinning' into global-pinning-fix-request-flooding
santicomp2014 Jun 12, 2020
19edb39
storage: changed comments in netstore for clarity in global pinning
santicomp2014 Jun 12, 2020
aecc382
api/client,api/http,cmd/swarm,storage: changed publisher query param …
santicomp2014 Jun 15, 2020
41cfbe5
api/client: encoded publisher in query param
santicomp2014 Jun 15, 2020
688122a
api/http: added isRecoveryAttempt to List, storage changed recoveryCa…
santicomp2014 Jun 15, 2020
79f3867
api: removed Manifest not found from readManifest, global pinning wil…
santicomp2014 Jun 15, 2020
5e57f81
api/http,storage: changed StatusRecoveryAttempt to 420, refactored Er…
santicomp2014 Jun 16, 2020
3494897
api/http: added isRecoveryAttemptError for HandleGetFile
santicomp2014 Jun 16, 2020
6cc23bf
api/http: changed StatusRecoveryAttempt to StatusEnhanceYourCalm
santicomp2014 Jun 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions api/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,10 @@ func (c *Client) DownloadDirectory(hash, path, destDir, credentials, publisher s
}

uri := c.Gateway + "/bzz:/" + hash + "/" + path
if publisher != "" {
uri += "?publisher=" + publisher
}
req, err := http.NewRequest("GET", uri, nil)
values := req.URL.Query()
Copy link
Member

Choose a reason for hiding this comment

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

why not just

 req.URL.Query().Add("publisher", publisher)

Copy link
Contributor Author

@santicomp2014 santicomp2014 Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I used req.URL.Query().Add("publisher", publisher) and the query param was not reaching the other side.
I looked at other places of the code using Add/Set and it was encoding after adding it.

After changing it to .Encode() it works correctly.
@mortelli saw the same thing

values.Add("publisher", publisher)
mortelli marked this conversation as resolved.
Show resolved Hide resolved
req.URL.RawQuery = values.Encode()
if err != nil {
return err
}
Expand Down Expand Up @@ -315,10 +315,10 @@ func (c *Client) DownloadFile(hash, path, dest, credentials, publisher string) e
}

uri := c.Gateway + "/bzz:/" + hash + "/" + path
if publisher != "" {
uri += "?publisher=" + publisher
}
req, err := http.NewRequest("GET", uri, nil)
values := req.URL.Query()
values.Add("publisher", publisher)
mortelli marked this conversation as resolved.
Show resolved Hide resolved
req.URL.RawQuery = values.Encode()
if err != nil {
return err
}
Expand Down Expand Up @@ -418,10 +418,10 @@ func (c *Client) DownloadManifest(hash string) (*api.Manifest, bool, error) {
// where entries ending with "/" are common prefixes.
func (c *Client) List(hash, prefix, credentials, publisher string) (*api.ManifestList, error) {
uri := c.Gateway + "/bzz-list:/" + hash + "/" + prefix
if publisher != "" {
uri = uri + "?publisher=" + publisher
}
req, err := http.NewRequest(http.MethodGet, uri, nil)
values := req.URL.Query()
values.Add("publisher", publisher)
mortelli marked this conversation as resolved.
Show resolved Hide resolved
req.URL.RawQuery = values.Encode()
if err != nil {
return nil, err
}
Expand Down
60 changes: 38 additions & 22 deletions api/http/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"io"
"io/ioutil"
Expand Down Expand Up @@ -52,28 +53,29 @@ import (
)

var (
postRawCount = metrics.NewRegisteredCounter("api/http/post/raw/count", nil)
postRawFail = metrics.NewRegisteredCounter("api/http/post/raw/fail", nil)
postFilesCount = metrics.NewRegisteredCounter("api/http/post/files/count", nil)
postFilesFail = metrics.NewRegisteredCounter("api/http/post/files/fail", nil)
deleteCount = metrics.NewRegisteredCounter("api/http/delete/count", nil)
deleteFail = metrics.NewRegisteredCounter("api/http/delete/fail", nil)
getCount = metrics.NewRegisteredCounter("api/http/get/count", nil)
getFail = metrics.NewRegisteredCounter("api/http/get/fail", nil)
getFileCount = metrics.NewRegisteredCounter("api/http/get/file/count", nil)
getFileNotFound = metrics.NewRegisteredCounter("api/http/get/file/notfound", nil)
getFileFail = metrics.NewRegisteredCounter("api/http/get/file/fail", nil)
getListCount = metrics.NewRegisteredCounter("api/http/get/list/count", nil)
getListFail = metrics.NewRegisteredCounter("api/http/get/list/fail", nil)
getTagCount = metrics.NewRegisteredCounter("api/http/get/tag/count", nil)
getTagNotFound = metrics.NewRegisteredCounter("api/http/get/tag/notfound", nil)
getTagFail = metrics.NewRegisteredCounter("api/http/get/tag/fail", nil)
getPinCount = metrics.NewRegisteredCounter("api/http/get/pin/count", nil)
getPinFail = metrics.NewRegisteredCounter("api/http/get/pin/fail", nil)
postPinCount = metrics.NewRegisteredCounter("api/http/post/pin/count", nil)
postPinFail = metrics.NewRegisteredCounter("api/http/post/pin/fail", nil)
deletePinCount = metrics.NewRegisteredCounter("api/http/delete/pin/count", nil)
deletePinFail = metrics.NewRegisteredCounter("api/http/delete/pin/fail", nil)
postRawCount = metrics.NewRegisteredCounter("api/http/post/raw/count", nil)
postRawFail = metrics.NewRegisteredCounter("api/http/post/raw/fail", nil)
postFilesCount = metrics.NewRegisteredCounter("api/http/post/files/count", nil)
postFilesFail = metrics.NewRegisteredCounter("api/http/post/files/fail", nil)
deleteCount = metrics.NewRegisteredCounter("api/http/delete/count", nil)
deleteFail = metrics.NewRegisteredCounter("api/http/delete/fail", nil)
getCount = metrics.NewRegisteredCounter("api/http/get/count", nil)
getFail = metrics.NewRegisteredCounter("api/http/get/fail", nil)
getFileCount = metrics.NewRegisteredCounter("api/http/get/file/count", nil)
getFileNotFound = metrics.NewRegisteredCounter("api/http/get/file/notfound", nil)
getFileFail = metrics.NewRegisteredCounter("api/http/get/file/fail", nil)
getListCount = metrics.NewRegisteredCounter("api/http/get/list/count", nil)
getListFail = metrics.NewRegisteredCounter("api/http/get/list/fail", nil)
getTagCount = metrics.NewRegisteredCounter("api/http/get/tag/count", nil)
getTagNotFound = metrics.NewRegisteredCounter("api/http/get/tag/notfound", nil)
getTagFail = metrics.NewRegisteredCounter("api/http/get/tag/fail", nil)
getPinCount = metrics.NewRegisteredCounter("api/http/get/pin/count", nil)
getPinFail = metrics.NewRegisteredCounter("api/http/get/pin/fail", nil)
postPinCount = metrics.NewRegisteredCounter("api/http/post/pin/count", nil)
postPinFail = metrics.NewRegisteredCounter("api/http/post/pin/fail", nil)
deletePinCount = metrics.NewRegisteredCounter("api/http/delete/pin/count", nil)
deletePinFail = metrics.NewRegisteredCounter("api/http/delete/pin/fail", nil)
errRecoveryAttempt = errors.New("recovery was initiated")
Copy link
Member

Choose a reason for hiding this comment

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

define this in netstore once

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 was worried about circular dependency, moved it.
Thanks

mortelli marked this conversation as resolved.
Show resolved Hide resolved
)

const (
Expand Down Expand Up @@ -252,6 +254,11 @@ func (s *Server) HandleBzzGet(w http.ResponseWriter, r *http.Request) {
respondError(w, r, err.Error(), http.StatusUnauthorized)
return
}
if isRecoveryAttemptError(err) {
w.Header().Set("WWW-Authenticate", fmt.Sprintf("Basic realm=%q", uri.Address().String()))
respondError(w, r, err.Error(), http.StatusPaymentRequired)
mortelli marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

no i meant 420 not 402, you can just construct the response as:

respondError(w, r, err.Error(), 420)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I was confused it did not make sense.
Changed

return
}
respondError(w, r, fmt.Sprintf("Had an error building the tarball: %v", err), http.StatusInternalServerError)
return
}
Expand Down Expand Up @@ -868,6 +875,11 @@ func (s *Server) HandleGetList(w http.ResponseWriter, r *http.Request) {
respondError(w, r, err.Error(), http.StatusUnauthorized)
return
}
if isRecoveryAttemptError(err) {
w.Header().Set("WWW-Authenticate", fmt.Sprintf("Basic realm=%q", uri.Address().String()))
mortelli marked this conversation as resolved.
Show resolved Hide resolved
respondError(w, r, err.Error(), http.StatusPaymentRequired)
mortelli marked this conversation as resolved.
Show resolved Hide resolved
return
}
respondError(w, r, err.Error(), http.StatusInternalServerError)
return
}
Expand Down Expand Up @@ -1177,3 +1189,7 @@ func (lrw *loggingResponseWriter) WriteHeader(code int) {
func isDecryptError(err error) bool {
return strings.Contains(err.Error(), api.ErrDecrypt.Error())
}

func isRecoveryAttemptError(err error) bool {
return strings.Contains(err.Error(), errRecoveryAttempt.Error())
}
1 change: 0 additions & 1 deletion api/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,6 @@ func readManifest(mr storage.LazySectionReader, addr storage.Address, fileStore
if err != nil { // size == 0
// can't determine size means we don't have the root chunk
log.Trace("manifest not found", "addr", addr)
err = fmt.Errorf("Manifest not Found")
Copy link
Contributor

@mortelli mortelli Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

leaving a comment here mainly for @zelig:

the Manifest not Found error created here was swallowing up the 420 that was trying to be returned.

in any case, i believe without line 246 the returned error will be 500 or something of the sort (@santicomp2014 can confirm/refute this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly, the error code becomes Manifest not found and the error code is 500.
The error manifest not found is already being thrown in the error.

return
}
if size > manifestSizeLimit {
Expand Down
2 changes: 1 addition & 1 deletion cmd/swarm/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,6 @@ var (
}
SwarmPublisher = cli.StringFlag{
Name: "publisher",
Usage: "Manually specify content recovery publisher for a hash",
Usage: "Manually specify content recovery publisher",
}
)
5 changes: 2 additions & 3 deletions storage/netstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,8 @@ func (n *NetStore) Get(ctx context.Context, mode chunk.ModeGet, req *Request) (c
if err != nil {
if n.recoveryCallback != nil && publisher != "" {
log.Debug("content recovery callback triggered", "ref", ref.String())
n.recoveryCallback(ctx, ref)
time.Sleep(500 * time.Millisecond) // TODO: view what the ideal timeout is
return n.RemoteFetch(ctx, req, fi)
go n.recoveryCallback(ctx, ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

based on some tests, we think it might be necessary to call the recovery callback with go here.

do you see any issues with this @zelig?

return nil, errors.New("recovery was initiated")
}
return nil, err
}
Expand Down