upcoming code review requirement changes

1367 views
Skip to first unread message

Russ Cox

unread,
Apr 4, 2022, 9:00:14 AMApr 4
to golang-dev
Hi all,

For compliance and supply chain security reasons, Google recently revisited the code review requirements we use in all settings, both internal development and open source. We are now required to have two Google employees review each change before it is shipped to users, which for most of our tools means submitted in Gerrit (tools like “go get” and “gotip download” and the go.dev auto-deployment read directly from Gerrit).

As a result, later this month we will be adding a new Gerrit submit requirement that two Google employees have either uploaded or contributed a positive Code-Review vote (+1 or +2) to each change before it can be submitted. This will replace the current Trust+1 scheme.

In x/website repo, Code-Review+2 and Trust+1 votes have been restricted to Google employees on the Go team for the past half year or so, for related supply chain security reasons (changes to the web site are automatically deployed and could potentially redirect downloads of the Go distribution). That repo will be the first to adopt the new system, and once we are confident it is working well there, we will move on to the other repos. I will send a reply to this thread and update go.dev/wiki/GerritAccess when all repos have been converted to the new system.

The x/proposal and x/scratch repos will continue to allow single-person self-review, since they do not contain any code being shipped to users.

Everyone who can currently Code-Review+2 a CL will still be able to do that, and x/website will reopen to Code-Review+2 from all our approvers. Some CLs will then need additional Code-Review+1 votes from Google employees. I realize this change may be disappointing, and I apologize. All of us on the Go team at Google remain very committed to Go being open source and to preserving our open development processes. Once the new system is in place, I will share a link to a dashboard that we will be checking regularly for CLs that have a Code-Review+2 but need additional Code-Review+1 votes.

Thank you for all your contributions and help working on Go. We couldn't do it without all of you.

Best,
Russ

U Cirello

unread,
Apr 4, 2022, 9:05:27 AMApr 4
to Russ Cox, golang-dev
Hi,


If I am correctly informed, within Google, every change to the main repository has to be reviews by another class of reviewers whose job is to confirm readability. For the sake of transparency, wouldn't it be better to actually establish a "Compliance+1" vote with assigned people to the job. So to make it transparent when an approval happened because of the change vs an approval that happened because of compliance.

My understanding is that by enforcing two "vanilla" reviewers, each reviewer may not understand what role they are playing in the approval.


Thanks,
Ulderico

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAA8EjDQv2CTZPmiMHM4HWJnT3QTd630%3DrB-dPeDDjQj6%3DhyoqQ%40mail.gmail.com.

Dominik Honnef

unread,
Apr 4, 2022, 9:38:34 AMApr 4
to golang-dev
If two Googlers need to sign off on CLs then what is the point of non-Googlers being able to +2 changes? Even if I +2 something, 1-2 Googlers still need to review the change and at least leave a +1. But since their +1 signs off on my +2, it is effectively a +2, too. Plus their review probably needs to be as in-depth as an actual +2, anyway, to prevent those pesky supply chain attacks. So whether I +2 a CL or not makes no difference. Am I missing something?

Alberto Donizetti

unread,
Apr 4, 2022, 9:54:30 AMApr 4
to Dominik Honnef, golang-dev
> Am I missing something?

I don't think you're missing anything. This change effectively restricts
the committer group to Google employees.

Alberto
> --
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/8e9a6611-240d-49a0-bb75-270837459b7fn%40googlegroups.com.

Russ Cox

unread,
Apr 4, 2022, 9:56:15 AMApr 4
to Dominik Honnef, golang-dev
On Mon, Apr 4, 2022 at 9:38 AM Dominik Honnef <dominik...@gmail.com> wrote:
If two Googlers need to sign off on CLs then what is the point of non-Googlers being able to +2 changes? Even if I +2 something, 1-2 Googlers still need to review the change and at least leave a +1. But since their +1 signs off on my +2, it is effectively a +2, too. Plus their review probably needs to be as in-depth as an actual +2, anyway, to prevent those pesky supply chain attacks. So whether I +2 a CL or not makes no difference. Am I missing something?

