Implement regional STS endpoint support #229
Conversation
|
Love it :-) Only comment would be to maybe add a default/enum to make it easier for operators to understand what value to set in the flag? |
|
Will this support aws china regions? I have problem deploying |
Sure, I can do that. I'm thinking something like "global-endpoint" as it would be highly unlikely for AWS to use that as a region. |
The diff does support regional endpoints in China assuming they exist (I haven't tested there). |
Oh wait did you mean replicate all the region list as an enum? Although I see benefit in that, we'd have to constantly edit the list when AWS adds regions which seems error prone. Perhaps I can just add an example to the flag description? I'm also not a fan of the previous diff I just posted (i'd prefer to leave it the zero value of the string and explain the behavior in the description). I think I misunderstood your feedback. Anyway, thanks for the opportunity to contribute! |
|
Good point, don't want to needlessly replicate the list.
Maybe you could extend the description with a couple of examples so people
can see what kinds of values ought to be accepted?
Additionally, it'd be good to make sure the server exits with a failure if
the user has provided an invalid region for the host- maybe resolve the
address and if it fails to resolve successfully the server process exits
with a failure log message?
…On Tue, 12 Mar 2019 at 05:34, Chris Bradfield ***@***.***> wrote:
Love it :-)
Only comment would be to maybe add a default/enum to make it easier for
operators to understand what value to set in the flag?
Oh wait did you mean replicate all the region list as an enum? Although I
see benefit in that, we'd have to constantly edit the list when AWS adds
regions which seems error prone. Perhaps I can just add an example to the
flag description?
I'm also not a fan of the previous diff I just posted (i'd prefer to leave
it the zero value of the string and explain the behavior in the
description). I think I misunderstood your feedback.
Anyway, thanks for the opportunity to contribute!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#229 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAEfjBwJmy3v4aVf9sM5gREelIAhDmxks5vVzxMgaJpZM4bmeMJ>
.
|
|
Could region be auto-discovered from EC2 Metadata / kubernetes labels? You could also update docs/readme to point out this new feature. Gonna check this in |
|
I can confirm that |
… lookup. Refactor to cache the result so that we don't validate the region on every request.
Hmm. Well, this would get me what I want as-is. I'll add it if everyone agrees it makes the whole thing a lot better. The only downside I could see is it binds kiam-server to running in-cluster. That may be the case already. I also don't know how we would support the fips regions as well (technically, they aren't regions but the Go SDK treats them that way). |
I don't think this is a big issue, as when kiam fails to discover region via EC2 Metadata it can fall back to old behavior. Anyway, this can be a scope of another PR. Thanks for this submission, it saved my day ;-) |
|
It'd be cool to have it auto-discover but I'm also happy for it to default to current behaviour (and add a second for the auto-discovery later). Only comment would be if we can add some help docs to the flags to indicate the kinds of values you could set: As I said earlier, other thing I'd like (if possible) would be something that could validate and fail early if someone provides a bad region (typo etc.) as we wouldn't want to start the server, it accept connections and never be able to generate credentials. |
@pingles did you have a chance to look at the updated PR? In it I updated the flag description to include an example and also information about the default behavior. The regional endpoint is now validated based on the existence of a region as well as DNS resolution of the hostname of the endpoint. In addition, I cache the ResolvedEndpoint so that these checks aren't performed after the initial endpoint resolution. I have also added a bullet point to the features list in the README.md Let me know if you want more examples. The tests also will test the resolution of the endpoint. It might be best to make one more change there to make the resolver parametric for testability so that you don't need network connectivity to run the tests. |
|
Fantastic, thanks for the contribution! @Joseph-Irving be great if you guys could cut a release when you can 😄 |
This fixes uswitch#229 while also cleaning up the run logic to catch the error returned by server.Serve() and log when iptables changes are being undone.
This fixes uswitch#229 while also cleaning up the run logic to catch the error returned by server.Serve() and log when iptables changes are being undone.
* agent: fix #229: ensure deferred funcs run + cleanup This fixes #229 while also cleaning up the run logic to catch the error returned by server.Serve() and log when iptables changes are being undone. * agent: handle server shutdown and error Handle server shutdown directly in the run() command and log any errors. Previously, the code relied on a deferred call to close to stop the server and errors were ignored.
fixes #190