search

kubernetes / test-infra

Test infrastructure for the Kubernetes project.
Apache License 2.0
3.83k stars 2.65k forks source link

Add an option to make the `lgtm` label sticky for PRs authored by repo collaborators. #9786

Closed cjwagner closed 6 years ago

cjwagner commented 6 years ago

Currently the lgtm label is removed when a PR's commits change unless the store_tree_hash feature is enabled and the tree hash is unchanged. Istio has expressed that the removal and reapplication of the lgtm label is producing excessive spam and that they would prefer for the lgtm label to remain on PRs when they are updated, at least for PRs authored by repo collaborators.

This feature will need to be optional as it reduces the security of Prow by placing complete trust in org members not to merge something maliciously. This would be possible if an org member gets a PR lgtmed and then changes the contents to something malicious. The feature certainly needs to be limited to repository collaborators, otherwise anyone on the internet could use this attack.

If we add a stick_to_collaborator_prs option to the LGTM config struct we can update the lgtm plugin to skip removing the label if the PR author is a repo collaborator.

/area prow /help

k8s-ci-robot commented 6 years ago

@cjwagner: This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes/test-infra/issues/9786): >Currently the `lgtm` label is removed when a PR's commits change unless the `store_tree_hash` feature is enabled and the tree hash is unchanged. Istio has expressed that the removal and reapplication of the `lgtm` label is producing excessive spam and that they would prefer for the `lgtm` label to remain on PRs when they are updated, at least for PRs authored by repo collaborators. > >This feature will need to be optional as it reduces the security of Prow by placing complete trust in org members not to merge something maliciously. This would be possible if an org member gets a PR `lgtm`ed and then changes the contents to something malicious. The feature certainly needs to be limited to repository collaborators, otherwise anyone on the internet could use this attack. > >If we add a `stick_to_collaborator_prs` option to the [LGTM config struct](https://github.com/kubernetes/test-infra/blob/9b5cb4218db97e17aeef5c4d7984466e5a2ebc28/prow/plugins/plugins.go#L363-L372) we can update the `lgtm` plugin to skip removing the label if the PR author is a repo collaborator. > >/area prow >/help Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
cjwagner commented 6 years ago

cc @hklai

BenTheElder commented 6 years ago

If we create this option we really need to strongly suggest not using this with tide, and consider disallowing it in the kubernetes orgs via presubmit... Even ignoring malicious changes this would be really surprising behavior within these projects currently.

BenTheElder commented 6 years ago

Honestly though, if LGTM is sticky and approve is sticky and anyone in the org can apply lgtm... there's not much point in ever applying it, you might as well just only require approve ...

hklai commented 6 years ago

Had a follow up discussion with @cjwagner.

Perhaps we can skip LGTM removal if the PR author is already in the org. That way we won't allow any random person to sneak in code after LGTM, and we certainly trust the people who are already in the org.

cjwagner commented 6 years ago

Perhaps we can skip LGTM removal if the PR author is already in the org.

Yeah, thats what I am suggesting in the issue body. However...

As Ben points out, this kind of defeats the point of the lgtm label since you are already using the approve plugin.

Kubernetes would definitely not use this feature and I would strongly discourage any Prow instance from using it given that it introduces a giant security hole that is pretty trivial to exploit. @hklai said that Istio is okay with this because they trust all their org members. That doesn't seem like a safe assumption to make in my opinion, but I think thats Istio's decision to make? Adding this as an optional feature seems like it could be fine to me since it may be suitable for very small organizations (notably private ones where you must be a repo collaborator in order to make a PR), but it would need to be prefaced with a gauntlet of warnings about the security risks that are being accepted.

stevekuznetsov commented 6 years ago

Istio is okay with this because they trust all their org members

Just give them merge rights to the repo and push the button? Or only require approve?

hklai commented 6 years ago

Just give them merge rights to the repo and push the button? Or only require approve?

We still want to avoid manual merge.

What we want here is to simplify developer workflow and reduce spam. More often than not, PRs are updated after LGTM to address comments, fix comments, lint erros, and comments. Removing LGTM significantly increase merge latency.

As long as this option is restricted to PRs authored by collaborators, we are fine, as we have a formal on-boarding procedure, and we generally trust them.

stevekuznetsov commented 6 years ago

I still don't see why requiring only approve is not sufficient for your workflow.

neolit123 commented 6 years ago

if a PR is submitted by an approver it would be approved already? so once the tests passed, without LGTM it will just auto-marge. maybe that's the issue they have with only approve.

i guess the whole point of using LGTM here is that someone else reviews and applies a second sticky label.

hklai commented 6 years ago

Indeed. Also there are more people who can lgtm than approve (only those on OWNERS).

neolit123 commented 6 years ago

but at the same time, while it's OK to have the sticky Approve, the sticky LGTM can cause you a PR merged, before all the desired changes are applied.

after all, the moment both labels are present the bot will try to merge unless you put a hold on that PR.

BenTheElder commented 6 years ago

Right, but if you're requiring approve you still need an approve from that smaller pool of people, why also require an LGTM? If you trust them then trust them with just approval..? The person who applied approve can apply LGTM, but the person who LGTM cannot approve. Without stickness differences the LGTM label is just noise on top of the actual approval.

If you drop LGTM from the merge requirements today, you now have approve as a merge requirement. If you add sticky LGTM you effectively still just need an approver to approve for merge to happen.

LGTM really only exists as a merge mechanism to make sure all changes have been reviewed by someone. Anything else is a social construct not a mechanical one and for those we have /hold etc.

hklai commented 6 years ago

If you drop LGTM from the merge requirements today, you now have approve as a merge requirement. If you add sticky LGTM you effectively still just need an approver to approve for merge to happen.

@BenTheElder, if a PR is sent by an owner, she supposedly needs only an LGTM, not approval. And also more people can give her LGTM.

but at the same time, while it's OK to have the sticky Approve, the sticky LGTM can cause you a PR merged, before all the desired changes are applied.

after all, the moment both labels are present the bot will try to merge unless you put a hold on that PR.

@neolit123, Right, LGTM should not be given if the PR requires significant changes, or simply does not look good at all. The annoyance we are trying to address is the need to re-LGTM after the author fixes linter, shellcheck, or maybe test failures. Today, also partly due to the noisy github emails, many PRs are left in this state as reviewers have stopped paying attention to the PRs after LGTM.

So, this change effectively "upgrades" LGTM to mean that your PR looks good and can be merged as long as you (a collaborator) can make CI happy (so the bot can merge).

neolit123 commented 6 years ago

Right, LGTM should not be given if the PR requires significant changes, or simply does not look good at all. The annoyance we are trying to address is the need to re-LGTM after the author fixes linter, shellcheck, or maybe test failures

this is also an issue in k8s - the requirement for LGTM re-apply, but it's also a protection measure. the non-sticky LGTM ensures that a merge is only possible if another set of eyes verify the validity of the final change.

if you think Istio can handle such situations, so be it. :) perhaps once the project membership grows you'd want to switch this off again. ...or is the idea for this to only apply to collaborators??

So, this change effectively "upgrades" LGTM to mean that your PR looks good and can be merged as long as you can make CI happy (so the bot can merge).

for a project of the size of k8s, one might qualify this as a security downgrade, given the relatively low membership bar that we have. :)

hklai commented 6 years ago

if you think Istio can handle such situations, so be it. :) perhaps once the project membership grows you'd want to switch this off again. ...or is the idea for this to only apply to collaborators??

Yes. The plan is to implement an option that applies to collaborators only. If the PR author is not a collaborator, the existing LGTM rules apply.

for a project of the size of k8s, one might qualify this as a security downgrade, given the relatively low membership bar that we have. :)

Istio currently maintains a fairly high bar for membership, based on contribution. We review new member applications weekly in TOC meetings.

neolit123 commented 6 years ago

Yes. The plan is to implement an option that applies to collaborators only. If the PR author is not a collaborator, the existing LGTM rules apply.

collaborators sounds an order of magnitude better compared to members, yet collaborators are not perfect either.

looks like the related PR is receiving reviews already.

cjwagner commented 6 years ago

The approve plugin can be configured to require explicit self approval so that code owners do not automatically approve their own PRs: https://github.com/kubernetes/test-infra/blob/2c26f647f17a89acb9d299a83e1336034156361b/prow/plugins/plugins.go#L352-L354

hklai commented 6 years ago

To re-iterate why approve plugin alone is not sufficient:

  1. lgtm and approval mean different things, and they are meant for different roles.
  2. An approver needs another pair of eyes to make sure her PR looks good. Her PR should not be merged automatically (when ImplicitSelfApprove is ON or a self supplied /approve when ImplicitSelfApprove is off)
  3. There are more people who can /lgtm than those who can /approve. Even if we somehow disallow self approve, it can be much harder for an approver to seek approval from another approver. It will greatly degrade approver's experience in this case.
  4. A non-approver seek /lgtm and /approve not necessarily from the same guy. Sometimes /lgtm is given by developers who are more familiar with that domain.