-
Notifications
You must be signed in to change notification settings - Fork 13
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
Spike on updating AWS IP ranges #5503
Spike on updating AWS IP ranges #5503
Conversation
Heroku app: https://gyr-review-app-5503-0040b9749b9f.herokuapp.com/ |
|
||
def update_aws_ip_ranges | ||
RemoteIpTrustedProxiesService.configure_trusted_proxies(RemoteIpTrustedProxiesService.load_current_aws_ip_ranges) | ||
Rails.logger.info("LOGGING AWS SNS SUBSCRIPTION CONFIRMATION FOR UPDATE_AWS_IP_RANGES WEBHOOK: #{params}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See SNS subscription confirmation steps here. We will remove this logline after we've set up the subscription & confirmed ownership of the URL in demo & prod.
An alternative to logging this out so that we can confirm ownership manually would be for the webhook controller to visit the confirmation URL automatically. I didn't implement that because this work already got blocked by enough other things and I'd like to not make it any more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much I can verify that this works, but it makes sense and seems reasonable
end | ||
|
||
def priority | ||
PRIORITY_MEDIUM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny dust - consider making this PRIORITY_LOW ? since it's running at startup there might be other jobs that are a higher priority, but idk if actually matters
@@ -395,6 +395,8 @@ def scoped_navigation_routes(context, navigation) | |||
post "/outgoing_email_status", to: "mailgun_webhooks#update_outgoing_email_status", as: :outgoing_email_status | |||
# OAuth login callback routes | |||
devise_for :users, path: "hub", only: :omniauth_callbacks, skip: [:session, :invitation], controllers: { omniauth_callbacks: "users/omniauth_callbacks" } | |||
# AWS IP ranges update trigger | |||
post "/update_aws_ip_ranges", to: "aws_ip_ranges_webhooks#update_aws_ip_ranges", as: :update_aws_ip_ranges |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a fyst route also? or only if we separate the deployments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided against it, since they share a server/configuration and this is where webhook routes live currently.
… latter doesn't work with AWS' subscription confirmation request
This adds a webhook action which loads the current AWS IP ranges. The webhook will be called both at server initialization and by an AWS SNS subscription whenever the AWS IP ranges are updated. It also refactors the loading of IPs (from disk & from an AWS URL) into a service.