From d18a85e19587628b05c4023e4dd970c21dee0122 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Tue, 3 Dec 2019 21:30:04 -0800 Subject: [PATCH 01/15] Update rules_closure to latest version Change-Id: Ifae9462251bea0b3fd683452f16572572d4a7cea --- WORKSPACE | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 9b17514fbb..a7c9e5eb10 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -32,9 +32,9 @@ http_archive( http_archive( name = "io_bazel_rules_closure", - sha256 = "0409f8bd2a8b6fd1db289cdc0acb394dafd69f60a86d0169bc6495e648e01587", - strip_prefix = "rules_closure-18f8acf24ae0d03a9c3ee872ff91dcfbf383d69e", - urls = ["https://github.com/bazelbuild/rules_closure/archive/18f8acf24ae0d03a9c3ee872ff91dcfbf383d69e.tar.gz"], + sha256 = "03c3b16f205085817fd89cfdcb2220a0138647ee7992be9cef291b069dd90301", + strip_prefix = "rules_closure-196a45f0ede2faec11dcc6c60fbc5e7471f4bd58", + urls = ["https://github.com/bazelbuild/rules_closure/archive/196a45f0ede2faec11dcc6c60fbc5e7471f4bd58.tar.gz"], ) # File is specific to Polymer and copied from the Closure Github -- should be From 0d2135640c4b2c2df750caab7fc20263cd9253d2 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 29 Nov 2019 02:01:56 +0100 Subject: [PATCH 02/15] Document how to backup Gerrit Bug: Issue 11440 Change-Id: Ia4514e28c82b97375e0f896d86654c9a9fcbf15d --- Documentation/backup.txt | 270 ++++++++++++++++++++++++++++++++++++++ Documentation/install.txt | 5 + 2 files changed, 275 insertions(+) create mode 100644 Documentation/backup.txt diff --git a/Documentation/backup.txt b/Documentation/backup.txt new file mode 100644 index 0000000000..7246ca766e --- /dev/null +++ b/Documentation/backup.txt @@ -0,0 +1,270 @@ += Gerrit Code Review - Backup + +A Gerrit Code Review site contains data that needs to be backed up regularly. +This document describes best practices for backing up review data. + +[#mand-backup] +== Data which must be backed up + +[#mand-backup-git] +Git repositories:: ++ +The bare Git repositories managed by Gerrit are typically stored in the +`${SITE}/git` directory. However, the locations can be customized in +`${site}/etc/gerrit.config`. They contain the history of the respective +projects, and since 2.15 if you are using _NoteDB_, and for 3.0 and newer, +also change and review metadata, user accounts and groups. ++ + +[#mand-backup-db] +SQL database:: ++ +Gerrit releases in the 2.x series store some data in the database you +have chosen when installing Gerrit. If you are using 2.16 and have +migrated to _NoteDB_ only the schema version is stored in the database. ++ +If you are using h2 you need to backup the `.db` files in the folder +`${SITE}/db`. ++ +For all other database types refer to their backup documentation. ++ +Gerrit release 3.0 and newer store all primary data in _NoteDB_ inside +the git repositories of the Gerrit site. Only the review flag marking in +the UI when you have reviewed a changed file is stored in a relational +database. If you are using h2 this database is named +`account_patch_reviews.h2.db`. + +[#optional-backup] +== Data optional to be backed up + +[#data-optional-backup-index] +Search index:: ++ +The _Lucene_ search index is stored in the `${SITE}/index` folder. +It can be recomputed from primary data in the git repositories but +reindexing may take a long time hence backing up the index makes sense +for production installations. ++ +If you have chosen to use _Elastic Search_ for indexing, +refer to its +link:https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-snapshots.html[backup documentation]. + +[#optional-backup-cache] +Caches:: ++ +Gerrit uses many caches which populate automatically. Some of the caches +are persisted in the directory `${SITE}/cache` to retain the cached data +across restarts. Since repopulating persistent caches takes time and server +resources it makes sense to include them in backups to avoid unnecessary +higher load and degraded performance when a Gerrit site has been restored +from backup and caches need to be repopulated. + +[#optional-backup-config] +Configuration:: ++ +Gerrit configuration files are located in the directory `${SITE}/etc` +and should be backed up or versioned in a git repository. The `etc` +directory also contains secrets which should be handled separately ++ +* `secure.config` contains passwords and `auth.registerEmailPrivateKey` +* public and private SSH host keys ++ +You may consider to use the +link:https://gerrit.googlesource.com/plugins/secure-config/[secure-config plugin] +to encrypt these secrets. + +[#optional-backup-plugin-data] +Plugin Data:: ++ +The `${SITE}/data/` directory is used by plugins storing data like e.g. +the delete-project and the replication plugin. + +[#optional-backup-libs] +Libraries:: ++ +The `${SITE}/lib/` directory contains libraries used as statically loaded +plugin or providing additional dependencies needed by Gerrit plugins. + +[#optional-backup-plugins] +Plugins:: ++ +The `${SITE}/plugins/` directory contains the installed Gerrit plugins. + +[#optional-backup-static] +Static Resources:: ++ +The `${SITE}/static/` directory contains static resources used to customize the +Gerrit UI and email templates. + +[#optional-backup-logs] +Logs:: ++ +The `${SITE}/logs/` directory contains Gerrit server log files. Logs can still +be written when the server is in read-only mode. + +[#cons-backup] +== Consistent backups + +There are several ways to ensure consistency when backing up primary data. + +[#cons-backup-snapshot] +=== Filesystem snapshots + +Gerrit 3.0 or newer:: ++ +* all primary data is stored in git +* Use a file system like lvm, zfs, btrfs or nfs supporting snapshots. +Create a snapshot and then archive the snapshot. + +Gerrit 2.x:: ++ +Gerrit 2.16 can use _NoteDB_ to store almost all this data which +simplifies creating backups since consistency between database and git +repositories is no longer critical. If you migrated to noteDB you can +follow the backup procedure for 3.0 and higher and additionally take +a backup of the database, which only contains the schema version, +hence consistency between git and database is no longer critical since +the schema version only changes during upgrade. If you didn't migrate +to noteDB then follow the backup procedure for older 2.x Gerrit versions. ++ +Older 2.x Gerrit versions store change meta data, review comments, votes, +accounts and group information in a SQL database. Creating consistent backups +where git repositories and the data stored in the database are backed up +consistently requires to turn the server read-only or to shut it down +while creating the backup since there is no integrated transaction handling +between git repositories and the SQL database. Also crons and currently +running cron jobs (e.g. repacking repositories) which affect the repositories +may need to be shut down. +Use a file system supporting snapshots to keep the period where the gerrit +server is read-only or down as short as possible. + +[#cons-backup-read-only] +=== Turn master read-only for backup + +Make the server read-only before taking the backup. This means read-access +is still available during backup, because only write operations have to be +stopped to ensure consistency. This can be implemented using the +link:https://gerrit.googlesource.com/plugins/readonly/[_readonly_] plugin. + +[#cons-backup-replicate] +=== Replicate data for backup + +Replicating the git repositories can backup the most critical repository data +but does not backup repository meta-data such as the project description +file, ref-logs, git configs, and alternate configs. + +Replicate all git repositories to another file system using +`git clone --mirror`, +or the +link:https://gerrit.googlesource.com/plugins/replication[replication plugin] +or the +link:https://gerrit.googlesource.com/plugins/pull-replication[pull-replication plugin]. +Best you use a filesystem supporting snapshots to create a backup archive +of such a replica. + +For 2.x Gerrit versions also set up a database slave for the data stored in the +SQL database. If you are using 2.16 and migrated to noteDB you may consider to +skip setting up a database slave, instead take a backup of the database which only +contains the current schema version in this case. +In addition you need to ensure that no write operations are in flight before you +take the replica offline. Otherwise the database backup might be inconsistent +with the backup of the git repositories. + +Do not skip backing up the replica, the replica alone IS NOT a backup. +Imagine someone deleted a project by mistake and this deletion got replicated. +Replication of repository deletions can be switched off using the +link:https://gerrit.googlesource.com/plugins/replication/+/refs/heads/master/src/main/resources/Documentation/config.md[server option] +`remote.NAME.replicateProjectDeletions`. + +If you are using Gerrit slaves to offload read traffic you can use one of these +slaves for creating backups. + +[#cons-backup-offline] +=== Take master offline for backup + +Shutdown the server before taking a backup. This is simple but means downtime +for the users. Also crons and currently running cron jobs (e.g. repacking +repositories) which affect the repositories may need to be shut down. + +[#backup-methods] +== Backup methods + +[#backup-methods-snapshots] +=== Filesystem snapshots + +Filesystems supporting copy on write snapshots:: ++ +Use a file system supporting copy-on-write snapshots like +link:https://btrfs.wiki.kernel.org/index.php/SysadminGuide#Snapshots[btrfs] +or +https://wiki.debian.org/ZFS#Snapshots[zfs]. + + +Other filesystems supporting snapshots:: +https://wiki.archlinux.org/index.php/LVM#Snapshots[lvm] or nfs. ++ +Create a snapshot and then archive the snapshot to another storage. ++ +While snapshots are great for creating high quality backups quickly, they are +not ideal as a format for storing backup data. Snapshots typically depend and +reside on the same storage infrastructure as the original disk images. +Therefore, it’s crucial that you archive these snapshots and store them +elsewhere. + +3.0 or newer:: +Snapshot the complete site directory + +2.x:: +Similar, but the data of the database should be stored on the very same volume +on the same machine, so that the snapshot is taken atomically over both +the git data and the database data. Because everything should be ACID, it can safely +crash-recover - as if the power has been plugged and the server got booted up again. +(Actually more safe than that, because the filesystem knows about taking the snapshot, +and also about the pending writes it can sync.) + +In addition to that, using filesystem snapshots allows to: + +* easy and fast roll back without having to access remote backup data (e.g. to restore +accidental rm -rf git/ back in seconds). +* incremental transfer of consistent snapshots +* save a lot of data while still keeping multiple "known consistent states" + +[#backup-methods-other] +=== Other backup methods + +To ensure consistent backups these backup methods require to turn the server into +read-only mode while a backup is running. + +* create an archive like `tar.gz` to backup the site +* `rsync` +* plain old `cp` + +[#backup-methods-test] +== Test backups + +Test backups and fire drill restoring backups to ensure the backups aren't +corrupt or incomplete and you can restore a backup quickly. + +[#backup-dr] +== Disaster recovery + +[#backup-dr-repl] +=== Replicate backup archives + +To enable disaster recovery at least replicate backup archives to another data center. +And fire drill restoring a new site using the backup. + +[#backup-dr-multi-site] +=== Multi-site setup + +Use the https://gerrit.googlesource.com/plugins/multi-site[multi-site plugin] +to install Gerrit with multiple sites installed in different datacenters +across different regions. This ensures that in case of a severe problem with +one of the sites, the other sites can still serve your repositories. + +GERRIT +------ +Part of link:index.html[Gerrit Code Review] + +SEARCHBOX +--------- diff --git a/Documentation/install.txt b/Documentation/install.txt index dbca36882e..b6a295449d 100644 --- a/Documentation/install.txt +++ b/Documentation/install.txt @@ -260,6 +260,11 @@ Place Gerrit plugins in the review_site/plugins directory to have them loaded on * http://www.kernel.org/pub/software/scm/git/docs/git-daemon.html[git-daemon] +[[backup]] +== Backup + +See the link:backup.html[backup documentation]. + GERRIT ------ Part of link:index.html[Gerrit Code Review] From 180bca5b6153bd6554cfb5768af113644140f6c0 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 4 Dec 2019 20:05:07 +0900 Subject: [PATCH 03/15] GitProtocolV2IT: Fail if git client is too old to support v2 JUnit assumption violation is not reflected on Bazel UI: [1]. As the consequence, if a test is passing it's not clear, it's skipped due to assumption violation or it is passing the actual tests. Especially for the important Gerrit features like git wire protocol v2 we must be sure that the integration tests got actually executed and were successful. Given that the git wire protocol test requires very recent git client version (2.18) add git-protocol-v2 label to the test rule and provide instructions how to skip the test in Bazel documentation. [1] https://github.com/bazelbuild/bazel/issues/3476 Bug: Issue 12032 Change-Id: Icba99ab22fa7e99fb303eb79cf3df3b354ee3d77 --- Documentation/dev-bazel.txt | 7 +++++++ javatests/com/google/gerrit/integration/git/BUILD | 2 +- .../com/google/gerrit/integration/git/GitProtocolV2IT.java | 6 +++--- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/Documentation/dev-bazel.txt b/Documentation/dev-bazel.txt index 3eac4e5d0a..eac18d9e38 100644 --- a/Documentation/dev-bazel.txt +++ b/Documentation/dev-bazel.txt @@ -304,6 +304,12 @@ To exclude tests that require a Docker host: bazel test --test_tag_filters=-docker //... ---- +To exclude tests that require very recent git client version: + +---- + bazel test --test_tag_filters=-git-protocol-v2 //... +---- + To ignore cached test results: ---- @@ -324,6 +330,7 @@ The following values are currently supported for the group name: * edit * elastic * git +* git-protocol-v2 * notedb * pgm * rest diff --git a/javatests/com/google/gerrit/integration/git/BUILD b/javatests/com/google/gerrit/integration/git/BUILD index 6a6f5ade57..4184f1fa2a 100644 --- a/javatests/com/google/gerrit/integration/git/BUILD +++ b/javatests/com/google/gerrit/integration/git/BUILD @@ -3,5 +3,5 @@ load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests") acceptance_tests( srcs = glob(["*IT.java"]), group = "git", - labels = ["git"], + labels = ["git-protocol-v2"], ) diff --git a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java index 22981f0ab4..162f73a853 100644 --- a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java +++ b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java @@ -15,7 +15,6 @@ package com.google.gerrit.integration.git; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.deny; import static java.nio.charset.StandardCharsets.UTF_8; @@ -63,8 +62,9 @@ public class GitProtocolV2IT extends StandaloneSiteTest { GitClientVersion requiredGitVersion = new GitClientVersion(2, 18, 0); GitClientVersion actualGitVersion = new GitClientVersion(execute(ImmutableList.of("git", "version"))); - // If not found, test succeeds with assumption violation - assume().that(actualGitVersion).isAtLeast(requiredGitVersion); + // If git client version cannot be updated, consider to skip this tests. Due to + // an existing issue in bazel, JUnit assumption violation feature cannot be used. + assertThat(actualGitVersion).isAtLeast(requiredGitVersion); try (ServerContext ctx = startServer()) { ctx.getInjector().injectMembers(this); From 9a5061212fbaf2158e728d43a82e63889ea211e6 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 6 Dec 2019 08:57:21 +0900 Subject: [PATCH 04/15] Upgrade gitiles blame-cache to 0.2-7.1 Version 0.2-7 is quite old and does not include fixes that have since been included in versions up to the latest 0.2-11 and on the stable-0.2 branch. We cannot upgrade directly to 0.2-11 because that also includes updates to JGit and soy which are not compatible with core Gerrit's 2.16 series. Hence, 0.2-7.1 has been released which includes all of the bug fixes and improvements, but not the library upgrades Change-Id: If208520b23f0b16d97d716ee2da85d98974ab440 --- WORKSPACE | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index cc2435a24e..803ff80f57 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -768,10 +768,10 @@ maven_jar( maven_jar( name = "blame-cache", - artifact = "com/google/gitiles:blame-cache:0.2-7", + artifact = "com/google/gitiles:blame-cache:0.2-7.1", attach_source = False, repository = GERRIT, - sha1 = "8170f33b8b1db6f55e41d7069fa050a4d102a62b", + sha1 = "73915991bb7472a730102ab01ca68776a52466fd", ) # Keep this version of Soy synchronized with the version used in Gitiles. From ece80db8e9d9e0c0b5d4bf2fa135a7b948c16815 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 5 Dec 2019 09:57:10 +0100 Subject: [PATCH 05/15] CreateBranchIT: Add tests that set revision in the input Signed-off-by: Edwin Kempin Change-Id: I75c4424c9f516d46ce7ce34fdea568c05567cc84 --- .../rest/project/CreateBranchIT.java | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java index df896864e5..a4a87d5464 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java @@ -28,12 +28,14 @@ import com.google.gerrit.extensions.api.projects.BranchApi; import com.google.gerrit.extensions.api.projects.BranchInfo; import com.google.gerrit.extensions.api.projects.BranchInput; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.RefNames; +import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Before; import org.junit.Test; @@ -127,6 +129,74 @@ public class CreateBranchIT extends AbstractDaemonTest { "Not allowed to create group branch."); } + @Test + public void createWithRevision() throws Exception { + RevCommit revision = getRemoteHead(project, "master"); + + // update master so that points to a different revision than the revision on which we create the + // new branch + pushTo("refs/heads/master"); + assertThat(getRemoteHead(project, "master")).isNotEqualTo(revision); + + BranchInput input = new BranchInput(); + input.revision = revision.name(); + BranchInfo created = branch(testBranch).create(input).get(); + assertThat(created.ref).isEqualTo(testBranch.get()); + assertThat(created.revision).isEqualTo(revision.name()); + assertThat(getRemoteHead(project, testBranch.getShortName())).isEqualTo(revision); + } + + @Test + public void createWithBranchNameAsRevision() throws Exception { + RevCommit expectedRevision = getRemoteHead(project, "master"); + + BranchInput input = new BranchInput(); + input.revision = "master"; + BranchInfo created = branch(testBranch).create(input).get(); + assertThat(created.ref).isEqualTo(testBranch.get()); + assertThat(created.revision).isEqualTo(expectedRevision.name()); + assertThat(getRemoteHead(project, testBranch.getShortName())).isEqualTo(expectedRevision); + } + + @Test + public void createWithFullBranchNameAsRevision() throws Exception { + RevCommit expectedRevision = getRemoteHead(project, "master"); + + BranchInput input = new BranchInput(); + input.revision = "refs/heads/master"; + BranchInfo created = branch(testBranch).create(input).get(); + assertThat(created.ref).isEqualTo(testBranch.get()); + assertThat(created.revision).isEqualTo(expectedRevision.name()); + assertThat(getRemoteHead(project, testBranch.getShortName())).isEqualTo(expectedRevision); + } + + @Test + public void cannotCreateWithNonExistingBranchNameAsRevision() throws Exception { + assertCreateFails( + testBranch, + "refs/heads/non-existing", + BadRequestException.class, + "invalid revision \"refs/heads/non-existing\""); + } + + @Test + public void cannotCreateWithNonExistingRevision() throws Exception { + assertCreateFails( + testBranch, + "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", + BadRequestException.class, + "invalid revision \"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef\""); + } + + @Test + public void cannotCreateWithInvalidRevision() throws Exception { + assertCreateFails( + testBranch, + "invalid\trevision", + BadRequestException.class, + "invalid revision \"invalid\trevision\""); + } + private void blockCreateReference() throws Exception { block("refs/*", Permission.CREATE, ANONYMOUS_USERS); } From 65008c3b03680addad780d4e442b6819dd8caec7 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 5 Dec 2019 10:03:19 +0100 Subject: [PATCH 06/15] CreateBranch: Allow revision in input to be empty If a revision in the input is not specified we default to the HEAD revision, but when the revision in the input was empty we failed with 400 Bad Request. Be more tolerant and consistent and default to the HEAD revision too if the specified revision is empty. Signed-off-by: Edwin Kempin Change-Id: Ibc1461b744b65dba4857b3ffa00b49bd2c96df99 --- .../server/restapi/project/CreateBranch.java | 3 ++- .../rest/project/CreateBranchIT.java | 26 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/restapi/project/CreateBranch.java b/java/com/google/gerrit/server/restapi/project/CreateBranch.java index 62106e8c1d..05525b5e28 100644 --- a/java/com/google/gerrit/server/restapi/project/CreateBranch.java +++ b/java/com/google/gerrit/server/restapi/project/CreateBranch.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.project; import static com.google.gerrit.reviewdb.client.RefNames.isConfigRef; +import com.google.common.base.Strings; import com.google.common.flogger.FluentLogger; import com.google.gerrit.extensions.api.projects.BranchInfo; import com.google.gerrit.extensions.api.projects.BranchInput; @@ -91,7 +92,7 @@ public class CreateBranch if (input.ref != null && !ref.equals(input.ref)) { throw new BadRequestException("ref must match URL"); } - if (input.revision == null) { + if (Strings.isNullOrEmpty(input.revision)) { input.revision = Constants.HEAD; } while (ref.startsWith("/")) { diff --git a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java index a4a87d5464..cd442e5458 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java @@ -146,6 +146,32 @@ public class CreateBranchIT extends AbstractDaemonTest { assertThat(getRemoteHead(project, testBranch.getShortName())).isEqualTo(revision); } + @Test + public void createWithoutSpecifyingRevision() throws Exception { + // If revision is not specified, the branch is created based on HEAD, which points to master. + RevCommit expectedRevision = getRemoteHead(project, "master"); + + BranchInput input = new BranchInput(); + input.revision = null; + BranchInfo created = branch(testBranch).create(input).get(); + assertThat(created.ref).isEqualTo(testBranch.get()); + assertThat(created.revision).isEqualTo(expectedRevision.name()); + assertThat(getRemoteHead(project, testBranch.getShortName())).isEqualTo(expectedRevision); + } + + @Test + public void createWithEmptyRevision() throws Exception { + // If revision is not specified, the branch is created based on HEAD, which points to master. + RevCommit expectedRevision = getRemoteHead(project, "master"); + + BranchInput input = new BranchInput(); + input.revision = ""; + BranchInfo created = branch(testBranch).create(input).get(); + assertThat(created.ref).isEqualTo(testBranch.get()); + assertThat(created.revision).isEqualTo(expectedRevision.name()); + assertThat(getRemoteHead(project, testBranch.getShortName())).isEqualTo(expectedRevision); + } + @Test public void createWithBranchNameAsRevision() throws Exception { RevCommit expectedRevision = getRemoteHead(project, "master"); From 1a9610aedbbe1e1bf868958f3fa373b0732006f9 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 5 Dec 2019 10:08:29 +0100 Subject: [PATCH 07/15] CreateBranch: Trim revision that is provided in input Signed-off-by: Edwin Kempin Change-Id: I378d38452f1f9d09cb6e52573d71018ceb9342e0 --- .../gerrit/server/restapi/project/CreateBranch.java | 3 +++ .../acceptance/rest/project/CreateBranchIT.java | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/java/com/google/gerrit/server/restapi/project/CreateBranch.java b/java/com/google/gerrit/server/restapi/project/CreateBranch.java index 05525b5e28..ec1f56e718 100644 --- a/java/com/google/gerrit/server/restapi/project/CreateBranch.java +++ b/java/com/google/gerrit/server/restapi/project/CreateBranch.java @@ -92,6 +92,9 @@ public class CreateBranch if (input.ref != null && !ref.equals(input.ref)) { throw new BadRequestException("ref must match URL"); } + if (input.revision != null) { + input.revision = input.revision.trim(); + } if (Strings.isNullOrEmpty(input.revision)) { input.revision = Constants.HEAD; } diff --git a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java index cd442e5458..c49a62b29b 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java @@ -172,6 +172,18 @@ public class CreateBranchIT extends AbstractDaemonTest { assertThat(getRemoteHead(project, testBranch.getShortName())).isEqualTo(expectedRevision); } + @Test + public void createRevisionIsTrimmed() throws Exception { + RevCommit revision = getRemoteHead(project, "master"); + + BranchInput input = new BranchInput(); + input.revision = "\t" + revision.name(); + BranchInfo created = branch(testBranch).create(input).get(); + assertThat(created.ref).isEqualTo(testBranch.get()); + assertThat(created.revision).isEqualTo(revision.name()); + assertThat(getRemoteHead(project, testBranch.getShortName())).isEqualTo(revision); + } + @Test public void createWithBranchNameAsRevision() throws Exception { RevCommit expectedRevision = getRemoteHead(project, "master"); From 822e544ac4fc1a02cc31f7f3c1a13a2db5149624 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 5 Dec 2019 10:26:57 +0100 Subject: [PATCH 08/15] InvalidRevisionException: Include invalid revision into message This way we know which revision was invalid when this exception gets logged. Signed-off-by: Edwin Kempin Change-Id: I0f693c63f32abc65bfb83574def1179896724ad9 --- .../google/gerrit/server/project/RefUtil.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/java/com/google/gerrit/server/project/RefUtil.java b/java/com/google/gerrit/server/project/RefUtil.java index 9f1fa4a21c..696190b94f 100644 --- a/java/com/google/gerrit/server/project/RefUtil.java +++ b/java/com/google/gerrit/server/project/RefUtil.java @@ -19,6 +19,7 @@ import static org.eclipse.jgit.lib.Constants.R_TAGS; import com.google.common.collect.Iterables; import com.google.common.flogger.FluentLogger; +import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; @@ -46,16 +47,16 @@ public class RefUtil { try { ObjectId revid = repo.resolve(baseRevision); if (revid == null) { - throw new InvalidRevisionException(); + throw new InvalidRevisionException(baseRevision); } return revid; } catch (IOException err) { logger.atSevere().withCause(err).log( "Cannot resolve \"%s\" in project \"%s\"", baseRevision, projectName.get()); - throw new InvalidRevisionException(); + throw new InvalidRevisionException(baseRevision); } catch (RevisionSyntaxException err) { logger.atSevere().withCause(err).log("Invalid revision syntax \"%s\"", baseRevision); - throw new InvalidRevisionException(); + throw new InvalidRevisionException(baseRevision); } } @@ -66,7 +67,7 @@ public class RefUtil { try { rw.markStart(rw.parseCommit(revid)); } catch (IncorrectObjectTypeException err) { - throw new InvalidRevisionException(); + throw new InvalidRevisionException(revid.name()); } RefDatabase refDb = repo.getRefDatabase(); Iterable refs = @@ -86,11 +87,11 @@ public class RefUtil { rw.checkConnectivity(); return rw; } catch (IncorrectObjectTypeException | MissingObjectException err) { - throw new InvalidRevisionException(); + throw new InvalidRevisionException(revid.name()); } catch (IOException err) { logger.atSevere().withCause(err).log( "Repository \"%s\" may be corrupt; suggest running git fsck", repo.getDirectory()); - throw new InvalidRevisionException(); + throw new InvalidRevisionException(revid.name()); } } @@ -125,8 +126,8 @@ public class RefUtil { public static final String MESSAGE = "Invalid Revision"; - InvalidRevisionException() { - super(MESSAGE); + InvalidRevisionException(@Nullable String invalidRevision) { + super(MESSAGE + ": " + invalidRevision); } } } From dd07f8bcd1fe89b7247f05ce8c766536fedc799c Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 5 Dec 2019 10:28:38 +0100 Subject: [PATCH 09/15] RefUtil#parseBaseRevision: Do not log an error if baseRevision is invalid Callers should decide whether they want to log the InvalidRevisionException that is thrown in this case. All current callers pass in user provided input, if that is invalid it's a 400 Bad Request, but nothing that should be logged as server error. Signed-off-by: Edwin Kempin Change-Id: Ifdbac1bccea17f1e45a2c84b8869f9abafbd6518 --- java/com/google/gerrit/server/project/RefUtil.java | 1 - 1 file changed, 1 deletion(-) diff --git a/java/com/google/gerrit/server/project/RefUtil.java b/java/com/google/gerrit/server/project/RefUtil.java index 696190b94f..4e08137a1c 100644 --- a/java/com/google/gerrit/server/project/RefUtil.java +++ b/java/com/google/gerrit/server/project/RefUtil.java @@ -55,7 +55,6 @@ public class RefUtil { "Cannot resolve \"%s\" in project \"%s\"", baseRevision, projectName.get()); throw new InvalidRevisionException(baseRevision); } catch (RevisionSyntaxException err) { - logger.atSevere().withCause(err).log("Invalid revision syntax \"%s\"", baseRevision); throw new InvalidRevisionException(baseRevision); } } From b56d2cb2add7720495638c8d8d3f0187ece6d098 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 5 Dec 2019 10:56:36 +0100 Subject: [PATCH 10/15] CreateTag: Allow revision in input to be empty If a revision in the input is not specified we default to the HEAD revision, but when the revision in the input was empty we failed with 400 Bad Request. Be more tolerant and consistent and default to the HEAD revision too if the specified revision is empty. Signed-off-by: Edwin Kempin Change-Id: I78afe991d274d0feba134612382a65c76916cfd5 --- .../server/restapi/project/CreateTag.java | 2 +- .../acceptance/rest/project/TagsIT.java | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/restapi/project/CreateTag.java b/java/com/google/gerrit/server/restapi/project/CreateTag.java index e72deaf255..cdeba62800 100644 --- a/java/com/google/gerrit/server/restapi/project/CreateTag.java +++ b/java/com/google/gerrit/server/restapi/project/CreateTag.java @@ -87,7 +87,7 @@ public class CreateTag implements RestCollectionCreateView expected, List actual) throws Exception { assertThat(actual).hasSize(expected.size()); From bb1aaa14a732f91631db9ced3d3154767c6704df Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 5 Dec 2019 10:59:08 +0100 Subject: [PATCH 11/15] CreateTag: Trim revision that is provided in input Signed-off-by: Edwin Kempin Change-Id: I2fb1cd153c6109caa9444a7c39987c0d8ca30db7 --- .../gerrit/server/restapi/project/CreateTag.java | 3 +++ .../gerrit/acceptance/rest/project/TagsIT.java | 15 +++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/java/com/google/gerrit/server/restapi/project/CreateTag.java b/java/com/google/gerrit/server/restapi/project/CreateTag.java index cdeba62800..855a7c7429 100644 --- a/java/com/google/gerrit/server/restapi/project/CreateTag.java +++ b/java/com/google/gerrit/server/restapi/project/CreateTag.java @@ -87,6 +87,9 @@ public class CreateTag implements RestCollectionCreateView expected, List actual) throws Exception { assertThat(actual).hasSize(expected.size()); From 0e1ad387550cf10702e28a445893c903667d3fd0 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 6 Dec 2019 12:43:50 +0900 Subject: [PATCH 12/15] GitProtocolV2IT: Split git client version check to a separate method Split the version check to a separate method annotated @BeforeClass so that the entire test is failed if the client version is not suitable. Currently there is only one test method, but creating a separate method means that we don't need to copy and paste the same check into each new test method that gets added later. Change-Id: I09ab265f95f56d1ead3128c87edfa17c2e0fa02b --- .../google/gerrit/integration/git/GitProtocolV2IT.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java index 162f73a853..4acd8b8ed0 100644 --- a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java +++ b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java @@ -39,6 +39,7 @@ import java.io.File; import java.net.InetSocketAddress; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; +import org.junit.BeforeClass; import org.junit.Test; @UseSsh @@ -56,16 +57,19 @@ public class GitProtocolV2IT extends StandaloneSiteTest { @Inject private @TestSshServerAddress InetSocketAddress sshAddress; @Inject private @GerritServerConfig Config config; - @Test - public void testGitWireProtocolV2WithSsh() throws Exception { + @BeforeClass + public static void assertGitClientVersion() throws Exception { // Minimum required git-core version that supports wire protocol v2 is 2.18.0 GitClientVersion requiredGitVersion = new GitClientVersion(2, 18, 0); GitClientVersion actualGitVersion = - new GitClientVersion(execute(ImmutableList.of("git", "version"))); + new GitClientVersion(execute(ImmutableList.of("git", "version"), new File("/"))); // If git client version cannot be updated, consider to skip this tests. Due to // an existing issue in bazel, JUnit assumption violation feature cannot be used. assertThat(actualGitVersion).isAtLeast(requiredGitVersion); + } + @Test + public void testGitWireProtocolV2WithSsh() throws Exception { try (ServerContext ctx = startServer()) { ctx.getInjector().injectMembers(this); From 761dc71b18ace52ebec437165b353222da86a9a0 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 22 Nov 2019 23:54:08 +0000 Subject: [PATCH 13/15] Hide refs/meta/config on git protocol v2 Fix a problem in the refs advertisement with git protocol v2 where the refs/meta/config was leaked even though the client did not have any read access to it. The leak was limited to the value of the SHA1 and not to the actual content of the refs/meta/config branch. A fetch of the SHA1 would have resulted in a WantNotValidException. Bug: Issue 11962 Change-Id: I2e4d1d7c8d487166fe00e7d0946b6927bc1cd9c8 --- .../PermissionAwareReadOnlyRefDatabase.java | 30 +++++++++- .../integration/git/GitProtocolV2IT.java | 60 +++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/java/com/google/gerrit/server/git/PermissionAwareReadOnlyRefDatabase.java b/java/com/google/gerrit/server/git/PermissionAwareReadOnlyRefDatabase.java index 8f7e68444c..b80e543952 100644 --- a/java/com/google/gerrit/server/git/PermissionAwareReadOnlyRefDatabase.java +++ b/java/com/google/gerrit/server/git/PermissionAwareReadOnlyRefDatabase.java @@ -28,6 +28,8 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Function; +import java.util.stream.Collectors; import org.eclipse.jgit.annotations.NonNull; import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.lib.Ref; @@ -101,11 +103,35 @@ public class PermissionAwareReadOnlyRefDatabase extends DelegateRefDatabase { Map result; try { - result = forProject.filter(refs, getDelegate(), RefFilterOptions.defaults()); + // The security filtering assumes to receive the same refMap, independently from the ref + // prefix offset + result = + forProject.filter( + prefixIndependentRefMap(prefix, refs), getDelegate(), RefFilterOptions.defaults()); } catch (PermissionBackendException e) { throw new IOException(""); } - return result; + return applyPrefixRefMap(prefix, result); + } + + private Map prefixIndependentRefMap(String prefix, Map refs) { + if (prefix.length() > 0) { + return refs.values().stream().collect(Collectors.toMap(Ref::getName, Function.identity())); + } + + return refs; + } + + private Map applyPrefixRefMap(String prefix, Map refs) { + int prefixSlashPos = prefix.lastIndexOf('/') + 1; + if (prefixSlashPos > 0) { + return refs.values().stream() + .collect( + Collectors.toMap( + (Ref ref) -> ref.getName().substring(prefixSlashPos), Function.identity())); + } + + return refs; } @Override diff --git a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java index 4acd8b8ed0..9c4bdef407 100644 --- a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java +++ b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java @@ -29,7 +29,10 @@ import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.acceptance.UseSsh; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.common.data.Permission; +import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; +import com.google.gerrit.entities.RefNames; import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.common.ChangeInput; import com.google.gerrit.server.config.GerritServerConfig; @@ -48,6 +51,8 @@ public class GitProtocolV2IT extends StandaloneSiteTest { new String[] {"ssh-keygen", "-t", "rsa", "-q", "-P", "", "-f"}; private final String[] GIT_LS_REMOTE = new String[] {"git", "-c", "protocol.version=2", "ls-remote", "-o", "trace=12345"}; + private final String[] GIT_CLONE_MIRROR = + new String[] {"git", "-c", "protocol.version=2", "clone", "--mirror"}; private final String GIT_SSH_COMMAND = "ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -i"; @@ -197,6 +202,61 @@ public class GitProtocolV2IT extends StandaloneSiteTest { } } + @Test + public void testGitWireProtocolV2HidesRefMetaConfig() throws Exception { + try (ServerContext ctx = startServer()) { + ctx.getInjector().injectMembers(this); + String url = config.getString("gerrit", null, "canonicalweburl"); + + // Create project + Project.NameKey allRefsVisibleProject = Project.nameKey("all-refs-visible"); + gApi.projects().create(allRefsVisibleProject.get()); + + // Set protocol.version=2 in target repository + execute( + ImmutableList.of("git", "config", "protocol.version", "2"), + sitePaths + .site_path + .resolve("git") + .resolve(allRefsVisibleProject.get() + Constants.DOT_GIT) + .toFile()); + + // Set up project permission to allow reading all refs + projectOperations + .project(allRefsVisibleProject) + .forUpdate() + .add(allow(Permission.READ).ref("refs/heads/*").group(SystemGroupBackend.ANONYMOUS_USERS)) + .add( + allow(Permission.READ) + .ref("refs/changes/*") + .group(SystemGroupBackend.ANONYMOUS_USERS)) + .update(); + + // Create new change and retrieve refs for the created patch set + ChangeInput visibleChangeIn = + new ChangeInput(allRefsVisibleProject.get(), "master", "Test public change"); + visibleChangeIn.newBranch = true; + int visibleChangeNumber = gApi.changes().create(visibleChangeIn).info()._number; + Change.Id changeId = Change.id(visibleChangeNumber); + String visibleChangeNumberRef = RefNames.patchSetRef(PatchSet.id(changeId, 1)); + String visibleChangeNumberMetaRef = RefNames.changeMetaRef(changeId); + + // Read refs from target repository using git wire protocol v2 over HTTP anonymously + String outAnonymousLsRemote = + execute( + ImmutableList.builder() + .add(GIT_CLONE_MIRROR) + .add(url + "/" + allRefsVisibleProject.get()) + .build(), + ImmutableMap.of("GIT_TRACE_PACKET", "1")); + + assertThat(outAnonymousLsRemote).contains("git< version 2"); + assertThat(outAnonymousLsRemote).doesNotContain(RefNames.REFS_CONFIG); + assertThat(outAnonymousLsRemote).contains(visibleChangeNumberRef); + assertThat(outAnonymousLsRemote).contains(visibleChangeNumberMetaRef); + } + } + private void setUpUserAuthentication(String username) throws Exception { // Assign HTTP password to user gApi.accounts().id(username).setHttpPassword("secret"); From 3df01b01fb25fb6d654fc2257152956cf14b52da Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 6 Dec 2019 13:36:44 +0100 Subject: [PATCH 14/15] Use consistent capitalization for term NoteDb in documentation Signed-off-by: Edwin Kempin Change-Id: I556efac6f229dc987f02c5477322694691f30825 --- Documentation/backup.txt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Documentation/backup.txt b/Documentation/backup.txt index 7246ca766e..ed044ba665 100644 --- a/Documentation/backup.txt +++ b/Documentation/backup.txt @@ -12,7 +12,7 @@ Git repositories:: The bare Git repositories managed by Gerrit are typically stored in the `${SITE}/git` directory. However, the locations can be customized in `${site}/etc/gerrit.config`. They contain the history of the respective -projects, and since 2.15 if you are using _NoteDB_, and for 3.0 and newer, +projects, and since 2.15 if you are using _NoteDb_, and for 3.0 and newer, also change and review metadata, user accounts and groups. + @@ -21,14 +21,14 @@ SQL database:: + Gerrit releases in the 2.x series store some data in the database you have chosen when installing Gerrit. If you are using 2.16 and have -migrated to _NoteDB_ only the schema version is stored in the database. +migrated to _NoteDb_ only the schema version is stored in the database. + If you are using h2 you need to backup the `.db` files in the folder `${SITE}/db`. + For all other database types refer to their backup documentation. + -Gerrit release 3.0 and newer store all primary data in _NoteDB_ inside +Gerrit release 3.0 and newer store all primary data in _NoteDb_ inside the git repositories of the Gerrit site. Only the review flag marking in the UI when you have reviewed a changed file is stored in a relational database. If you are using h2 this database is named @@ -118,14 +118,14 @@ Create a snapshot and then archive the snapshot. Gerrit 2.x:: + -Gerrit 2.16 can use _NoteDB_ to store almost all this data which +Gerrit 2.16 can use _NoteDb_ to store almost all this data which simplifies creating backups since consistency between database and git -repositories is no longer critical. If you migrated to noteDB you can +repositories is no longer critical. If you migrated to _NoteDb_ you can follow the backup procedure for 3.0 and higher and additionally take a backup of the database, which only contains the schema version, hence consistency between git and database is no longer critical since the schema version only changes during upgrade. If you didn't migrate -to noteDB then follow the backup procedure for older 2.x Gerrit versions. +to _NoteDb_ then follow the backup procedure for older 2.x Gerrit versions. + Older 2.x Gerrit versions store change meta data, review comments, votes, accounts and group information in a SQL database. Creating consistent backups @@ -163,7 +163,7 @@ Best you use a filesystem supporting snapshots to create a backup archive of such a replica. For 2.x Gerrit versions also set up a database slave for the data stored in the -SQL database. If you are using 2.16 and migrated to noteDB you may consider to +SQL database. If you are using 2.16 and migrated to _NoteDb_ you may consider to skip setting up a database slave, instead take a backup of the database which only contains the current schema version in this case. In addition you need to ensure that no write operations are in flight before you From 5b330f58b58bdf13b0da9548acfeeb52ebfc7a11 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sat, 7 Dec 2019 09:26:14 +0900 Subject: [PATCH 15/15] Update replication plugin to get fixes for CI flakiness Bug: Issue 11843 Change-Id: I520eff3a86279048d143a3c0f68a844b2b580cd9 --- plugins/replication | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/replication b/plugins/replication index 84d508247c..505f2c63c4 160000 --- a/plugins/replication +++ b/plugins/replication @@ -1 +1 @@ -Subproject commit 84d508247c619b9e1871d7ce47517334bb0240c1 +Subproject commit 505f2c63c41100d215e706c7df6932854d213bd7