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

feat: add t0124 with ipns fixtures #37

Merged
merged 21 commits into from May 18, 2023
Merged

Conversation

laurentsenta
Copy link
Contributor

@laurentsenta laurentsenta commented Apr 10, 2023

contributes to #10
requires ipfs/kubo#9667 (allow-offline for ipns fixtures)

🎶 the regular test is failing; this is expected: we need the ipfs routing put --allow-offline option that is not released yet. The second workflow "Test Master" is passing.

Failing test to make sure tests are running - we removed the ipns-record on purpose.

  • implement ipns-record fixture loader & exporter
  • implement ipns checker for response
  • t0116-gateway-cache.sh
  • t0124-gateway-ipns-record.sh
  • fix fixture collision names
  • add a test runner on kubo's master

Todo

  • kubo: make it possible to import records even in offline mode
  • kubo: make it possible to call the api in offline mode
    • If we try to get the record with http://127.0.0.1:8080/ipns/k51qzi5uqu5dhjjqhpcuvdnskq4mz84a2xg1rpqzi6s5460q2612egkfjsk42x?format=ipns-record
      the call timeouts because kubo take its time to load the record from the network.
    • If we try to start the daemon in offline mode we get "this action must be run in online mode, try running 'ipfs daemon' first".
  • Discuss code reuse with the team, we have to copy and paste a lot of code (see kubo_name)
  • validate the API:
    • how we distribute ipns-records
    • how we export as fixture & use them in tests

Follow-up

  • t0114-gateway-subdomains.sh
  • t0123-gateway-json-cbor.sh

@laurentsenta laurentsenta marked this pull request as draft April 10, 2023 13:32
@laurentsenta laurentsenta changed the base branch from main to feat/add-t0109-with-dnslink April 10, 2023 13:34
@laurentsenta
Copy link
Contributor Author

(temporary target on t0109 because this builts on top of dnslink)

@laurentsenta
Copy link
Contributor Author

