VYGR-391: Add OpsGenie integration manager client - #119
Conversation
| ) | ||
|
|
||
| const ( | ||
| asapAudience = "micros-server" |
There was a problem hiding this comment.
integration manager? We probably should use a separate audience, otherwise we wouldn't be able to migrate it to a separate service. We should have built a separate service in the first place.
There was a problem hiding this comment.
if we want to optimize for migration to a separate service in the future, we could also make this configurable.
@jokeyrhyme can you confirm the ASAP audience to be used there? agree with Mikhail that it shouldn't be tied to micros-server.
There was a problem hiding this comment.
I asked @jokeyrhyme on Slack and he replied that we can set whatever we like. I'll set this and comment.
There was a problem hiding this comment.
Well, this is... not secure. The server must check audience otherwise someone can use any valid ASAP token to talk to it. e.g. I can generate a token myself and use it. /cc @jokeyrhyme
There was a problem hiding this comment.
it probably has a whitelist of issuers though? otherwise audience is useless.
@ash2k I think you're confusing issuers and audience.
There was a problem hiding this comment.
@fcobb what about issuers? Can you double check that issuers are whitelisted (i.e. only voyager synchronizer and micros-server are allowed to talk to Opsgenie integration manager)
There was a problem hiding this comment.
I ran the manual client test (found in this PR) using the asap-creator secret and it worked.
There was a problem hiding this comment.
Sorry, I was previously tuning out from GitHub notifications, because there are multiple teams worth of activity here, but I've adjusted this now so mentions will stand out :)
I've commented on MICROS-6953 about this, after taking a quick look
We don't check claims.aud in our code or dependencies anywhere as far as I can see, but we do check claims.iss to assert that it matches
There was a problem hiding this comment.
@fcobb do creator and synchronization share the same issuer then?
There was a problem hiding this comment.
@nilebox No.
kubectl -n voyager get secrets asap-voyager-synchronization gives me issuer: a3ViZS92b3lhZ2VyLXN5bmNocm9uaXphdGlvbg==
kubectl -n voyager get secrets asap-creator -o yaml gives me issuer: a3ViZS92b3lhZ2VyLWNyZWF0b3I=
Even the reporter ASAP secret works.
@jokeyrhyme "but we do check claims.iss to assert that it matches" What does it match exactly?
| ) | ||
|
|
||
| const ( | ||
| asapAudience = "micros-server" |
There was a problem hiding this comment.
if we want to optimize for migration to a separate service in the future, we could also make this configurable.
@jokeyrhyme can you confirm the ASAP audience to be used there? agree with Mikhail that it shouldn't be tied to micros-server.
|
|
||
| retriable := false | ||
| switch response.StatusCode { | ||
| case http.StatusInternalServerError: |
There was a problem hiding this comment.
any 5xx is theoretically retriable (as it's not caused by a client-side error)?
There was a problem hiding this comment.
Would you like me to add cases for the majority of the 5xx status codes? Seems odd to me
There was a problem hiding this comment.
I don't think all 5xx's are retriable in an backoff-loop sense; 501 for example is http.StatusNotImplemented:
The server either does not recognize the request method, or it lacks the ability to fulfil the request. Usually this implies future availability (e.g., a new feature of a web-service API).[63]
| return retriable, nil, errors.Wrap(err, "failed to read response body") | ||
| } | ||
|
|
||
| if response.StatusCode != http.StatusOK { |
There was a problem hiding this comment.
I think this was copied from Service Central client, and SC returns response with error details (see below that it's used for error message).
TODO: check the behavior of Opsgenie integration manager with non-200 code.
EDIT FROM FRASER:
I can't reply so editing here.
As mentioned elsewhere in this PR the swagger defines an error message response similar to the SC client. https://micros.prod.atl-paas.net/swagger/#!/opsgenie/getOrCreateOgIntegrations
I also tested this with a curl command.
f6f5e25 to
04e1d7d
Compare
fcobb
changed the title
| } | ||
| defer util.CloseSilently(response.Body) | ||
|
|
||
| retriable := false |
There was a problem hiding this comment.
We really need a bit of code that determines where a response means a request can be retried.
The following could also be retriable:
http.StatusRequestTimeout
http.StatusServiceUnavailable
The error is also a *url.Error which has a Temporary() and Timeout() check to see what the cause might have been. I haven't been able to determine what causes a Temporary() or Timeout() to return true though.
I used them here in 732c738#diff-3ec5620e3479cc95305dc75d504a817eR84 if you want to check it out.
There was a problem hiding this comment.
Yeah as discussed IRL I like the idea but would like to make those in a follow up PR unless we decide it blocks this PR.
My intention here was to address the main use cases where we want to retry.
scottgreenup
left a comment
scottgreenup
left a comment
There was a problem hiding this comment.
Apart from some nits, let's get this through. I'd wait for @nilebox to check as well (as you mentioned).
6 participants
This is a follow on from getting team from service central PR - #107
This adds a client that will be used in the next PR. #120