Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

github-bot: add IPs of Jenkins workers #985

Merged
merged 3 commits into from
Nov 27, 2017

Conversation

phillipj
Copy link
Member

@phillipj phillipj commented Nov 7, 2017

Providing $JENKINS_WORKER_IPS environment variable at startup will activate the whitelist implemented in the bot, validating who's allowed to pushed Jenkins job updates as inline PR statuses on github.com.

Refs nodejs/github-bot#142

/cc @nodejs/github-bot

@phillipj phillipj mentioned this pull request Nov 7, 2017
maclover7

This comment was marked as off-topic.

@phillipj
Copy link
Member Author

phillipj commented Nov 8, 2017

@maclover7 thanks! Pushed a commit adding a comment with a friendly reminder.

maclover7

This comment was marked as off-topic.

@maclover7
Copy link
Contributor

@phillipj This should be ready to go, right? Would you be able to merge this, and check and see if it helps at all with nodejs/node#17151 (comment)?

@phillipj
Copy link
Member Author

@maclover7 I'm not able to immediately, but I'll at least have a look tomorrow.

I'm unsure if it would help any issues ATM, if anything probably make it worse? Currently it will happily respond to requests from anywhere, having this whitelist activated could if anything, make it worse? Or have I misunderstood the issue in the PR you referenced?

joyeecheung

This comment was marked as off-topic.

@rvagg
Copy link
Member

rvagg commented Nov 25, 2017

LGTM, although it should be possible to insert some python in plugins/inventory/nodejs_yaml.py to pull out the IP addresses with the jenkins-workspace alias and insert them into a new context variable that you can then use in setup/github-bot/resources/environment-file. If you can't figure out how to do this then this change is a good first step. There's a checklist on the README in this ansible directory where you could add a TODO for doing this work.

But as I've mentioned in nodejs/node#17151 (comment) I think the effect of this on non-pipeline jobs is that it's going to stop status reporting completely because the curling is happening on worker machines, not the workspace ones that are exclusively used by Jenkins.

@phillipj do you have access to update the bot machine with this change?

@phillipj
Copy link
Member Author

@phillipj do you have access to update the bot machine with this change?

Yepp. I'll merge this and update the env variables on the machine out shortly.

phillipj added a commit to phillipj/build that referenced this pull request Nov 27, 2017
Instead of keeping the list of whitelisted Jenkins worker IPs up-to-date
manually, it would be ideal to automate that with a python script
instead.

Refs nodejs#985 (comment)
Providing `$JENKINS_WORKER_IPS` environment variable at startup will
activate the whitelist implemented in the bot, validating who's allowed
to pushed Jenkins job updates as inline PR statuses on github.com.

Refs: nodejs/github-bot#142
PR-URL: nodejs#985
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
The github-bot has a whitelist of IPs that's allowed to push Jenkins
job status as inline PR status on github.com. Whenever we change our
Jenkins worker IPs, we should remember to update the bot's whitelist
as well.

PR-URL: nodejs#985
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Instead of keeping the list of whitelisted Jenkins worker IPs up-to-date
manually, it would be ideal to automate that with a python script
instead.

Refs: nodejs#985 (comment)
PR-URL: nodejs#985
@phillipj phillipj merged commit c065634 into nodejs:master Nov 27, 2017
phillipj added a commit that referenced this pull request Nov 27, 2017
Providing `$JENKINS_WORKER_IPS` environment variable at startup will
activate the whitelist implemented in the bot, validating who's allowed
to pushed Jenkins job updates as inline PR statuses on github.com.

Refs: nodejs/github-bot#142
PR-URL: #985
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
phillipj added a commit that referenced this pull request Nov 27, 2017
The github-bot has a whitelist of IPs that's allowed to push Jenkins
job status as inline PR status on github.com. Whenever we change our
Jenkins worker IPs, we should remember to update the bot's whitelist
as well.

PR-URL: #985
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@phillipj phillipj deleted the add-gh-bot-jenkins-ips branch November 27, 2017 09:05
@phillipj
Copy link
Member Author

@maclover7 this is live in production now.

phillipj added a commit to phillipj/github-bot that referenced this pull request Nov 27, 2017
After the Jenkins IP whitelist was activated, the inline PR status
reports for nodejs/node seems to be broken.

Seems like the issue here is that IPv4 remote addresses are converted
to IPv6, which wasn't expected too happen.. Stripping any IPv6 parts
off the remote addresses should make our IPv4 comparisons work as
expected.

Refs nodejs/build#1016
Refs nodejs/build#985
phillipj added a commit to nodejs/github-bot that referenced this pull request Nov 27, 2017
After the Jenkins IP whitelist was activated, the inline PR status
reports for nodejs/node seems to be broken.

Seems like the issue here is that IPv4 remote addresses are converted
to IPv6, which wasn't expected too happen.. Stripping any IPv6 parts
off the remote addresses should make our IPv4 comparisons work as
expected.

Refs nodejs/build#1016
Refs nodejs/build#985
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants