swipely / iam-docker

Use different IAM roles for each Docker container on an EC2 instance
MIT License
211 stars 22 forks source link

Some issues between v1.0.0 and v1.1.0 #27

Open ozbillwang opened 6 years ago

ozbillwang commented 6 years ago

I successfully use iam-docker roles for containers running in a Rancher v1.6.x platform finally, but only with an old version v1.0.0, no luck with v1.1.0 and v1.2.0.

Please reference the issue #25 for details.

I don't want to miss the new features and need understand what's the problem between the commits from v1.0.0 to v1.1.0, because I got this issue start from v1.1.0

The only notable change is about new IAM_ROLE environment variable (https://github.com/swipely/iam-docker/commit/26680c89e020481fe890d1e5b8173fe77f89807e)

Seems some bugs in below codes, which was added into v1.1.0

https://github.com/swipely/iam-docker/blame/master/src/docker/container_store.go#L177-L185

-       iamRole, hasKey := container.Config.Labels[iamLabel]
-       if !hasKey {
-               return nil, fmt.Errorf("Unable to find label named '%s' for container: %s", iamLabel, id)
+       iamRole, hasLabel := container.Config.Labels[iamLabel]
+       if !hasLabel {
+               env := dockerClient.Env(container.Config.Env)
+               envRole := env.Get(iamEnvironmentVariable)
+               if envRole != "" {
+                       iamRole = envRole
+               } else {
+                       return nil, fmt.Errorf("Unable to find label named '%s' or environment variable '%s' for container: %s", iamLabel, iamEnvironmentVariable, id)
+               }

@willglynn

Could you take a look?

willglynn commented 6 years ago

Per @nahiluhmot's comment in #25: are you setting IAM_ROLE on the iam-docker container?

If so, I would expect that to fail in some spectacular fashion, possibly including a panic. I hadn't considered what that would do. It doesn't make any sense to configure the iam-docker container's role since iam-docker is responsible for assuming that role; setting either the environment variable or the label is an infinite regress.

Does iam-docker need to notice this situation and emit a better error message?

ozbillwang commented 6 years ago

@willglynn

Thanks.

I didn't use IAM_ROLE environment and still get error. I added this variable because I saw the error message related to IAM_ROLE.

You can follow the first error I have, which has no IAM_ROLE when run iam-docker container.

ozbillwang commented 6 years ago

another finding is, even running successfully with v1.0.0, I can see below error log, but the container keep running with Up status.

2018-07-04T02:22:26Z [docker] Unable to add container event-handler=4 id=42026072677d9a20baf02f06c04fea5bc57309bbf087df5780cbde6397edf154 event=start error="Unable to find label named 'com.swipely.iam-docker.iam-profile' for container: 42026072677d9a20baf02f06c04fea5bc57309bbf087df5780cbde6397edf154"

Compare the error (without IAM_ROLE environment) with v1.1.0 and v1.2.0

2018-07-04T02:21:29Z [docker] Unable to add container event-handler=4 id=c28e1a9c7de3791af0dd942331508e299cd18e2bd65ccd77ea0434cbe2697750 event=start error="Unable to find label named 'com.swipely.iam-docker.iam-profile' or environment variable 'IAM_ROLE' for container: c28e1a9c7de3791af0dd942331508e299cd18e2bd65ccd77ea0434cbe2697750" panic: runtime error: index out of range

with golang errors and container is stopped.

github.com/swipely/iam-docker/vendor/github.com/fsouza/go-dockerclient.(*Env).Map(0xc420127588, 0xc42007e940)
    /go/src/github.com/swipely/iam-docker/vendor/github.com/fsouza/go-dockerclient/env.go:165 +0x170
github.com/swipely/iam-docker/vendor/github.com/fsouza/go-dockerclient.(*Env).Get(0xc420127588, 0x77f26f, 0x8, 0x22, 0x97fb80)
    /go/src/github.com/swipely/iam-docker/vendor/github.com/fsouza/go-dockerclient/env.go:20 +0x2b
github.com/swipely/iam-docker/src/docker.(*containerStore).findConfigForID(0xc42007e940, 0xc42010b8c0, 0x40, 0x0, 0x7c0ae0, 0xc420302ed0)
    /go/src/github.com/swipely/iam-docker/src/docker/container_store.go:180 +0x536
github.com/swipely/iam-docker/src/docker.(*containerStore).SyncRunningContainers(0xc42007e940, 0x0, 0x0)
    /go/src/github.com/swipely/iam-docker/src/docker/container_store.go:146 +0x290
github.com/swipely/iam-docker/src/app.(*App).syncRunningContainers(0xc420063f80, 0x7c5120, 0xc42007e940, 0x7c2740, 0xc42007e980, 0xc42007ebc0)
    /go/src/github.com/swipely/iam-docker/src/app/app.go:122 +0xba
created by github.com/swipely/iam-docker/src/app.(*App).containerSyncWorker
    /go/src/github.com/swipely/iam-docker/src/app/app.go:52 +0x1af
willglynn commented 6 years ago

Looks like something in go-dockerclient: Env.Get(key) delegates to Env.Map()[key], and Env.Map() can return nil, which means Env.Get() can panic. We can work around it here, but that seems surprising. Digging a bit more…

willglynn commented 6 years ago

No, I'm wrong, dereferencing nil maps is totally fine. env.go line 165 is something to do with parsing the container environment.

Could you run docker ps --format {{.ID}} | xargs docker inspect | jq '.[] | .Config | .Env' and paste the result into a new gist? I think something is formatted wrong somewhere in a container environment, and trying to parse it is the cause of the problem.

willglynn commented 6 years ago

(My hypothesis is that this was the issue in https://github.com/fsouza/go-dockerclient/commit/cd1c311c4bf5da6776d6ee85266f3f0d2d74d8db, which is already fixed upstream and needs a dependency bump here in iam-docker.)

ozbillwang commented 6 years ago

@willglynn

Do you still need the output you asked for? Or you know the root cause already?

willglynn commented 6 years ago

I'm pretty sure it's this, but the raw environment data would confirm.

ozbillwang commented 6 years ago

@willglynn I run the command, but there are a lot of details with company informations, I can't provide.

willglynn commented 6 years ago

Fair enough. Please try my update_dockerclient branch (#28) and see if that fixes the issue.