Add default IAM role auto discovery #63

jescarri · 2017-04-06T18:22:47Z

Related to:
#13

jescarri · 2017-04-14T16:32:31Z

Hi any update on this?

jtblin · 2017-04-18T01:23:14Z

@jescarri if you just assign the default IAM role to containers that are running on the worker, what is the point of running kube2iam which prevents using the default role and add this option to use the default role?

jescarri · 2017-04-18T03:35:35Z

@jtblin my use case is as follows:

I have a dozen of clusters all spread across multiple accounts base ARN and IAM name changes, I don't want to go to ec2 and look for the name when I add a new cluster.
we have mixed workloads one of those workloads is prometheus + ec2 discovery
EC2 default role allows assumeRole and ec2:describe-instances this is used mainly for prometheus, our prometheus users use prometheus as a service and IAM is abstracted from them, this is the main reason for auto discovery the instance iam role, that way our prometheus users can just deploy prometheus to any cluster and not worry about the correct iam role for the cluster.

We use Kube2Iam as intended, pods that require more than ec2:describe-instances, need to go to the process of annotating their pods, establishing trust relationships etc, this PR is just for convenience of deployment and basic usage.

What I think is missing is the ability to auto detect the base arn and being able to specify the default iam role.

Does this makes sense?, I have been running the code of this PR in prod a couple of weeks, w/o issues.

jtblin · 2017-04-19T21:45:17Z

Thanks for the explanation, makes sense. Didn't think about this use case.

jtblin

In addition to the comment above, instead of retrieving the role, we could just let the server proxy to the metadata API in case there is no annotation and the flag is set i.e. around here. That would remove the need for the logic dealing with the metadata API entirely and would be more consistent with the other endpoints (we don't use the metadata API, we just proxy for non secure endpoints like instance name etc.). The downside is that we wouldn't get the base role but I'm not sure how important it is.

jtblin · 2017-04-18T23:11:08Z

cmd/iam.go

+	if !metadata.Available() {
+		return "", "", fmt.Errorf("EC2 Metadata is not available, are you running on EC2?")
+	}
+	resp, err := http.Get("http://169.254.169.254/latest/meta-data/iam/security-credentials/")


You don't need to make a manual http request, the purpose of the ec2metadata package is to abstract that, use metadata.GetMetadata.

Let me fix the http request thing, I wanted to be consistent with the current operation mode, same as the --default-role but auto discovered and set to the iam instance role.

The base arn is crucial, because it changes from account to account, we could have standard IAM role names but the base will change, keeping track of those base arn is a pain.

What I propose is to break this into two flags:
--auto-discover-base-arn
--auto-discover-default-role

the flag check will be as follows:
if --auto-discover-base-arn and no default-role then die
if --auto-discover-default-role it will populate the base-arn and default-role ( current operation mode of this PR ).

Let me know if you agree so I can do the modifications.

Thanks!

jtblin · 2017-04-19T22:36:13Z

Makes sense, I forgot the base arn is mandatory, and this will fix #13. Maybe break that into 2 PRs then, it will be easier to review, starting by the one for the base arn?

…

On 20 Apr. 2017 8:20 am, "Jesus Rafael Carrillo" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In cmd/iam.go <#63 (comment)>: > @@ -51,6 +54,40 @@ func getHash(text string) string { return fmt.Sprintf("%x", h.Sum32()) } +// GetInstanceIamRole get instance iam role from metadata service +func GetInstanceIamRole() (string, string, error) { + sess, err := session.NewSession() + if err != nil { + return "", "", err + } + metadata := ec2metadata.New(sess) + if !metadata.Available() { + return "", "", fmt.Errorf("EC2 Metadata is not available, are you running on EC2?") + } + resp, err := http.Get("http://169.254.169.254/latest/meta-data/iam/security-credentials/") Let me fix the http request thing, I wanted to be consistent with the current operation mode, same as the --default-role but auto discovered and set to the iam instance role. The base arn is crucial, because it changes from account to account, we could have standard IAM role names but the base will change, keeping track of those base arn is a pain. What I propose is to break this into two flags: --auto-discover-base-arn --auto-discover-default-role — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUxrhlCEr5RJlxa2kuCW3Kdiss1BI3Oks5rxojFgaJpZM4M1_Lb> .

jescarri · 2017-04-19T22:37:55Z

sure, will have both of them in the next day or so.

jescarri · 2017-04-23T07:59:00Z

@jtblin I made a single PR because the --auto-discover-default-role is dependent on --auto-discover-base-arn feature, I also upgraded the AWS SDK version to latest and corrected the kubernetes api version in the glide.yaml file it was set to a newer version but the glide.lock file was never upgraded, using a newer k8s client library seems to break the compile.

Thanks!.

jtblin · 2017-04-24T03:25:55Z

main.go

+			log.Fatal("--auto-discover-base-arn can't be used if --base-role-arn is specified")
+		}
+		if s.DefaultIAMRole == "" {
+			log.Fatal("You need to specify the default IAM role, use --auto-discover-default-role to autodiscover it")


You're setting the default role with AutoDiscoverDefaultRole below. Shouldn't that be reversed?

Also I don't understand why it is mandatory to set the default role to retrieve the base ARN?

Is just a warning, if DefaultIamRole is blank print that is a required argument and suggest the use of --auto-discover-default-role

Well even if you set --auto-discover-default-role it won't work as the test is only if the default role is set which will not have happen at this stage yet. Anyhow see my other comment about why this is even mandatory (maybe I'm missing something :)?).

