Add IP ranges #259
Add IP ranges #259
Conversation
dabcoder
force-pushed
the
davidb/ip-ranges
branch
from
407aec5 to
df27bb6
Compare
|
Hey David, thanks for submitting this PR. The |
dabcoder
force-pushed
the
davidb/ip-ranges
branch
2 times, most recently
from
2d61547 to
abf3dac
Compare
request.go
Outdated
| apiBase, err := url.Parse(client.baseUrl + "/api" + api) | ||
| var err error | ||
| // If api is an endpoint starting with v1, we construct the apiBase URL using the baseUrl to which we append "/api" and the actual value of api | ||
| if strings.Contains(api, "v1") { |
There was a problem hiding this comment.
TBH I'm a little unsure about this condition. We could theoretically be passing in full URLs that we expect to just return and which might still contain v1. I'd much rather that this was if !(strings.HasPrefix("https://") or strings.HasPrefix("http://"))
There was a problem hiding this comment.
Right, I also started in that direction but both URL types will start with https:
client.baseUrl + "/api" + apiwill behttps://api.datadoghq.com/api/v1....- IP ranges URLs:
https://ip-ranges.datadoghq.com/orhttps://ip-ranges.datadoghq.eu/
So not sure if we should compare based onhttpsorhttp?
There was a problem hiding this comment.
Hey, sorry I'm a bit late here.
Hmm, this is a good point, because we also aren't sending the API/APP keys in the query params based on this if condition. Due to that and the fact that we have some v2 endpoints already (see the dash list v2 endpoint), we can't use this condition, otherwise those requests will be sent without auth and fail.
Let me know if I'm reading any of this incorrectly 😄 At this point in the code, api is either a URI from the caller, like /v2/dashboards/... or /v1/hosts/... or it's the whole URL for IP ranges https://ip-ranges.datadoghq.com/.
(the https:api.datadog.... is only added later) So I think the proposed change Slavek made would work here. And then we only add authentication (api/app keys) to paths that are URIs, and not if a full URL is passed into this function.
request.go
Outdated
| @@ -31,17 +31,43 @@ type Response struct { | |||
| // uriForAPI is to be called with something like "/v1/events" and it will give | |||
| // the proper request URI to be posted to. | |||
There was a problem hiding this comment.
Could you please amend the docstring of this function to cover the change done below?
dabcoder
force-pushed
the
davidb/ip-ranges
branch
from
838986b to
8f66d33
Compare
request.go
Outdated
| apiBase, err := url.Parse(client.baseUrl + "/api" + api) | ||
| var err error | ||
| // If api is a URI such as /v1/hosts/, /v2/dashboards... add credentials and return a properly formatted URL | ||
| if !strings.HasPrefix(api, "https://") || !strings.HasPrefix(api, "http://") { |
There was a problem hiding this comment.
So I didn't actually try this out, but I don't think this condition is right. If the api value is e.g. https://something, the second part would still evaluate as true and so the if clause would actually be entered. I think what you need to do is !(strings.HasPrefix(api, "https://") || strings.HasPrefix(api, "http://"))
Could be used in the terraform provider as there is an open feature request for that:
https://github.com/terraform-providers/terraform-provider-datadog/issues/216.
WIP, so the approach may not be the best.