(note the test will not pass until ipfs/kubo#9667 is released)

tooling/ipns/kubo_name.go Outdated Show resolved Hide resolved
@BigLep
Copy link

BigLep commented May 1, 2023

@laurentsenta : please request review from @hacdias when this is out of draft.

@BigLep
Copy link

BigLep commented May 5, 2023

@laurentsenta : I saw the comment here about waiting on review. Who are you waiting on review from? This may not be clear given that this is in draft.

tooling/ipns/ipns.go Outdated Show resolved Hide resolved
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

Very nice overall. I also wanted to ask if there is any reason why we're keeping the bash code from the old sharness tests as comments. I would personally like to get them cleanup up, but I just want to understand the reasoning. Is it to make it clear which tests are which during the transitioning period from sharness to the gateway conformance suite?

Also, CI ain't happy. Maybe I just reviewed before time, but I saw @BigLep's comment so I just went for it 😃

Base automatically changed from feat/add-t0109-with-dnslink to main May 9, 2023 14:57
@laurentsenta
Copy link
Contributor Author

laurentsenta commented May 9, 2023

Thanks for the review @hacdias & for the note @BigLep,

We had a sync this morning with @galargh and @hacdias we reviewed and agreed on the API.
With #34 merged I will move this one out of draft state next.

@hacdias
Copy link
Member

hacdias commented May 10, 2023

@laurentsenta this is how I would update to use the new boxo/ipns.ValidateWithPeerID. Sadly, I can't commit here, so here it is:

diff --git a/go.mod b/go.mod
index 1d7b8f0..47500e3 100644
--- a/go.mod
+++ b/go.mod
@@ -3,7 +3,7 @@ module github.com/ipfs/gateway-conformance
 go 1.20
 
 require (
-	github.com/ipfs/boxo v0.8.1
+	github.com/ipfs/boxo v0.8.2-0.20230510114019-33e3f0cd052b
 	github.com/ipfs/go-cid v0.4.1
 	github.com/ipld/go-ipld-prime v0.20.0
 	github.com/libp2p/go-libp2p v0.26.3
diff --git a/go.sum b/go.sum
index bfa7c29..cf7a061 100644
--- a/go.sum
+++ b/go.sum
@@ -47,8 +47,8 @@ github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uG
 github.com/huin/goupnp v1.0.3 h1:N8No57ls+MnjlB+JPiCVSOyy/ot7MJTqlo7rn+NYSqQ=
 github.com/ipfs/bbloom v0.0.4 h1:Gi+8EGJ2y5qiD5FbsbpX/TMNcJw8gSqr7eyjHa4Fhvs=
 github.com/ipfs/bbloom v0.0.4/go.mod h1:cS9YprKXpoZ9lT0n/Mw/a6/aFV6DTjTLYHeA+gyqMG0=
-github.com/ipfs/boxo v0.8.1 h1:3DkKBCK+3rdEB5t77WDShUXXhktYwH99mkAsgajsKrU=
-github.com/ipfs/boxo v0.8.1/go.mod h1:xJ2hVb4La5WyD7GvKYE0lq2g1rmQZoCD2K4WNrV6aZI=
+github.com/ipfs/boxo v0.8.2-0.20230510114019-33e3f0cd052b h1:6EVpfwbBgwhfZOA19i55jOGokKOy+OaQAm1dg4RbXmc=
+github.com/ipfs/boxo v0.8.2-0.20230510114019-33e3f0cd052b/go.mod h1:Ej2r08Z4VIaFKqY08UXMNhwcLf6VekHhK8c+KqA1B9Y=
 github.com/ipfs/go-bitfield v1.1.0 h1:fh7FIo8bSwaJEh6DdTWbCeZ1eqOaOkKFI74SCnsWbGA=
 github.com/ipfs/go-bitfield v1.1.0/go.mod h1:paqf1wjq/D2BBmzfTVFlJQ9IlFOZpg422HL0HqsGWHU=
 github.com/ipfs/go-block-format v0.0.2/go.mod h1:AWR46JfpcObNfg3ok2JHDUfdiHRgWhJgCQF+KIgOPJY=
diff --git a/tooling/ipns/ipns_test.go b/tooling/ipns/ipns_test.go
index 4cced6f..b2c9dbb 100644
--- a/tooling/ipns/ipns_test.go
+++ b/tooling/ipns/ipns_test.go
@@ -42,9 +42,8 @@ func TestLoadIPNSRecord(t *testing.T) {
 	assert.Equal(t, ipns.Value(), "/ipfs/bafkreidfdrlkeq4m4xnxuyx6iae76fdm4wgl5d4xzsb77ixhyqwumhz244")
 	assert.True(t, ipns.Entry.Validity.After(YEAR_100))
 
-	verify, err := ipns.Verify()
-	assert.Nil(t, err)
-	assert.True(t, verify)
+	err = ipns.Valid()
+	assert.NoError(t, err)
 }
 
 func TestLoadTestRecord(t *testing.T) {
@@ -60,7 +59,6 @@ func TestLoadTestRecord(t *testing.T) {
 	assert.Equal(t, ipns.Value(), "/ipfs/bafyaaeykceeaeeqlnbswy3dpo5xxe3debimaw")
 	assert.True(t, ipns.Entry.Validity.After(YEAR_100))
 
-	verify, err := ipns.Verify()
-	assert.Nil(t, err)
-	assert.True(t, verify)
+	err = ipns.Valid()
+	assert.NoError(t, err)
 }
diff --git a/tooling/ipns/kubo_name.go b/tooling/ipns/kubo_name.go
index b97fc9c..0cf288e 100644
--- a/tooling/ipns/kubo_name.go
+++ b/tooling/ipns/kubo_name.go
@@ -7,7 +7,6 @@ package ipns
 import (
 	"bytes"
 	"encoding/json"
-	"fmt"
 	"time"
 
 	"github.com/gogo/protobuf/proto"
@@ -16,7 +15,6 @@ import (
 	"github.com/ipld/go-ipld-prime"
 	"github.com/ipld/go-ipld-prime/codec/dagcbor"
 	"github.com/ipld/go-ipld-prime/codec/dagjson"
-	ic "github.com/libp2p/go-libp2p/core/crypto"
 	"github.com/libp2p/go-libp2p/core/peer"
 	mbase "github.com/multiformats/go-multibase"
 )
@@ -24,15 +22,15 @@ import (
 // IpnsInspectEntry contains the deserialized values from an IPNS Entry:
 // https://github.com/ipfs/specs/blob/main/ipns/IPNS.md#record-serialization-format
 type IpnsInspectEntry struct {
-	Value        string                     `json:"value"`
+	Value        string                          `json:"value"`
 	ValidityType *ipns_pb.IpnsEntry_ValidityType `json:"validityType"`
-	Validity     *time.Time                 `json:"validity"`
-	Sequence     uint64                     `json:"sequence"`
-	TTL          *uint64                    `json:"ttl"`
-	PublicKey    string                     `json:"publicKey"`
-	SignatureV1  string                     `json:"signatureV1"`
-	SignatureV2  string                     `json:"signatureV2"`
-	Data         interface{}                `json:"data"`
+	Validity     *time.Time                      `json:"validity"`
+	Sequence     uint64                          `json:"sequence"`
+	TTL          *uint64                         `json:"ttl"`
+	PublicKey    string                          `json:"publicKey"`
+	SignatureV1  string                          `json:"signatureV1"`
+	SignatureV2  string                          `json:"signatureV2"`
+	Data         interface{}                     `json:"data"`
 }
 
 func unmarshalIPNSEntry(data []byte) (*ipns_pb.IpnsEntry, error) {
@@ -90,7 +88,6 @@ func unmarshalIPNSRecord(entry *ipns_pb.IpnsEntry) (*IpnsInspectEntry, error) {
 		result.Validity = &validity
 	}
 
-
 	return &result, nil
 }
 
@@ -99,49 +96,3 @@ type IpnsInspectValidation struct {
 	Reason    string
 	PublicKey peer.ID
 }
-
-func verify(key string, entry *ipns_pb.IpnsEntry) (*IpnsInspectValidation, error) {
-	id, err := peer.Decode(key)
-	if err != nil {
-		return nil, err
-	}
-
-	validation := &IpnsInspectValidation{
-		PublicKey: id,
-	}
-
-	pub, err := id.ExtractPublicKey()
-	if err != nil {
-		// Make sure it works with all those RSA that cannot be embedded into the
-		// Peer ID.
-		if len(entry.PubKey) > 0 {
-			pub, err = ic.UnmarshalPublicKey(entry.PubKey)
-			if err != nil {
-				return nil, err
-			}
-
-			// Verify the public key matches the name we are verifying.
-			entryID, err := peer.IDFromPublicKey(pub)
-
-			if err != nil {
-				return nil, err
-			}
-
-			if id != entryID {
-				return nil, fmt.Errorf("record public key does not match the verified name")
-			}
-		}
-	}
-	if err != nil {
-		return nil, err
-	}
-
-	err = ipns.Validate(pub, entry)
-	if err == nil {
-		validation.Valid = true
-	} else {
-		validation.Reason = err.Error()
-	}
-
-	return validation, nil
-}
diff --git a/tooling/ipns/record.go b/tooling/ipns/record.go
index 49be37a..817db29 100644
--- a/tooling/ipns/record.go
+++ b/tooling/ipns/record.go
@@ -1,7 +1,9 @@
 package ipns
 
 import (
+	"github.com/ipfs/boxo/ipns"
 	ipns_pb "github.com/ipfs/boxo/ipns/pb"
+	"github.com/libp2p/go-libp2p/core/peer"
 )
 
 type IpnsRecord struct {
@@ -36,12 +38,11 @@ func (i *IpnsRecord) Key() string {
 	return i.Entry.PublicKey
 }
 
-func (i *IpnsRecord) Verify() (bool, error) {
-	result, err := verify(i.Entry.PublicKey, i.Pb)
-
+func (i *IpnsRecord) Valid() error {
+	id, err := peer.Decode(i.Entry.PublicKey)
 	if err != nil {
-		return false, err
+		return err
 	}
 
-	return result.Valid, nil
+	return ipns.ValidateWithPeerID(id, i.Pb)
 }

I would also remove the IpnsInspectEntry and rework it somehow else. As I mentioned in the Boxo PR, that struct was made specifically for the ipns name inspect command and I don't think it's adequate to be used here.

@BigLep
Copy link

BigLep commented May 10, 2023

Sadly, I can't commit here, so here it is:

@hacdias or @laurentsenta : lets add the w3dt-stewards team as an owner of this repo please.

@laurentsenta
Copy link
Contributor Author

(team added in ipfs/github-mgmt#138 cc @BigLep @hacdias)

@laurentsenta laurentsenta requested a review from galargh May 11, 2023 13:12
@laurentsenta laurentsenta marked this pull request as ready for review May 11, 2023 13:15
@laurentsenta
Copy link
Contributor Author

laurentsenta commented May 11, 2023

Team, thanks for the feedbacks, and the patch @hacdias, the PR is complete and ready for review!

Note the Test won't pass until we release a new Kubo version, but Test Master is expected to pass.

Very nice overall. I also wanted to ask if there is any reason why we're keeping the bash code from the old sharness tests as comments. I would personally like to get them cleanup up, but I just want to understand the reasoning. Is it to make it clear which tests are which during the transitioning period from sharness to the gateway conformance suite?

Exactly, many tests are not complete because we're missing features (like ipns records fixtures), so keeping the comments helps keep track of what's done, what's in progress, and what's not started. When we're done porting the tests, we'll remove the comment and make it the v1.

do
key=$(basename -s .ipns-record "$record" | cut -d'_' -f1)
ipfs routing put --allow-offline "/ipns/$key" "$record"
done
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewers, note this is how we move ipns records fixtures around,
we use a pubkey_...ipns-record file in the fixtures folder.

func TestGatewayCacheWithIPNS(t *testing.T) {
fixture := car.MustOpenUnixfsCar("t0116/gateway-cache.car")
ipns := ipns.MustOpenIPNSRecordWithKey("t0116/k51qzi5uqu5dlxdsdu5fpuu7h69wu4ohp32iwm9pdt9nq3y5rpn3ln9j12zfhe.ipns-record")
ipnsId := ipns.Key()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

highlight: this is the API to use an ipns record

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

Some minor notes, but everything else seems 👍

tests/t0116_gateway_cache_test.go Outdated Show resolved Hide resolved
tests/t0124_gateway_ipns_record_test.go Show resolved Hide resolved
tooling/ipns/record.go Outdated Show resolved Hide resolved
laurentsenta and others added 2 commits May 12, 2023 13:49
Co-authored-by: Henrique Dias <hacdias@gmail.com>
.github/workflows/test-master.yml Outdated Show resolved Hide resolved
Comment on lines 14 to +17
- name: Setup Go
uses: actions/setup-go@v3
with:
go-version: 1.19.1
go-version: 1.20.4
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't actually use Go here right now, do we?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, but we will with the matrix!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch; probably a leftover from before the containerization 👍

Note I'll merge the two tests.yml, which means we'll need go because we want to call:

- run: go install github.com/ipfs/kubo/cmd/ipfs@${{ matrix.target }}

In place of

# only work with released binaries
- uses: ipfs/download-ipfs-distribution-action@v1

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably could get away with using the default from the machine for this but it's fine to leave it in.

tests/t0124_gateway_ipns_record_test.go Outdated Show resolved Hide resolved
t.Fatal(err)
}

check := IsIPNSKey("k51qzi5uqu5dgh7y9l90nqs6tvnzcm9erbt8fhzg3fu79p5qt9zb2izvfu51ki").IsValid()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here:

Suggested change
check := IsIPNSKey("k51qzi5uqu5dgh7y9l90nqs6tvnzcm9erbt8fhzg3fu79p5qt9zb2izvfu51ki").IsValid()
check := IsIPNSKey("k51qzi5uqu5dgh7y9l90nqs6tvnzcm9erbt8fhzg3fu79p5qt9zb2izvfu51ki")

Comment on lines 59 to 60
IsIPNSKey(ipnsKey).
IsValid().
Copy link
Contributor

Choose a reason for hiding this comment

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

And here:

Suggested change
IsIPNSKey(ipnsKey).
IsValid().
IsIPNSKey(ipnsKey)

tooling/ipns/ipns_test.go Show resolved Hide resolved
tooling/test/test.go Outdated Show resolved Hide resolved
@galargh
Copy link
Contributor

galargh commented May 15, 2023

Sadly, I can't commit here

Just an FYI, in a situation like this, you could create a fork and propose changes through a PR to the PR branch. And in case of smaller changes, you can always suggest them through a comment.

.github/workflows/test.yml Outdated Show resolved Hide resolved
Co-authored-by: Piotr Galar <piotr.galar@gmail.com>
Header("Cache-Control").Contains("public, max-age=3155760000"),
).
Body(
IsIPNSKey(ipnsKey).
Copy link
Member

Choose a reason for hiding this comment

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

nit: what we get in response body is not IPNS Key :) It is IPNS Record.

@laurentsenta mind renaming this helper to use IPNSKeyIPNSRecord?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks for the clarification!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
{
Name: "GET for /ipns/ unixfs dir listing succeeds",
Request: Request().
Path("ipns/%s/root2/root3/", ipnsKey),
Copy link
Member

@lidel lidel May 16, 2023

Choose a reason for hiding this comment

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

unrelated nit: every time we reference URL path, or IPFS content path, it should always start with /.

Looks weird otherwise, especially when we use Equals("/ipns/%s/root2/root3/" few lines below :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair, I'll create a follow-up task + PR to fix this!

for record in $records
do
key=$(basename -s .ipns-record "$record" | cut -d'_' -f1)
ipfs routing put --allow-offline "/ipns/$key" "$record"
Copy link
Member

@lidel lidel May 16, 2023

Choose a reason for hiding this comment

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

💭 (not a blocker, just a note for the future) when ipfs/specs#369 is figured out, we will be able to remove our reliance on ipfs routing put

@laurentsenta laurentsenta requested a review from lidel May 17, 2023 10:46
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

Headers(
Header("Content-Disposition").Contains("attachment;"),
Header("Content-Type").Contains("application/vnd.ipfs.ipns-record"),
Header("Cache-Control").Contains("public, max-age=3155760000"),
Copy link
Member

Choose a reason for hiding this comment

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

@laurentsenta all tests use the same timestamp, humans are lazy, which means some implementers will just hard-code Cache-Control and call it a day :P

Mind adding one more test that has record with slightly different TTL, and call it out in the test Name that Cache-Control max-age MUST be based on the TTL from the IPNS Record? (fine to do it in a separate PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, I created #51

@laurentsenta laurentsenta merged commit 93bc763 into main May 18, 2023
1 of 2 checks passed
@laurentsenta laurentsenta deleted the feat/add-t0124-with-ipns branch May 18, 2023 12:55
@laurentsenta laurentsenta mentioned this pull request Jun 26, 2023
2 tasks
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.

None yet

5 participants