jtblin · 2017-04-24T03:26:57Z

main.go

+			log.Fatal("--auto-discover-base-arn can't be used if --base-role-arn is specified")
+		}
+		if s.DefaultIAMRole == "" {
+			log.Fatal("You need to specify the default IAM role, use --auto-discover-default-role to autodiscover it")


Also I don't understand why it is mandatory to set the default role to retrieve the base ARN?

jtblin · 2017-04-24T03:29:46Z

README.md

+      --api-token string                    Token to authenticate with the api server
+      --app-port string                     Http port (default "8181")
+      --auto-discover-base-arn              Queries EC2 Metadata to determine the base ARN
+      --auto-discover-default-role          Queries EC2 Metadata to determine the default Iam Role, can't be used with --default-role


can't be used with --default-role

You don't seem to be enforcing that.

jtblin · 2017-04-24T23:36:39Z

Thanks a ton for the contribution and for your patience! 👍

jescarri · 2017-04-25T05:07:26Z

You are welcome!

whereisaaron · 2019-03-12T17:01:28Z

I don't see it documented but --namespace restrictions appear to (correctly in my view) block any --auto-discover-default-role or --default-role. The default role appears to get assigned to the Pod, but then gets blocked (500) when the Pod tries to use it.

Should the default role even get assigned if it doesn't match the namespace restrictions? Maybe yes, because the namespace restrictions could change after the Pod starts?

It would be good if the README explained this under Namespace Restrictions @jtblin. I don't mind doing that but I am not clear my understanding is correct.

-----------------------------------------------------
DEBUG: Request ec2metadata/GetMetadata Details:
---[ REQUEST POST-SIGN ]-----------------------------
GET /latest/meta-data/iam/security-credentials/myDefaultRole HTTP/1.1
Host: 169.254.169.254
User-Agent: aws-sdk-go/1.13.34 (go1.11.5; linux; amd64)
Accept-Encoding: gzip


-----------------------------------------------------
DEBUG: Response ec2metadata/GetMetadata Details:
---[ RESPONSE ]--------------------------------------
HTTP/1.1 500 Internal Server Error
Content-Length: 96
Content-Type: text/plain; charset=utf-8
Date: Tue, 12 Mar 2019 16:56:05 GMT
Server: EC2ws
X-Content-Type-Options: nosniff

jtblin requested changes Apr 19, 2017

View reviewed changes

Adding base arn autodiscovery and default iam role autodiscovery

6215576

jtblin reviewed Apr 24, 2017

View reviewed changes

Fixing requested changes

a29be63

jtblin approved these changes Apr 24, 2017

View reviewed changes

jtblin merged commit 6470cad into jtblin:master Apr 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default IAM role auto discovery #63

Add default IAM role auto discovery #63

jescarri commented Apr 6, 2017

jescarri commented Apr 14, 2017

jtblin commented Apr 18, 2017

jescarri commented Apr 18, 2017 •

edited by jtblin

Loading

jtblin commented Apr 19, 2017

jtblin left a comment

jtblin Apr 18, 2017

jescarri Apr 19, 2017 •

edited

Loading

jtblin commented Apr 19, 2017 via email

jescarri commented Apr 19, 2017

jescarri commented Apr 23, 2017

jtblin Apr 24, 2017

jtblin Apr 24, 2017

jescarri Apr 24, 2017

jtblin Apr 24, 2017

jtblin Apr 24, 2017

jtblin Apr 24, 2017

jtblin commented Apr 24, 2017

jescarri commented Apr 25, 2017

whereisaaron commented Mar 12, 2019

Add default IAM role auto discovery #63

Add default IAM role auto discovery #63

Conversation

jescarri commented Apr 6, 2017

jescarri commented Apr 14, 2017

jtblin commented Apr 18, 2017

jescarri commented Apr 18, 2017 • edited by jtblin Loading

jtblin commented Apr 19, 2017

jtblin left a comment

Choose a reason for hiding this comment

jtblin Apr 18, 2017

Choose a reason for hiding this comment

jescarri Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

jtblin commented Apr 19, 2017 via email

jescarri commented Apr 19, 2017

jescarri commented Apr 23, 2017

jtblin Apr 24, 2017

Choose a reason for hiding this comment

jtblin Apr 24, 2017

Choose a reason for hiding this comment

jescarri Apr 24, 2017

Choose a reason for hiding this comment

jtblin Apr 24, 2017

Choose a reason for hiding this comment

jtblin Apr 24, 2017

Choose a reason for hiding this comment

jtblin Apr 24, 2017

Choose a reason for hiding this comment

jtblin commented Apr 24, 2017

jescarri commented Apr 25, 2017

whereisaaron commented Mar 12, 2019

jescarri commented Apr 18, 2017 •

edited by jtblin

Loading

jescarri Apr 19, 2017 •

edited

Loading