Bug 540694 - Github IP validation needs to be more robust
Summary: Github IP validation needs to be more robust
Status: RESOLVED FIXED
Alias: None
Product: Community
Classification: Eclipse Foundation
Component: GitHub (show other bugs)
Version: unspecified   Edit
Hardware: PC Linux
: P1 critical (vote)
Target Milestone: ---   Edit
Assignee: Christopher Guindon CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 536051 540008 541057 (view as bug list)
Depends on:
Blocks: 496885 498340 514758 521596 534451 539760
  Show dependency tree
 
Reported: 2018-11-01 12:05 EDT by Christian Kaltepoth CLA
Modified: 2019-09-30 17:39 EDT (History)
13 users (show)

See Also:


Attachments
Screenshot (61.89 KB, image/png)
2018-11-01 12:05 EDT, Christian Kaltepoth CLA
no flags Details
Screenshot: Github UI (181.55 KB, image/png)
2018-12-11 13:03 EST, Christopher Guindon CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Kaltepoth CLA 2018-11-01 12:05:28 EDT
Created attachment 276446 [details]
Screenshot

Dear webmaster team,

for some reason the 'ip-validation' check of the jaxrs-api project seems hang in the status 'Expected — Waiting for status to be reported'. See attached screenshot.

https://github.com/eclipse-ee4j/jaxrs-api/pull/686

Any idea what is causing this? It is not the first time this is happening. I already saw this error yesterday. So I rebased the PR and did a force push to trigger the check again, but it still doesn't proceed into the next state.

Is there anything we can do to work around this?
Comment 1 Christian Kaltepoth CLA 2018-11-08 02:02:04 EST
I fixed the first pull request by doing an interactive rebase and rewording all the commit messages. A forced push then fixed the problem.

However, here is the next broken check:

https://github.com/eclipse-ee4j/jaxrs-api/pull/696

This check is also in "Expected — Waiting for status to be reported" state for more than an hour now.
Comment 2 Christian Kaltepoth CLA 2018-11-18 05:29:44 EST
In the meantime we fixed the second build by doing another rebase. Anyway, it would be great if this issue could be addressed.
Comment 3 Markus Karg CLA 2018-12-02 12:23:20 EST
I want to amend that the hanging ip validation checks on Github are an ongoing annoying problem. It happens rather often and is a really huge pain for committers. This is not just a rare effect, it is really happening OFTEN.

Also I think it is really a shame that an IT organization like us is unable to simply provide a fix after months. This is just ridiculous. Instead of asking committers to forcefully push empty commits to trigger the hanging job, the EF admins should start to search for the actual cause and finally fix it. It is absolutely unacceptable that we simply ask people to re-push instead of observing the original cause. As projects might have push protection rules in place (like JAX-RS), a real solution is really needed ASAP.

Please don't further ignore this bug but hear the voice of annoyed committers and start to fix it. As we do not have access to the source and config of this service, we cannot fix it on ouw won. So we urge you to help us.
Comment 4 Denis Roy CLA 2018-12-03 11:58:30 EST
My apologies for the absence of communication. We are aware of the situation. We're also aware that the code repo is the heartbeat of the community.

The current IP Validation hook was written long ago. It is brittle and unnecessarily complex, lacks proper logging, monitoring and error reporting.  But yes, it is open source: https://github.com/eclipse/eclipse-webhook

In the last year we've attempted to fix it, but our efforts have failed. Our WebDev team is taking over the project and will develop a new IP validator based on the latest GitHub API, using a more robust framework. They hope to have a a new validator to test, starting Dec 14. Roll out to all projects will be seamless.

Just to be clear: we take *all* incoming bugs very seriously.

