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

Resolve parameters via HTTP #1

Merged
merged 2 commits into from Jan 14, 2020
Merged

Resolve parameters via HTTP #1

merged 2 commits into from Jan 14, 2020

Conversation

@orien
Copy link
Member

orien commented Jan 13, 2020

Add initial functionality that can obtaining StackMaster parameters from HTTP calls returning plain text.

For example, to resolve the Cloudflare IPv4 ranges:

cloudflare_ips:
  http: https://www.cloudflare.com/ips-v4

To obtain both the Cloudlare IPv4 and IPv6 ranges:

cloudflare_ips:
  - http: https://www.cloudflare.com/ips-v4
  - http: https://www.cloudflare.com/ips-v6
@jacobbednarz

This comment has been minimized.

Copy link
Member

jacobbednarz commented Jan 13, 2020

I like the concept! 💯

Perhaps for the Cloudflare IPs we make it specific to how we would expect to be using it and also limit the scope of how it can be used to avoid shaping "HTTP resolver" into Cloudflare specific one. I'm not sure I have enough prior art on how we would use a HTTP resolver to be able to classify this as the way forward just yet. WDYT?

@simpsora

This comment has been minimized.

Copy link

simpsora commented Jan 13, 2020

I like it!

@jacobbednarz I'm not sure I understand your concern. I don't see anything Cloudflare-specific here -- the only thing not super generic about it is the fact that it splits on whitespace and returns a list (rather than a string or some other structure).

I'm not against having a specific resolver for this use case, but I also don't see the point yet.

@jacobbednarz

This comment has been minimized.

Copy link
Member

jacobbednarz commented Jan 13, 2020

the only thing not super generic about it is the fact that it splits on whitespace and returns a list (rather than a string or some other structure).

Yep, this. I don't see other use cases for this at the moment and would prefer to subscribe to using something like the rule of three before we broaden the use cases for it. For example, I could see a time we might want to lock down access to other AWS IPs (or specific services). Right now, that won't work here despite us saying this a HTTP resolver. I wouldn't expect there to be no work involved in adding that functionality but I would expect it to be able to be built on top of the HTTP resolver and not completely rewritten which would be the case here as we've built it to support a specific use case (plain text, split on whitespace).

Our current use case is for Cloudflare and we should build it for that until we have more examples of using it to avoid YAGNI. We may never actually need a generic HTTP resolver 🤷‍♂️

@simpsora

This comment has been minimized.

Copy link

simpsora commented Jan 13, 2020

I agree YAGNI applies, though I don't think the rule of threes does (we're not refactoring). The risks are very low, but I see the point @jacobbednarz's making that it might be confusing to future people who wanted an HTTP resolver.

My care factor on this is low so 🤷‍♂. Making this use-case-specific for now then refactoring when we get future uses might make sense (whether separate resolvers, or a config param that lets the caller specify the split character, for example).

@orien

This comment has been minimized.

Copy link
Member Author

orien commented Jan 13, 2020

I think for a 0.1 release this is ok. It implements one use case. If we at Envato, or people from the wider development community, find another use case that requires different HTTP response body parsing we can tackle that at the time.

There are options to further configure for that.

Alternatively, if we find this gem isn't needed, we can deprecate it easily enough.

Copy link

simpsora left a comment

👍

Worth noting that the driver for this particular change (Tuts+) won't be implementing it right away because of some technical issues (their infra stack is from the early 1990s and doesn't run in a VPC yet).

There are several places that are currently hardcoding Cloudflare IPs, though, so this would still be applicable elsewhere.

@simpsora

This comment has been minimized.

Copy link

simpsora commented Jan 13, 2020

Copy link

mattpatterson94 left a comment

👏

@orien

This comment has been minimized.

Copy link
Member Author

orien commented Jan 14, 2020

Ok, I'll release version 0.1.

@orien orien merged commit ead0d57 into master Jan 14, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@orien orien deleted the http-parameter-resolving branch Jan 14, 2020
orien added a commit that referenced this pull request Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.