The future of commit access policy for core Firefox

Gregory Szorc gps at mozilla.com
Tue Mar 21 22:40:30 UTC 2017


On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor <mconnor at mozilla.com> wrote:

> (please direct followups to dev-planning, cross-posting to governance,
> firefox-dev, dev-platform)
>
>
> Nearly 19 years after the creation of the Mozilla Project, commit access
> remains essentially the same as it has always been.  We've evolved the
> vouching process a number of times, CVS has long since been replaced by
> Mercurial & others, and we've taken some positive steps in terms of
> securing the commit process.  And yet we've never touched the core idea of
> granting developers direct commit access to our most important
> repositories.  After a large number of discussions since taking ownership
> over commit policy, I believe it is time for Mozilla to change that
> practice.
>
> Before I get into the meat of the current proposal, I would like to
> outline a set of key goals for any change we make.  These goals have been
> informed by a set of stakeholders from across the project including the
> engineering, security, release and QA teams.  It's inevitable that any
> significant change will disrupt longstanding workflows.  As a result, it is
> critical that we are all aligned on the goals of the change.
>
>
> I've identified the following goals as critical for a responsible commit
> access policy:
>
>    - Compromising a single individual's credentials must not be
>    sufficient to land malicious code into our products.
>    - Two-factor auth must be a requirement for all users approving or
>    pushing a change.
>    - The change that gets pushed must be the same change that was
>    approved.
>    - Broken commits must be rejected automatically as a part of the
>    commit process.
>
>
> In order to achieve these goals, I propose that we commit to making the
> following changes to all Firefox product repositories:
>
>    - Direct commit access to repositories will be strictly limited to
>    sheriffs and a subset of release engineering.
>       - Any direct commits by these individuals will be limited to fixing
>       bustage that automation misses and handling branch merges.
>    - All other changes will go through an autoland-based workflow.
>       - Developers commit to a staging repository, with scripting that
>       connects the changeset to a Bugzilla attachment, and integrates with review
>       flags.
>       - Reviewers and any other approvers interact with the changeset as
>       today (including ReviewBoard if preferred), with Bugzilla flags as the
>       canonical source of truth.
>       - Upon approval, the changeset will be pushed into autoland.
>       - If the push is successful, the change is merged to
>       mozilla-central, and the bug updated.
>
> I know this is a major change in practice from how we currently operate,
> and my ask is that we work together to understand the impact and concerns.
> If you find yourself disagreeing with the goals, let's have that discussion
> instead of arguing about the solution.  If you agree with the goals, but
> not the solution, I'd love to hear alternative ideas for how we can achieve
> the outcomes outlined above.
>

(Responding to several topics from the top post because I don't want to
spam with multiple replies.)

I think a lot of people are conflating "push access" with "ability to land
something." The distinction is the former grants you privileges to `hg
push` directly to repos on hg.mozilla.org and the latter allows delegation
of this ability. It is easy to conflate them because today "push access"
(level 3) gives you permission to trigger the landing (via the autoland
service via MozReview). But this is only because it was convenient to
implement this way. I want to explicitly state that we're moving towards
N=~10 people having raw push access to the Firefox repos with the majority
of landings occurring via autoland (via Conduit via MozReview/Bugzilla).
This reduces our attack surface area (less exposure to compromised SSH
keys), establishes better auditing (history of change will be on
Bugzilla/MozReview and not on a random local machine), eliminates potential
for bugs or variance with incorrect rebasing done on local machines, and
allows us to do extra things as part of the landing/rebase, such as
automatically reformat code to match unified code styles, do binding
generation, etc. They say all problems in computer science can be solved by
another level of indirection. Deferred landing (autoland) is such an
indirection and it solves a lot of problems. It will be happening. Most of
us will lose access to push directly to the Firefox repos. We won't be
losing ability to initiate a push, however. For the record, I'm not in
favor of making this change until the tooling is such that it won't be a
significant workflow/productivity regression. This is a reason why it
hasn't been made yet.

A handful have commented on whether a rebase invalidates a previous r+.
This is a difficult topic. Strictly speaking, a rebase invalidates
everything because a change in a commit being rebased over could invalidate
assumptions. Same goes for a merge commit. In reality, most rebases are
harmless and we shouldn't be concerned with their existence. In the cases
where it does matter, we can help prevent things from falling through the
cracks by having the review tool detect when in-flight reviews touch the
same files / topics and route them to the same reviewer(s) and/or notify
the different reviewer(s) so people are aware of potential for conflict.
This feature was conceived before MozReview launched but (sadly) hasn't
been implemented.

There have been a few comments on interdiffs when rebases are in play. I
want to explicitly state that there is no single "correct" way to display a
diff much less an interdiff much less an interdiff when a rebase is
involved. This is why tools like Git have multiple diffing algorithms
(minimal, patience, histogram, Myers). This is why there are different ways
of rendering a diff (unified, side-by-side, 3-way). Rendering a simple diff
is hard. Rendering an interdiff is harder. Unfortunately, central review
tools force a specific rendering on users. ReviewBoard does allow some
tweaking of diff behavior (such as ignore whitespace). But no matter what
options you use, someone will be unhappy with it. An example is MozReview's
handling of interdiffs. It used to hide lines changed by a rebase that
weren't changed in the commit up for review. But that was slightly buggy in
some situations and some people wanted to actually see those changes, so
the behavior was changed to show all file changes in the interdiff. This
made a different set of people unhappy because the changes were spurious
and contributed noise. In summary, you can't please all the people all the
time. So you have to pick a reasonable default then debate about adding
complexity to placate the long tail or handle corner cases. Standard
product design challenges. On top of that, technical users are the 1% and
are a very difficult crowd to please.

I'm sensitive to concerns that removal of "r+ with nits" will provide a net
productivity regression. We should be thinking of ways to make landing code
easier, not harder. This is a larger discussion spanning several topics, of
course. But the point I want to make is we have major, systemic problems
with code review latency today. Doing away with "r+ with nits" exacerbates
them, which is part of the reason I think people feel so passionately about
it. I feel like there needs to be a major cultural shift that emphasizes
low review latency, especially for new and high volume contributors. Tools
can help some. But tools only encourage behavior, not enforce it. I feel
like some kind of forcing function (either social/cultural or formal in
policy) is needed. What exactly, I'm not sure. At this point (where I see
very senior engineers with massive review queues), I fear the problem may
have to be addressed by management to adequately correct. (Yes, I hated
having to type the past few sentences.)

I think Steven MacLeod's response was highly insightful and is worth
re-reading multiple times. Pursuant to his lines of thought, I think there
is room to have more flexible policies for different components. For
example, high-value code like anything related to cryptography or security
could be barred from having "r+ with nits" but low-impact, non-shipping
files like in-tree documentation could have a very low barrier to change.
Of course, this implies flexible tools to accommodate flexible workflows
which may imply Mozilla's continued investment in those tools (which is in
contrast to a mindset held by some that we should use tools in an
off-the-shelf configuration instead of highly tailoring them to our
workflows). I like the simplicity of one policy for everything but am not
convinced it is best. I'd like more details on what other large companies
do here.

I hope this post doesn't revive this thread and apologize in advance if it
does. Please consider replying privately instead of poking the bear.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.mozilla.org/pipermail/firefox-dev/attachments/20170321/296ed6f7/attachment.html>


More information about the firefox-dev mailing list