I don't think that's quite true. Two shallow +1 reviews is not the same as one in-depth +2 review. This is the text I plan to put on the GerritAccess page once we switch to the new system:

Every CL requires both a code review (Code-Review+2) from an approver and the involvement of two Google employees, either as code uploader or as a reviewer voting at least Code-Review+1. Requiring multiple people ensures that code cannot be submitted unilaterally from a single compromised account. Once a review has a Code-Review+2 and the necessary Google involvement, it can be submitted by any approver. All these rules are enforced by the Gerrit server.

A Code-Review+2 vote means that you have read the change and are confident that it is correct and appropriate to submit. Typically, you should only Code-Review+2 code in directories or packages that you "own"; the exception is trivial and obviously correct changes. Note that all user-visible new features or changes—new API, new command-line flags, and so on—need to go through the proposal process. The CLs should reference the specific accepted proposal in the commit message (“For #NNN.”).

A Code-Review+1 vote means that you have read the change and believe it seems reasonable but aren’t making the definitive judgement that Code-Review+2 indicates. It also means that are confident that the change does not introduce any sort of security vulnerability or other clearly inappropriate code change.

We fully expect that CLs will continue to land with only non-Googler Code-Review+2 reviews as they do today. 

Best,
Russ

Russ Cox

unread,
Apr 4, 2022, 10:04:02 AMApr 4
to U Cirello, golang-dev
On Mon, Apr 4, 2022 at 9:05 AM U Cirello <ulderi...@gmail.com> wrote:
If I am correctly informed, within Google, every change to the main repository has to be reviews by another class of reviewers whose job is to confirm readability. For the sake of transparency, wouldn't it be better to actually establish a "Compliance+1" vote with assigned people to the job. So to make it transparent when an approval happened because of the change vs an approval that happened because of compliance.

For what it's worth, we did look into this, but the system that has been developed in Gerrit does not work this way. And this is more like a code review than readability is, so I think it will work out okay. But I understand your point. In my previous reply on this thread I quoted the description that will go on the GerritAccess wiki page, in case that helps clarify matters. Thanks.

Best,
Russ

Dan Kortschak

unread,
Apr 4, 2022, 4:01:09 PMApr 4
to golan...@googlegroups.com
I can understand the rationale for this, but I'm concerned that we will
now see review length blow out the the square factor for reviews that
already can take a long time (I have a tiny change that is nearly a
year old).

Dan


Russ Cox

unread,
Apr 4, 2022, 5:40:36 PMApr 4
to Dan Kortschak, golan...@googlegroups.com
On Mon, Apr 4, 2022 at 4:01 PM 'Dan Kortschak' via golang-dev <golan...@googlegroups.com> wrote:
I can understand the rationale for this, but I'm concerned that we will
now see review length blow out the the square factor for reviews that
already can take a long time (I have a tiny change that is nearly a
year old).

Hi Dan,

I understand your concern. It should not cause a quadratic effect, though.
The hard part is getting the Code-Review+2, which isn't changing at all.
Once that is done, the CL will show up on a dashboard that we'll be watching
and we'll add the necessary Code-Review+1 votes after a quick review for
security and related problems, not an in-depth +2-level review.

Best,
Russ

Daniel Martí

unread,
Apr 4, 2022, 8:35:47 PMApr 4
to Russ Cox, golang-dev
Hello Russ,

Thanks for being upfront about this change. I expected to lose the
ability to click "submit" sooner than later due to the submit queue bot,
though I admit I didn't expect a wider change like this one.

Before I get to my concerns, I have some questions about GerritAccess:

> Every CL requires both a code review (Code-Review+2) from an approver and
> the involvement of two Google employees, either as code uploader or as a
> reviewer voting at least Code-Review+1. Requiring multiple people ensures
> that code cannot be submitted unilaterally from a single compromised
> account.

If you want to protect the project against a single approver account
being compromised, it seems to me like the existing system with two
Trust votes already accomplishes that. I assume this wording would need
to be adjusted, as it describes the current system.

Second, I would like to understand what it is about Google that prevents
two of their approver employees from being compromised at once.
There's clearly something that external approvers are missing, as
otherwise this wouldn't be a factor in the equation at all.

For example, I would be completely open to Go approvers being required
to have strong multi-factor authentication, even for each Gerrit session
where they approve or submit changes. This kind of security enhancement
hasn't been touched upon before, as far as I can tell.

Onto the concerns: your prediction is right that I am disappointed.
I'll try to explain why in three main points below:

First, it feels like the set of Go approvers loses much of its meaning.
Google company-wide security policies aside, it's as if the project no
longer trusted external contributors to perform their duties.
If they can't be trusted to keep their accounts reasonably secure,
should they be trusted to maintain parts of the Go project?

The above also goes the other way: as an external contributor and
maintainer, should I really invest that much time and energy in the
project if the project doesn't reciprocate with trust?
I understand that the "Googlers only" rule is probably not your choice,
but its effect on the project and community is the same regardless.

Second, this change will make timezone differences more painful.
Practically all Go team members at Google are in US timezones,
meaning that people like me have little overlap in working hours.
Our work is generally asynchronous and that is fine, but being able to
submit a CL without waiting hours or days is still valuable.

For example, if a "long" builder in build.golang.org unexpectedly breaks
due to a CL of mine, I can notice that and attempt to fix it if I'm
around, but not if the CL gets merged six hours later when I'm AFK.
I expect this will be the reality, because the new Gerrit dashboard
won't be manned anywhere close to 24/7.

Third, no matter how well engineered the Gerrit dashboard is,
it will still ultimately lead to more work for the Go team at Google,
which is worrying given the project is already lagging behind in reviews.
I'm too far removed to know who is responsible for the supply chain
security measures, but I would assume they can realise that they are not
viable unless the Go team magically doubles in headcount.

In summary, Go needs more maintainers and reviewers, and doing that work
outside of Google has always been a delicate balance. There are calls
and emails that I'll never be included in due to not being at Google,
and I've come to accept that. However, I do fear that this change will
make it even less likely to actively participate in Go from the outside.

Russ Cox

unread,
Apr 4, 2022, 10:00:18 PMApr 4
to Daniel Martí, golang-dev
Hi Daniel,

On the whole, you are right and I agree with you.

Unfortunately, as I noted briefly in the first message, Google also has compliance reasons - independent of software supply chain concerns - for the rule about review by two Google employees, and I can't say more about the details. All I can say is that we are now required to do this review before a change to Go ships to users, and our many tools that read directly from the latest commits in the Git repos force our hand as far as the review happening before code is submitted to those repos.

The existing system with two Trust+1 votes does protect against single-account compromises. The issue is unfortunately not whether we on the Go team at Google trust you or any of the other approvers - we certainly do. The fact that it's no longer about trust is part of why we are dropping the Trust label as the enforcement mechanism.

I understand what you are saying about it feeling like being a Go approver loses its meaning. We intend for that not to be true in practice, as we still intend to separate Code-Review+2 from the compliance-induced Code-Review+1. But again I understand what you are saying, and it is absolutely on us to demonstrate that being a Go approver still means something as we shift to the new system. The same applies to timezone lag as well as creating more work for us on the Go team at Google. We know about those problems and are going to work to ameliorate them. Again it's on us to make that reality once the new system is in place.

For the past six months, I have been trying to find some way, any way, to avoid writing today's email. Obviously I was unsuccessful in that endeavor, and for that I am truly sorry and I apologize. Having failed at not making the change at all, we are committed to doing everything we can to make the change as unobtrusive as possible.

I wish I had a better answer. Again, my apologies.

Best,
Russ

Daniel Martí

unread,
Apr 5, 2022, 7:05:45 PMApr 5
to Russ Cox, golang-dev
Hello Russ,

I appreciate your frankness, and I can see you anticipated the reaction
to the announcement and tried to prevent it altogether.

I seem to understand that this change is not really about security,
then; rather about some opaque set of compliance rules at Google
which are now being imposed on open source projects like Go.

I understand that you can't say more about that. I also can't help but
wonder who is in charge of this top-down decision. To anyone who
understands how open source projects and communities work, it honestly
feels like a rather short-sighted move. If the concern was simply
security, I am sure that we could figure out a better compromise.

I can see you have some plans to reduce the harm done by this change,
and I thank you for the effort, though I fear the end result will be
perceived as practically the same - a big part being the optics.

Put another way, we already apologize to contributors for requiring them
to have a Google account and to sign the Google CLA [1]. It seems like
we'll also be apologizing for requiring all approvals to go through
Google employees. We can collectively feel sorry about the situation,
but that doesn't sound like a good long-term plan for the OSS project :)

[1]: https://github.com/golang/go/issues/49383

Russ Cox

unread,
Apr 6, 2022, 4:30:31 PMApr 6
to golang-dev
Hi all,

Status update: We've made the change in x/website. The dashboard for CLs that need more Google reviews is https://go.dev/s/needs-review. We may adjust the search over time, so if you bookmark, bookmark that link and not the URL it currently redirects to. Probably next week we will make the change in the other repos. 

Thanks and apologies again.

Best,
Russ

Benny Siegert

unread,
Apr 7, 2022, 5:13:38 AMApr 7
to Russ Cox, golang-dev
It does not look like this redirects to a valid URL:

<a href="is:open label:Code-Review=+2 label:Review-Enforcement=NEED
label:Trybot-Result=+1">Found</a>.

--
Benny
> --
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAA8EjDS%2BcxyuByRhRmmRoX6x6psbbRR2yXdnmEFermY804vmiQ%40mail.gmail.com.



--
Benny

Dmitri Shuralyov

unread,
Apr 7, 2022, 11:12:33 AMApr 7
to golang-dev
Thanks Benny, the redirect URL is corrected.

Benny Siegert

unread,
Apr 7, 2022, 12:05:17 PMApr 7
to Dmitri Shuralyov, golang-dev
Works for me, thanks!
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/4dc17c1c-6e2f-41c0-ba32-d49af784f5can%40googlegroups.com.



--
Benny

Russ Cox

unread,
Apr 11, 2022, 12:30:04 PMApr 11
to golang-dev
Hi all,

Status update: We've made the change in all the repos, except proposal and scratch. 
https://go.dev/wiki/GerritAccess has been updated to describe the state of the world.
https://go.dev/s/needs-review shows the CLs that are waiting for more Google reviews.
Let us know if you run into any unexpected problems.
Thanks!

Best,
Russ

Russ Cox

unread,
Apr 11, 2022, 12:31:05 PMApr 11
to golang-dev
One more note: If you had configured "mail = codereview mail -trust" in your $HOME/.gitconfig,
then you will need to remove the -trust flag, since the Trust label no longer exists.

Best,
Russ

Brad Fitzpatrick

unread,
Apr 21, 2022, 6:37:53 PMApr 21
to Russ Cox, golang-dev
On Mon, Apr 11, 2022 at 9:30 AM Russ Cox <r...@golang.org> wrote:
Hi all,

Status update: We've made the change in all the repos, except proposal and scratch. 
https://go.dev/wiki/GerritAccess has been updated to describe the state of the world.
https://go.dev/s/needs-review shows the CLs that are waiting for more Google reviews.
Let us know if you run into any unexpected problems.

 There are a bunch queued up in that needs-review link, including one of mine I'm getting impatient about.

How often do Googlers check those?

GTAL (Googlers, Take A Look)

Benny Siegert

unread,
Apr 22, 2022, 3:12:49 AMApr 22
to Brad Fitzpatrick, Russ Cox, golang-dev
Thanks for the reminder, Brad. I just went through the list (and Than
did too, apparently). All but one are unblocked now.

--
Benny
> --
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAFzRk032bhzgSKRREfNp2iVnH94dKe8qbKfFmwZOSksjc%3Dx1-w%40mail.gmail.com.



--
Benny
Reply all
Reply to author
Forward
0 new messages