Dynamically get Travis VM IP addresses for authentication about amp-github-apps HOT 24 CLOSED

We're not doing the check because this issue is still open... this should still be fixed so we can reactivate the check

from amp-github-apps.

Unfortunately Travis doesn't provide any other way to validate that requests come from their jobs (unless we jump through hoops of going back and forth between APIs, but this seemed like an overkill to me). They recommend looking up their IP address as a way to validate

I updated both apps with the latest list from Travis and signed up to the mailing list to get updates when they change those. If you have any other ideas how to fix this, let me know :/

from amp-github-apps.

Thanks Daniel! Subscribing to IP address changes seems fine to me. I might have to do something similar for the pr-deploy bot so I'll let you know if I find an alternate solution.

from amp-github-apps.

Haven't seen this in a while.

from amp-github-apps.

After discussing this offline - Travis suggests using an API endpoint to get the latest list of IP addresses. See https://docs.travis-ci.com/user/ip-addresses/

This should be fixed on both the bundle-size and test-status apps

from amp-github-apps.

The pr-deploy bot could use it too - can this be a shared service?

from amp-github-apps.

from amp-github-apps.

So, we need to nail down if we want A) a shared service or B) an API call on-demand.

Arguments in favor of a shared service:

• Single source of truth
• No risk of hitting some form of rate-limit
• Reduced dependency on a 3rd-party service being up
• No API request for every request by Travis

Arguments in favor of on-demand:

• Keep IP checking code very simple
• No dependency on a separate GAE app
• No Cloud Storage request for every request by Travis
• IP source always up-to-date

from amp-github-apps.

@danielrozenberg @rsimha @estherkim If y'all also have strong opinions here, might make sense to leave the gist as a comment so we can add/read asynchronously and we'll be able to skip a lot of back-and-forth whenever we do actually meet

from amp-github-apps.

Thanks for the summary @rcebulko!

I like option B) for these reasons:

1. Less complex and reduced dependencies - it doesn't rely on GAE/GCS/the app itself.
2. Uses the authoritative source (nat.travisci.net) in real time.
3. All logic lives in this repo / nodejs land - MUCH easier for anyone to maintain or debug.

With either option, we can reduce XHRs sent to nat.travsci.net or GCS by keeping an ip address cache in memory, or setting env vars.

from amp-github-apps.

from amp-github-apps.

Do we have an idea of roughly how many requests we get a day?

from amp-github-apps.

It seems really unlikely that we'll have any trouble with rate limits on DNS requests if we are doing one DNS lookup on Travis for each request Travis sends to us

from amp-github-apps.

Also +1 to an N-hour local cache in the apps that support it. I'm not sure which ones have persistent state and which are only spun up in response to webhooks (not familiar with Probot)

from amp-github-apps.

Also +1 to an N-hour local cache in the apps that support it. I'm not sure which ones have persistent state and which are only spun up in response to webhooks (not familiar with Probot)

Once a Probot app is deployed, it's up and running via GAE unless you take it down. Currently IP address ranges stored in node env variables and works well (until Travis changes them, hence this issue).

We should definitely at least use some caching strategy here. Calling a 3rd party on every request is not a good idea and might even be against their terms of service :/

Agreed. We could keep the ranges hard coded, and send an XHR only when an incoming IP address is unrecognized. In that single XHR we can authenticate the address and update the ranges. Given that this service will be updating accepted ranges, I'm unsure if node env is the best way to keep them.

from amp-github-apps.

+1 to update-on-invalid

from amp-github-apps.

@danielrozenberg Does this sound reasonable?

from amp-github-apps.

Update on invalid might be more complex than just having it run as a setInterval job that updates an in-memory cache every ~1 hour :)

from amp-github-apps.

Sure :) @rcebulko how does this sound? I know you pretty much created A) already but hopefully we can reuse it for something else in the future.

from amp-github-apps.

Yep SGTM. There's minimal benefit to keeping the rest of A) around, so I'll pull that out. If we ever want it for some reason, that's what Git is for :)

from amp-github-apps.

@estherkim Reviewing my open issues/trying to determine if this should be shelved; in your experience, roughly how often do Travis VM IPs change?

from amp-github-apps.

Pretty often unfortunately :/

from amp-github-apps.

@danielrozenberg Since we don't actually do this check anymore, is it reasonable to close this issue as obsolete?

from amp-github-apps.

We no longer use Travis so this isn't relevant

from amp-github-apps.