Test-infra: prow: should the bot process comments from itself?

[cblecker]

Should the bot process comments that come from itself? Right now a couple plugins (sigmention, trigger, and soon project) have checks that if the comment comes from the bot, ignore it. Should this be something across the board?

There's a potential for functionality breaks:

• Someone using something in a way we don't intend
• Folks using the same token for a commenter, that they use for their main prow bot

But on the positive side:

• Would prevent loops where the bot keeps reacting to comments it makes and tripping up DoS protections on GitHub or using up all it's tokens
• Prevents potential privilege escalation where some exploits some flaw, and makes the bot comment with a privileged action that the user wouldn't otherwise have access to.

/area prow
/kind design
/cc @kubernetes/sig-testing

[cblecker]

ref: https://kubernetes.slack.com/archives/CDECRSC5U/p1554836885217700

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[spiffxp]

/assign @fejta @cjwagner @stevekuznetsov
Yes IMO prow should be ignoring comments and events generated by itself across the board, ideally before events even flow from hook to plugins. I didn't realize this was implemented on a case-by-case basis?

[cjwagner]

The fact that this is case by case is the main problem IMO. I'd be ok with prow ignoring its own comments or respecting them as long as we're consistent.
I just noticed the /cc in the autobump PR isn't respected because the bot creates the PR: https://github.com/kubernetes/test-infra/pull/13586#issue-300856456

[spiffxp]

/priority important-longterm

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[BenTheElder]

probably should make a decision or close this as defacto "yes"

[cblecker]

/remove-lifecycle stale

I would assert: prow should not respond to comments left by itself.

[fejta]

/area prow
/area prow/hook

IMO hook should understand who it is and ignore comments from itself -- don't send them to plugins.

This shouldn't be something plugins can mess up.

[cblecker]

I agree.

Okay, so the plan is:

• Any comment events (issue_comment, pull_request_review, pull_request_review_comment, GenericCommentEvent) where the user is the bot, should be dropped by hook before they get to the plugins
• We should not drop all events, because we expect the bot to open PRs/issues in certain circumstances (such as using the cherry pick plugin, or the prow image bumper). Events like PR synchronize should still work.
• An existing implementation inside a plugin to use as an example is here: https://github.com/kubernetes/test-infra/blob/80dde4258c653a982aefda789574cb2fb2a058c5/prow/plugins/sigmention/sigmention.go#L78-L85
• While this change isn't breaking any existing configs, it is an important change of functionality, and something should be added to ANNOUCEMENTS.md accordingly.

Once this is implemented, we can remove the extra logic from any plugins that use it.

As we now have a clear plan for this:
/help

[k8s-ci-robot]

@cblecker:
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:

I agree.

Okay, so the plan is:

• Any comment events (issue_comment, pull_request_review, pull_request_review_comment, GenericCommentEvent) where the user is the bot, should be dropped by hook before they get to the plugins
• We should not drop all events, because we expect the bot to open PRs/issues in certain circumstances (such as using the cherry pick plugin, or the prow image bumper)
• An existing implementation inside a plugin to use as an example is here: https://github.com/kubernetes/test-infra/blob/80dde4258c653a982aefda789574cb2fb2a058c5/prow/plugins/sigmention/sigmention.go#L78-L85
• While this change isn't breaking any existing configs, it is an important change of functionality, and something should be added to ANNOUCEMENTS.md accordingly.

Once this is implemented, we can remove the extra logic from any plugins that use it.

As we now have a clear plan for this:
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[cjwagner]

It might be good to have a period of time where we check the events we would filter out for anything that looks like a slash command and emit warnings that the commands will soon be ignored.

[cblecker]

Oh, so like, we have a option for hook where we use the same above logic, but emit a warning with an opt-in? And then remove the option, with the new default of drop the event?

I'm _okay_ with that, but also not convinced it's needed. Right now I don't know of any plugins that explicitly use this code path.

[cjwagner]

Thats basically what I'm suggesting except I don't see much point in adding an option unless we want to allow Prow instances to opt-out of ignoring bot comments. Just warning for a bit before actually filtering out the events seems fine to me.

We certainly don't _need_ it, but it would let us (and other Prow instances) know if anything is going to break before we break it. I haven't looked around much, but a quick search shows that there are at least some usages of this functionality that would break on the prow.k8s.io instance. See the /cc command here and the corresponding plugin here.

[fejta]

Long term I would argue in favor of hook always ignoring webhooks from itself. This is the simplest design.

The bump script should just directly assign Katharine -- it obviously has the ACLs to do this or else it couldn't do so in the plugin. I suspect every example will have a similar solution.

Short term I'm in favor of continuing to allow it during a migration period.

[cblecker]

If it ignores webhooks from itself, would it even start testing when the PR is created?

[BenTheElder]

perhaps it should ignore comments from itself but not PR sychronize events?
the bot doesn't need to post /test to itself, but if it pushes code we
should trigger tests :-)

On Wed, Oct 30, 2019 at 1:06 PM Christoph Blecker [e-mail]
wrote:

If it ignores webhooks from itself, would it even start testing when the
PR is created?

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/12138?email_source=notifications&email_token=AAHADKZA7OGQIY5VHGXU5HDQRHSMNA5CNFSM4HEU6MZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECVS52Y#issuecomment-548089579,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAHADK4XYSSYPTXO7UPRP5DQRHSMNANCNFSM4HEU6MZA
.

[cblecker]

That's my original suggestion, just said a million times clearer 😆

[fejta]

Great point and 100% agree

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[cblecker]

/remove-lifecycle rotten

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[cblecker]

/reopen
/remove-lifecycle rotten

[k8s-ci-robot]

@cblecker: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle rotten

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten