From 717d16345cf9b93ef1652c34a70e6109f8f9c97e Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 8 Jul 2021 10:07:39 +0100 Subject: [PATCH 1/6] Improve error handling and close files post-tarring --- cmd/dendrite-upgrade-tests/main.go | 1 + cmd/dendrite-upgrade-tests/tar.go | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/cmd/dendrite-upgrade-tests/main.go b/cmd/dendrite-upgrade-tests/main.go index 5b5cf9d7e..46e33c1cd 100644 --- a/cmd/dendrite-upgrade-tests/main.go +++ b/cmd/dendrite-upgrade-tests/main.go @@ -104,6 +104,7 @@ func downloadArchive(cli *http.Client, tmpDir, archiveURL string, dockerfile []b if resp.StatusCode != 200 { return nil, fmt.Errorf("got HTTP %d", resp.StatusCode) } + _ = os.RemoveAll(tmpDir) if err = os.Mkdir(tmpDir, os.ModePerm); err != nil { return nil, fmt.Errorf("failed to make temporary directory: %s", err) } diff --git a/cmd/dendrite-upgrade-tests/tar.go b/cmd/dendrite-upgrade-tests/tar.go index 9eadbb3de..8c6402a84 100644 --- a/cmd/dendrite-upgrade-tests/tar.go +++ b/cmd/dendrite-upgrade-tests/tar.go @@ -17,7 +17,7 @@ func compress(src string, buf io.Writer) error { tw := tar.NewWriter(zr) // walk through every file in the folder - _ = filepath.Walk(src, func(file string, fi os.FileInfo, e error) error { + err := filepath.Walk(src, func(file string, fi os.FileInfo, e error) error { // generate tar header header, err := tar.FileInfoHeader(fi, file) if err != nil { @@ -40,9 +40,15 @@ func compress(src string, buf io.Writer) error { if _, err := io.Copy(tw, data); err != nil { return err } + if err = data.Close(); err != nil { + return err + } } return nil }) + if err != nil { + return err + } // produce tar if err := tw.Close(); err != nil { From 3fb5ee7e1cc9545b492abefa62980f96ee58364e Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 8 Jul 2021 10:17:13 +0100 Subject: [PATCH 2/6] linting --- cmd/dendrite-upgrade-tests/tar.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/dendrite-upgrade-tests/tar.go b/cmd/dendrite-upgrade-tests/tar.go index 8c6402a84..fd45424db 100644 --- a/cmd/dendrite-upgrade-tests/tar.go +++ b/cmd/dendrite-upgrade-tests/tar.go @@ -37,7 +37,7 @@ func compress(src string, buf io.Writer) error { if err != nil { return err } - if _, err := io.Copy(tw, data); err != nil { + if _, err = io.Copy(tw, data); err != nil { return err } if err = data.Close(); err != nil { From ef331c52af7606062d9d7584e6c004cf363d01eb Mon Sep 17 00:00:00 2001 From: kegsay Date: Thu, 8 Jul 2021 12:28:04 +0100 Subject: [PATCH 3/6] dendrite-upgrade-test: tweaks to get it to run under CI in docker (#1905) * dendrite-upgrade-test: tweaks to get it to run under CI in docker * Linting --- cmd/dendrite-upgrade-tests/main.go | 49 ++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/cmd/dendrite-upgrade-tests/main.go b/cmd/dendrite-upgrade-tests/main.go index 46e33c1cd..a6cc2d3f9 100644 --- a/cmd/dendrite-upgrade-tests/main.go +++ b/cmd/dendrite-upgrade-tests/main.go @@ -35,9 +35,13 @@ var ( flagFrom = flag.String("from", "HEAD-1", "The version to start from e.g '0.3.1'. If 'HEAD-N' then starts N versions behind HEAD.") flagTo = flag.String("to", "HEAD", "The version to end on e.g '0.3.3'.") flagBuildConcurrency = flag.Int("build-concurrency", runtime.NumCPU(), "The amount of build concurrency when building images") + flagHead = flag.String("head", "", "Location to a dendrite repository to treat as HEAD instead of Github") + flagDockerHost = flag.String("docker-host", "localhost", "The hostname of the docker client. 'localhost' if running locally, 'host.docker.internal' if running in Docker.") alphaNumerics = regexp.MustCompile("[^a-zA-Z0-9]+") ) +const HEAD = "HEAD" + // Embed the Dockerfile to use when building dendrite versions. // We cannot use the dockerfile associated with the repo with each version sadly due to changes in // Docker versions. Specifically, earlier Dendrite versions are incompatible with newer Docker clients @@ -135,15 +139,36 @@ func downloadArchive(cli *http.Client, tmpDir, archiveURL string, dockerfile []b // buildDendrite builds Dendrite on the branchOrTagName given. Returns the image ID or an error func buildDendrite(httpClient *http.Client, dockerClient *client.Client, tmpDir, branchOrTagName string) (string, error) { - log.Printf("%s: Downloading version %s to %s\n", branchOrTagName, branchOrTagName, tmpDir) - // pull an archive, this contains a top-level directory which screws with the build context - // which we need to fix up post download - u := fmt.Sprintf("https://github.com/matrix-org/dendrite/archive/%s.tar.gz", branchOrTagName) - tarball, err := downloadArchive(httpClient, tmpDir, u, []byte(Dockerfile)) - if err != nil { - return "", fmt.Errorf("failed to download archive %s: %w", u, err) + var tarball *bytes.Buffer + var err error + // If a custom HEAD location is given, use that, else pull from github. Mostly useful for CI + // where we want to use the working directory. + if branchOrTagName == HEAD && *flagHead != "" { + log.Printf("%s: Using %s as HEAD", branchOrTagName, *flagHead) + // add top level Dockerfile + err = ioutil.WriteFile(path.Join(*flagHead, "Dockerfile"), []byte(Dockerfile), os.ModePerm) + if err != nil { + return "", fmt.Errorf("Custom HEAD: failed to inject /Dockerfile: %w", err) + } + // now tarball it + var buffer bytes.Buffer + err = compress(*flagHead, &buffer) + if err != nil { + return "", fmt.Errorf("failed to tarball custom HEAD %s : %s", *flagHead, err) + } + tarball = &buffer + } else { + log.Printf("%s: Downloading version %s to %s\n", branchOrTagName, branchOrTagName, tmpDir) + // pull an archive, this contains a top-level directory which screws with the build context + // which we need to fix up post download + u := fmt.Sprintf("https://github.com/matrix-org/dendrite/archive/%s.tar.gz", branchOrTagName) + tarball, err = downloadArchive(httpClient, tmpDir, u, []byte(Dockerfile)) + if err != nil { + return "", fmt.Errorf("failed to download archive %s: %w", u, err) + } + log.Printf("%s: %s => %d bytes\n", branchOrTagName, u, tarball.Len()) } - log.Printf("%s: %s => %d bytes\n", branchOrTagName, u, tarball.Len()) + log.Printf("%s: Building version %s\n", branchOrTagName, branchOrTagName) res, err := dockerClient.ImageBuild(context.Background(), tarball, types.ImageBuildOptions{ Tags: []string{"dendrite-upgrade"}, @@ -235,7 +260,7 @@ func calculateVersions(cli *http.Client, from, to string) []string { semvers = semvers[i:] } } - if to != "" && to != "HEAD" { + if to != "" && to != HEAD { toVer, err := semver.NewVersion(to) if err != nil { log.Fatalf("invalid --to: %s", err) @@ -253,8 +278,8 @@ func calculateVersions(cli *http.Client, from, to string) []string { for _, sv := range semvers { versions = append(versions, sv.Original()) } - if to == "HEAD" { - versions = append(versions, "HEAD") + if to == HEAD { + versions = append(versions, HEAD) } return versions } @@ -328,7 +353,7 @@ func runImage(dockerClient *client.Client, volumeName, version, imageID string) if !ok { return "", "", fmt.Errorf("port 8008 not exposed - exposed ports: %v", inspect.NetworkSettings.Ports) } - baseURL := fmt.Sprintf("http://localhost:%s", csapiPortInfo[0].HostPort) + baseURL := fmt.Sprintf("http://%s:%s", *flagDockerHost, csapiPortInfo[0].HostPort) versionsURL := fmt.Sprintf("%s/_matrix/client/versions", baseURL) // hit /versions to check it is up var lastErr error From 70e4bbda3b0b3eebc8a1c3f8e1bb1b1a85988070 Mon Sep 17 00:00:00 2001 From: kegsay Date: Thu, 8 Jul 2021 13:13:27 +0100 Subject: [PATCH 4/6] Only log filename and not entire path (#1906) --- internal/log.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/log.go b/internal/log.go index 0f374bd4a..d2b233c5b 100644 --- a/internal/log.go +++ b/internal/log.go @@ -73,9 +73,9 @@ func callerPrettyfier(f *runtime.Frame) (string, string) { // Append a newline + tab to it to move the actual log content to its own line funcname += "\n\t" - // Surround the filepath in brackets and append line number so IDEs can quickly - // navigate - filename := fmt.Sprintf(" [%s:%d]", f.File, f.Line) + // Use a shortened file path which just has the filename to avoid having lots of redundant + // directories which contribute significantly to overall log sizes! + filename := fmt.Sprintf(" [%s:%d]", path.Base(f.File), f.Line) return funcname, filename } From 816e1a402bdc69a681177c1994e07a2a907315b8 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Thu, 8 Jul 2021 14:54:03 +0100 Subject: [PATCH 5/6] Fix bug when rejecting invites (#1907) * Fix rejecting invites maybe * Remove comment that is no longer correct * Review comment on performFederatedRejectInvite --- roomserver/internal/perform/perform_leave.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/roomserver/internal/perform/perform_leave.go b/roomserver/internal/perform/perform_leave.go index 9d7c0816d..4d10dea67 100644 --- a/roomserver/internal/perform/perform_leave.go +++ b/roomserver/internal/perform/perform_leave.go @@ -64,7 +64,14 @@ func (r *Leaver) performLeaveRoomByID( // that. isInvitePending, senderUser, eventID, err := helpers.IsInvitePending(ctx, r.DB, req.RoomID, req.UserID) if err == nil && isInvitePending { - return r.performRejectInvite(ctx, req, res, senderUser, eventID) + var host gomatrixserverlib.ServerName + _, host, err = gomatrixserverlib.SplitID('@', senderUser) + if err != nil { + return nil, fmt.Errorf("Sender %q is invalid", senderUser) + } + if host != r.Cfg.Matrix.ServerName { + return r.performFederatedRejectInvite(ctx, req, res, senderUser, eventID) + } } // There's no invite pending, so first of all we want to find out @@ -94,9 +101,7 @@ func (r *Leaver) performLeaveRoomByID( if err != nil { return nil, fmt.Errorf("Error getting membership: %w", err) } - if membership != gomatrixserverlib.Join { - // TODO: should be able to handle "invite" in this case too, if - // it's a case of kicking or banning or such + if membership != gomatrixserverlib.Join && membership != gomatrixserverlib.Invite { return nil, fmt.Errorf("User %q is not joined to the room (membership is %q)", req.UserID, membership) } @@ -147,7 +152,7 @@ func (r *Leaver) performLeaveRoomByID( return nil, nil } -func (r *Leaver) performRejectInvite( +func (r *Leaver) performFederatedRejectInvite( ctx context.Context, req *api.PerformLeaveRequest, res *api.PerformLeaveResponse, // nolint:unparam From 3e50bac9441ae43387fee510d5838039bc07e0bb Mon Sep 17 00:00:00 2001 From: kegsay Date: Fri, 9 Jul 2021 10:49:49 +0100 Subject: [PATCH 6/6] bugfix: order the state blocks so recreating state snapshots works correctly (#1908) * Logging * Revert "Logging" This reverts commit 23ce334182d8a70f8e0381786f9dea77a9b91ed8. * bugfix: order the state blocks so recreating state snapshots works correctly --- roomserver/storage/postgres/state_block_table.go | 2 +- roomserver/storage/sqlite3/state_block_table.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/roomserver/storage/postgres/state_block_table.go b/roomserver/storage/postgres/state_block_table.go index 4523d18bb..7ae987d6d 100644 --- a/roomserver/storage/postgres/state_block_table.go +++ b/roomserver/storage/postgres/state_block_table.go @@ -64,7 +64,7 @@ const insertStateDataSQL = "" + const bulkSelectStateBlockEntriesSQL = "" + "SELECT state_block_nid, event_nids" + - " FROM roomserver_state_block WHERE state_block_nid = ANY($1)" + " FROM roomserver_state_block WHERE state_block_nid = ANY($1) ORDER BY state_block_nid ASC" type stateBlockStatements struct { insertStateDataStmt *sql.Stmt diff --git a/roomserver/storage/sqlite3/state_block_table.go b/roomserver/storage/sqlite3/state_block_table.go index cfb2a49e5..5cb21e91c 100644 --- a/roomserver/storage/sqlite3/state_block_table.go +++ b/roomserver/storage/sqlite3/state_block_table.go @@ -57,7 +57,7 @@ const insertStateDataSQL = ` const bulkSelectStateBlockEntriesSQL = "" + "SELECT state_block_nid, event_nids" + - " FROM roomserver_state_block WHERE state_block_nid IN ($1)" + " FROM roomserver_state_block WHERE state_block_nid IN ($1) ORDER BY state_block_nid ASC" type stateBlockStatements struct { db *sql.DB