Apologies for the lost productivity, and thank you for your patience. This will be fixed soon.
Comment 5 Denis Roy CLA 2018-12-03 11:59:29 EST
*** Bug 541057 has been marked as a duplicate of this bug. ***
Comment 6 Denis Roy CLA 2018-12-03 11:59:50 EST
*** Bug 540008 has been marked as a duplicate of this bug. ***
Comment 7 Denis Roy CLA 2018-12-03 12:30:32 EST
*** Bug 536051 has been marked as a duplicate of this bug. ***
Comment 8 Christian Kaltepoth CLA 2018-12-03 23:59:10 EST
Thanks a lot for the update. We are happy to hear that you are working on this. Looking forward to the new version of the validation hook.
Comment 9 James Roper CLA 2018-12-04 00:12:56 EST
I've been maintaining a GitHub IP validator webhook for my company for the past 6 years, and ours runs almost completely without issue (it validates all contributions to Scala, Akka, Play Framework and many other Lightbend run open source projects). If you have any questions about how this can be implemented robustly, I'm happy to help.
Comment 10 Christopher Guindon CLA 2018-12-04 09:37:49 EST
(In reply to James Roper from comment #9)
> I've been maintaining a GitHub IP validator webhook for my company for the
> past 6 years, and ours runs almost completely without issue (it validates
> all contributions to Scala, Akka, Play Framework and many other Lightbend
> run open source projects). If you have any questions about how this can be
> implemented robustly, I'm happy to help.

Hi James,

is your code available on github? I am interested to see how you are doing this since I will be implementing the new github validation hook.
Comment 11 James Roper CLA 2018-12-04 18:40:36 EST
It's in a private repo - while there's no reason I can't share the code for the validator, there are other things in that repo which can't be shared. What I'll do is copy the code that implements the commit hook and put it in a gist, I'll do that later today. It's implemented in Scala, running in a Play Framework app, hopefully that's ok for you.
Comment 12 Christopher Guindon CLA 2018-12-11 13:03:01 EST
Created attachment 276905 [details]
Screenshot: Github UI

(In reply to James Roper from comment #11)
> It's in a private repo - while there's no reason I can't share the code for
> the validator, there are other things in that repo which can't be shared.
> What I'll do is copy the code that implements the commit hook and put it in
> a gist, I'll do that later today. It's implemented in Scala, running in a
> Play Framework app, hopefully that's ok for you.

That works for me! 

I have a quick question for you:

I created a test repo on my account to test GitHub Status API:
https://github.com/chrisguindon/eclipse-eca-test-repo/

The Github UI will inform the user that "All checks have passed" if the last commit in the pull request has a "success" status. 

How can I make the IP validation check fail if one of the commits in the pull request does not pass validation?

I am thinking that I could solve this problem by validating the parent of each commit. I believe this will make all subsequent commits fail the IP validation. 

Before I move forward with this solution, I would like to know if you found an easier way to achieve this?
Comment 13 Christopher Guindon CLA 2018-12-13 17:45:48 EST
A pull request might have multiple commits by different contributor who's ECA status might vary:
https://github.com/chrisguindon/eclipse-eca-test-repo/pull/28
https://github.com/chrisguindon/eclipse-eca-test-repo/pull/35

From what I can tell, github will display to the end-user that "All checks have passed" based on the status check of the last commit.

I wanted to display in the github.com UI which commit did not pass IP by updating the status check of each commit, each time the pull request is updated based off the IP validation results.

Unfortunately, I don't think this how the status check should work. Instead, each time the pull request is updated, I should set/update the status check of the last commit.

The status check should validate the author of each commit in the pull request and it should return an error if 1 of the author in one of the commit has failed the IP validation. The detail link should explain to the user the reason why/who failed the IP validation.

My prototype is currently setting the "pending" state to each commit after a commit with an error but I don't think this is really useful to the end-user:
https://github.com/chrisguindon/eclipse-eca-test-repo/pull/35

I will revert some of those changes and simply focus on updating the last commit of a pull request.

If I am looking at this wrong, please let me know!

Cheers,
Comment 14 Christopher Guindon CLA 2019-01-11 15:27:08 EST
Hi All,

Just wanted to post a quick update on this project. We plan on rolling out the new ECA validation tool on January 21st for all Eclipse Spec projects.

Once this is done, we will make additional changes to this new tool that will support all Eclipse projects. 

After that, we will begin to roll this out to a subset of Eclipse projects before a full deployment.
Comment 15 Karsten Thoms CLA 2019-01-29 09:00:55 EST
> After that, we will begin to roll this out to a subset of Eclipse projects
> before a full deployment.

Project Xtext is interested to test this.
Comment 16 Christopher Guindon CLA 2019-02-15 13:10:16 EST
Hi All,

I made available our new Eclipse ECA Validation GitHub App and it's ready to be tested by the community:
https://github.com/apps/eclipse-eca-validation

Our current plan is to install this new App across all our GitHub Organizations/Repos within a week or so if the feedback from the community is positive and we don't find any major bug with the App.

This App will consider everyone a contributor if the App is installed on a non-eclipse project repository and every contributor is required to include the The sign-off-by footer.

Please comment on this bug to submit feedback!
Comment 17 Eclipse Webmaster CLA 2019-02-15 16:24:37 EST
(In reply to Karsten Thoms from comment #15)
> > After that, we will begin to roll this out to a subset of Eclipse projects
> > before a full deployment.
> 
> Project Xtext is interested to test this.

Ok we've added this to github.com/eclipse/xtext so you can kick the tires.

-M.
Comment 18 Christopher Guindon CLA 2019-02-18 08:19:28 EST
(In reply to Eclipse Webmaster from comment #17)
> (In reply to Karsten Thoms from comment #15)
> > > After that, we will begin to roll this out to a subset of Eclipse projects
> > > before a full deployment.
> > 
> > Project Xtext is interested to test this.
> 
> Ok we've added this to github.com/eclipse/xtext so you can kick the tires.
> 
> -M.

We are hoping to get some feedback on this new ECA Validation service before we roll this out all our Eclipse Project Github repositories.

If there is another project interested in testing this service, please let us know on this bug and we will install the App on your Github repos!

Thanks!
Comment 19 Christopher Guindon CLA 2019-02-21 13:21:57 EST
So far the testing period allowed us to find and fix two bugs:

1. The page title of the details page had a double encoding issue:
https://accounts.eclipse.org/legal/eca/validation/72

2. The detail page did not remove authors that where removed from a pull request.

Fixes for both issues has been deployed to production.
Comment 20 Christopher Guindon CLA 2019-02-25 09:58:21 EST
(In reply to Christopher Guindon from comment #19)
> So far the testing period allowed us to find and fix two bugs:
> 
> 1. The page title of the details page had a double encoding issue:
> https://accounts.eclipse.org/legal/eca/validation/72
> 
> 2. The detail page did not remove authors that where removed from a pull
> request.
> 
> Fixes for both issues has been deployed to production.

@webmaster, 

It was decided that we should go live with our new IP validation Github App this week.

Could you install our new Github app across all our Eclipse projects repos and remove the old one this week?
Comment 21 Eclipse Webmaster CLA 2019-02-26 15:53:23 EST
(In reply to Christopher Guindon from comment #20)

> Could you install our new Github app across all our Eclipse projects repos
> and remove the old one this week?

I've installed it for all repos in the Eclipse organization, and will start rolling it to our other orgs shortly.

After that I'll start removing the old webhook.

-M.
Comment 22 Eclipse Webmaster CLA 2019-02-27 14:07:02 EST
Ok I've rolled out the Github app to our other orgs and have removed the old webhook.

I think that finishes my part in this.

-M
Comment 23 Holger Voormann CLA 2019-03-04 07:19:56 EST
What should be entered as the "Effective Day"?

What are the committer's initials? My GitHub account name?

The text as images is not searchable and is probably not accessible to all. Is this an issue of HelloSign?
Comment 24 Christopher Guindon CLA 2019-03-04 09:30:07 EST
(In reply to Holger Voormann from comment #23)
> What should be entered as the "Effective Day"?
> 
> What are the committer's initials? My GitHub account name?
> 
> The text as images is not searchable and is probably not accessible to all.
> Is this an issue of HelloSign?

Hi Holger,

Are you refereeing to the New Eclipse Foundation Committer and Contributor Agreements? 
https://blogs.eclipse.org/post/mike-milinkovich/new-eclipse-foundation-committer-and-contributor-agreements

If so, I would recommend that you get in contact with license@eclipse.org with your question!
Comment 25 Markus Karg CLA 2019-03-04 13:17:49 EST
The new IP Validation Bot says that a JAX-RS contributor has a valid ECA in general, but misses the requirements for a Specification Contributor. Can you post a link to these additional requirements so contributors know what additional paperwork they have to file?
Comment 26 Christopher Guindon CLA 2019-03-04 13:52:18 EST
(In reply to Markus Karg from comment #25)
> The new IP Validation Bot says that a JAX-RS contributor has a valid ECA in
> general, but misses the requirements for a Specification Contributor. Can
> you post a link to these additional requirements so contributors know what
> additional paperwork they have to file?

When contributing to an Eclipse Foundation Specification Project, contributors must be covered with version 3.0.0 or greater of the ECA.

https://blogs.eclipse.org/post/christopher-guindon/eclipse-foundation-contributor-validation-service

Do you have a link to the failed validation?
Comment 27 Markus Karg CLA 2019-03-04 14:04:07 EST
(In reply to Christopher Guindon from comment #26)
> (In reply to Markus Karg from comment #25)
> > The new IP Validation Bot says that a JAX-RS contributor has a valid ECA in
> > general, but misses the requirements for a Specification Contributor. Can
> > you post a link to these additional requirements so contributors know what
> > additional paperwork they have to file?
> 
> When contributing to an Eclipse Foundation Specification Project,
> contributors must be covered with version 3.0.0 or greater of the ECA.
> 
> https://blogs.eclipse.org/post/christopher-guindon/eclipse-foundation-
> contributor-validation-service
> 
> Do you have a link to the failed validation?

https://accounts.eclipse.org/legal/eca/validation/513
Comment 28 Holger Voormann CLA 2019-03-04 14:11:42 EST
(In reply to Christopher Guindon from comment #24)
> (In reply to Holger Voormann from comment #23)
> > What should be entered as the "Effective Day"?
> > 
> > What are the committer's initials? My GitHub account name?
> > 
> > The text as images is not searchable and is probably not accessible to all.
> > Is this an issue of HelloSign?
> 
> Hi Holger,
> 
> Are you refereeing to the New Eclipse Foundation Committer and Contributor
> Agreements? 
> https://blogs.eclipse.org/post/mike-milinkovich/new-eclipse-foundation-
> committer-and-contributor-agreements
> 
> If so, I would recommend that you get in contact with license@eclipse.org
> with your question!

Yes, in my case it is the HelloSign Eclipse Individual Committer Agreement (here as PDF: https://www.eclipse.org/legal/committer_process/EclipseIndividualCommitterAgreement.pdf).

I wonder why the date is not automatically filled in like in the footer (if it is the current date) and why the committer's initials are required.

Anyway, I don't want to spend much time on it and just entered the current date and "HV" as committer's initials.
Comment 29 Christopher Guindon CLA 2019-03-04 14:13:28 EST
(In reply to Markus Karg from comment #27)
> (In reply to Christopher Guindon from comment #26)
> > (In reply to Markus Karg from comment #25)
> > > The new IP Validation Bot says that a JAX-RS contributor has a valid ECA in
> > > general, but misses the requirements for a Specification Contributor. Can
> > > you post a link to these additional requirements so contributors know what
> > > additional paperwork they have to file?
> > 
> > When contributing to an Eclipse Foundation Specification Project,
> > contributors must be covered with version 3.0.0 or greater of the ECA.
> > 
> > https://blogs.eclipse.org/post/christopher-guindon/eclipse-foundation-
> > contributor-validation-service
> > 
> > Do you have a link to the failed validation?
> 
> https://accounts.eclipse.org/legal/eca/validation/513

For this example, this repo is not flagged as an Eclipse Foundation Specification Project, therefore the standard validation rules are in effect.

I think the issue here is that the email address that we have for this contributor was not synchronized correctly in our Foundation DB.

I will see what I can do to fix this problem.
Comment 30 Christopher Guindon CLA 2019-03-04 14:31:08 EST
(In reply to Christopher Guindon from comment #29)
> (In reply to Markus Karg from comment #27)
> > (In reply to Christopher Guindon from comment #26)
> > > (In reply to Markus Karg from comment #25)
> > > > The new IP Validation Bot says that a JAX-RS contributor has a valid ECA in
> > > > general, but misses the requirements for a Specification Contributor. Can
> > > > you post a link to these additional requirements so contributors know what
> > > > additional paperwork they have to file?
> > > 
> > > When contributing to an Eclipse Foundation Specification Project,
> > > contributors must be covered with version 3.0.0 or greater of the ECA.
> > > 
> > > https://blogs.eclipse.org/post/christopher-guindon/eclipse-foundation-
> > > contributor-validation-service
> > > 
> > > Do you have a link to the failed validation?
> > 
> > https://accounts.eclipse.org/legal/eca/validation/513
> 
> For this example, this repo is not flagged as an Eclipse Foundation
> Specification Project, therefore the standard validation rules are in effect.
> 
> I think the issue here is that the email address that we have for this
> contributor was not synchronized correctly in our Foundation DB.
> 
> I will see what I can do to fix this problem.

We updated the contributor email address in the FoundationDB to match what we have on accounts.eclipse.org and re-triggered a validation for this pull request.

The validation was a success.
Comment 31 Markus Karg CLA 2019-03-05 01:36:55 EST
(In reply to Christopher Guindon from comment #30)
> (In reply to Christopher Guindon from comment #29)
> > (In reply to Markus Karg from comment #27)
> > > (In reply to Christopher Guindon from comment #26)
> > > > (In reply to Markus Karg from comment #25)
> > > > > The new IP Validation Bot says that a JAX-RS contributor has a valid ECA in
> > > > > general, but misses the requirements for a Specification Contributor. Can
> > > > > you post a link to these additional requirements so contributors know what
> > > > > additional paperwork they have to file?
> > > > 
> > > > When contributing to an Eclipse Foundation Specification Project,
> > > > contributors must be covered with version 3.0.0 or greater of the ECA.
> > > > 
> > > > https://blogs.eclipse.org/post/christopher-guindon/eclipse-foundation-
> > > > contributor-validation-service
> > > > 
> > > > Do you have a link to the failed validation?
> > > 
> > > https://accounts.eclipse.org/legal/eca/validation/513
> > 
> > For this example, this repo is not flagged as an Eclipse Foundation
> > Specification Project, therefore the standard validation rules are in effect.
> > 
> > I think the issue here is that the email address that we have for this
> > contributor was not synchronized correctly in our Foundation DB.
> > 
> > I will see what I can do to fix this problem.
> 
> We updated the contributor email address in the FoundationDB to match what
> we have on accounts.eclipse.org and re-triggered a validation for this pull
> request.
> 
> The validation was a success.

Works! Thanks a lot! :-)
Comment 32 Mikaël Barbero CLA 2019-03-12 04:30:23 EDT
(In reply to Eclipse Webmaster from comment #22)
> Ok I've rolled out the Github app to our other orgs and have removed the old
> webhook.
> 
> I think that finishes my part in this.
> 
> -M

It seems that the hook can still be triggered by "Branch protection rule". See 544974 comment #9 for an example. 

All repos should be checked for similar usage.
Comment 33 Eclipse Webmaster CLA 2019-03-12 15:36:33 EDT
(In reply to Mikaël Barbero from comment #32)

> All repos should be checked for similar usage.

I found 11 repos that were using branch protection that referenced the old webhook, so I updated them.

-M.
Comment 34 Erik Boasson CLA 2019-05-08 10:38:36 EDT
Hi,

Cyclone DDS pull request #177 (https://github.com/eclipse-cyclonedds/cyclonedds/pull/177) makes me think the IP check sometimes ignores some authors in a PR.  This PR contains rather many commits that are simply bringing the destination branch in line with master again, but there are also some new ones at the end and those are the ones that appear to be ignored.

For example, the currently most recent commit on this PR is (I've copied the relevant bit for reference):

commit 5886b86110a655bbf58b044614d35314d0929bda (HEAD -> security-kurtulus)
Author: Kurtulus Oksuztepe <kurtulus.oksuztepe@adlinktech.com>
Date:   Wed May 8 15:40:20 2019 +0200

    CYC-203 DDS Security core library removed, review logs were applied

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index d1539dd..48daaa2 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
[...]

obviously, the commit message is missing the "Signed-off-by" footer, and yet the check passes.  The interesting thing is that the Details page (https://accounts.eclipse.org/legal/eca/validation/4239) says:

[checkmark] Author(s) covered by necessary legal agreements
* Erik Boasson (eb@****.com)
* Martin Bremmer (martin.bremmer@****.com)
* Jeroen Koekkoek (jeroen@****.nl)

where the author of the above commit is missing.  Manual validation says the necessary agreements are currently not on file.

I didn't see it as a known issue in the comments below, but it seems rather serious to me.
Comment 35 Christopher Guindon CLA 2019-05-08 17:23:33 EDT
(In reply to Erik Boasson from comment #34)
> Hi,
> 
> Cyclone DDS pull request #177
> (https://github.com/eclipse-cyclonedds/cyclonedds/pull/177) makes me think
> the IP check sometimes ignores some authors in a PR.  This PR contains
> rather many commits that are simply bringing the destination branch in line
> with master again, but there are also some new ones at the end and those are
> the ones that appear to be ignored.
> 
> For example, the currently most recent commit on this PR is (I've copied the
> relevant bit for reference):
> 
> commit 5886b86110a655bbf58b044614d35314d0929bda (HEAD -> security-kurtulus)
> Author: Kurtulus Oksuztepe <kurtulus.oksuztepe@adlinktech.com>
> Date:   Wed May 8 15:40:20 2019 +0200
> 
>     CYC-203 DDS Security core library removed, review logs were applied
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index d1539dd..48daaa2 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> [...]
> 
> obviously, the commit message is missing the "Signed-off-by" footer, and yet
> the check passes.  The interesting thing is that the Details page
> (https://accounts.eclipse.org/legal/eca/validation/4239) says:
> 
> [checkmark] Author(s) covered by necessary legal agreements
> * Erik Boasson (eb@****.com)
> * Martin Bremmer (martin.bremmer@****.com)
> * Jeroen Koekkoek (jeroen@****.nl)
> 
> where the author of the above commit is missing.  Manual validation says the
> necessary agreements are currently not on file.
> 
> I didn't see it as a known issue in the comments below, but it seems rather
> serious to me.

+1 Verified!

The validation tool does not properly parse request with commits with multiple pages:

Link: <https://api.github.com/repositories/116033455/pulls/177/commits?page=2>; rel="next", <https://api.github.com/repositories/116033455/pulls/177/commits?page=3>; rel="last

I am currently working on a fix for this now!
Comment 36 Christopher Guindon CLA 2019-05-08 19:34:09 EDT
@Erik,

I made an update to the ECA validation tool. Could you tell me if the detail page properly display the result that you are expecting?

https://accounts.eclipse.org/legal/eca/validation/4239
Comment 37 Erik Boasson CLA 2019-05-08 20:21:02 EDT
Hi Christopher,

Yes, that's exactly what I expected to see.

Thanks!
Erik
Comment 38 Christopher Guindon CLA 2019-09-30 17:39:09 EDT
Known issues listed on Bug 496885 - Github IP validation incorrectly fails on merge commit were deployed to production today.

Closing this issue.