New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement drone-docker-ecr #155

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@danielkrainas
Copy link

danielkrainas commented Oct 13, 2017

Addresses a couple issues from the drone-ecr repository:

  1. drone-plugins/drone-ecr#43 - implements the drone-docker-ecr command.
  2. drone-plugins/drone-ecr#41 - will look for AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables.
  3. drone-plugins/drone-ecr#34 - kind of a duh since it was from scratch but worth noting the error wasn't transposed in the go version.

I tried taking a look at a couple other plugins for examples how to handle/report errors (drone-docker and drone-helm specifically) but nothing that fit my usage so not quite sure what the preferred way is but they're stubbed with os.Exit(1) for now. Let me know and I'd be glad to change them.

@danielkrainas

This comment has been minimized.

Copy link

danielkrainas commented Oct 24, 2017

ping

@gtaylor
Copy link
Contributor

gtaylor left a comment

Just a very minor suggestion for comment and/or function naming.

os.Exit(1)
}

// always attempt to create the repo if createBool is true, eat exists error

This comment has been minimized.

@gtaylor

gtaylor Oct 25, 2017

Contributor

I like that we're documenting that we're eating the exists error, but that's more of an implementation detail for createRepo. I was initially confused to see us exiting on err on line 46, since this comment seems to contradict that.

I'd suggest either removing eat exists error here, or doing the same and adding something on the createRepo call in 44 that is a little more clear:

"already exists" errors are not returned here, since we want to ensure the repo's existence

Another option would be to rename createRepo to ensureRepoExists or ensureRepo to make this more immediately obvious.

This comment has been minimized.

@danielkrainas

danielkrainas Oct 25, 2017

This flagged in my head when I was looking it over before submission as well as possibly confusing so I totally understand. I'll rename the function and drop the comment.

@gtaylor
Copy link
Contributor

gtaylor left a comment

Very nice!

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Oct 31, 2017

Thanks! I pulled down changes locally and pushed e4bc243. I do this only as a security precaution since we have no way to validate large vendor updates with govendor. I also switched to dep (a newer, official Go vendor solution) so that in the future validation will no longer be required.

@danielkrainas danielkrainas deleted the danielkrainas:implement-drone-ecr branch Oct 31, 2017

@gtaylor

This comment has been minimized.

Copy link
Contributor

gtaylor commented Oct 31, 2017

@danielkrainas hearing reports of issues (without details) here. Is this plugin working for you as merged and published to Docker Hub today?

@danielkrainas

This comment has been minimized.

Copy link

danielkrainas commented Oct 31, 2017

@gtaylor sorry to hear that, I'll be happy to help address anything that comes up. From the link, it looks like they are talking about the old/legacy ecr plugin. The plugin is functioning for us and we have a cluster suite of apps building and publishing with it.

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Oct 31, 2017

@danielkrainas are there any breaking changes (legacy vs new) we need to document in this thread?

@danielkrainas

This comment has been minimized.

Copy link

danielkrainas commented Oct 31, 2017

@bradrydzewski point 3 related to drone-plugins/drone-ecr#34 is the only thing that would count as "breaking" from the legacy version but that's more correctly a fix

@ktruckenmiller

This comment has been minimized.

Copy link

ktruckenmiller commented Nov 1, 2017

For some reason plugins/ecr isn't working on my 17.09 docker + drone 7.3 and also it's not working on 17.09 + drone 8.1. I'm not sure what changed, but both my servers stopped building ecr stuff.

To fix I stubbed in the wrapper script and pushed my own plugin and its now working with my fix.

@danielkrainas

This comment has been minimized.

Copy link

danielkrainas commented Nov 1, 2017

@ktruckenmiller

  1. Could you possibly turn on debugging (debug: true) for the ecr pipeline step to get a more verbose output and help narrow down the problem?
  2. Are you exposing the required AWS secrets to the step? (secrets: [AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY])
@ktruckenmiller

This comment has been minimized.

Copy link

ktruckenmiller commented Nov 1, 2017

I have an ECR role on the box I'm using. So I'm not exposing the secrets. Should I be?

id message build exited error exit_code
1171 pipeline done 6 null null null
1174 received execution 7 null null null
1174 listen for cancel signal 7 null null null
1174 update step status 7 false null 0
1174 update step status complete 7 false null 0
1174 log stream opened 7 null null null
1174 log stream copied 7 null null null
1174 log stream uploading 7 null null null
1174 update step status 7 true null 0
1174 log stream upload complete 7 null null null
1174 log stream closed 7 null null null
1174 update step status complete 7 true null 0
1174 update step status 7 false null 0
1174 update step status complete 7 false null 0
1174 log stream opened 7 null null null
1174 log stream copied 7 null null null
1174 log stream uploading 7 null null null
1174 log stream upload complete 7 null null null
1174 log stream closed 7 null null null
1174 update step status 7 true null 1
1174 update step status complete 7 true null 1
1174 pipeline complete 7 null 1
1174 uploading logs 7 null null null
1174 uploading logs complete 7 null null null
1174 updating pipeline status 7 null 1
1174 stop listening for cancel signal 7 null null null
1174 updating pipeline status complete 7 null null null
null request next execution null null null null
1174 pipeline done 7 null null null
@danielkrainas

This comment has been minimized.

Copy link

danielkrainas commented Nov 2, 2017

I'm not sure about the old ecr plugin but the current plugin runs with DIND so the docker daemon that is running and building the image is not the host's. Because of this, my guess is the DIND daemon doesn't have access to the instance's ECR role and so remains unauthenticated to ECR. You could expose the AWS secrets, that should work. Otherwise if you insist on using the host's ECR authentication, for the build step - you could try disabling the DIND daemon, set the repo in drone as "Trusted", and mount the host's docker socket as a volume in the step.

pipeline:
  ecr_publish:
    image: plugins/drone-ecr:latest
    repo: my-service-repo
    daemon_off: true
    tags:
      - latest
    volumes:
      - /var/run/docker.sock:/var/run/docker.sock
@ktruckenmiller

This comment has been minimized.

Copy link

ktruckenmiller commented Nov 2, 2017

No, you can get creds from the EC2metadata service on 169.254 if your host has it avail. Just add a conditional for the creds here:

https://github.com/drone-plugins/drone-docker/blob/master/cmd/drone-docker-ecr/main.go#L34

Unless you want to be opinionated about having users provide them. If so, let me know because I need to rethink some things.

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Nov 2, 2017

The primary difference between the old plugin and the new plugin is that the script that generates the username and password is written in Go instead of bash. Both plugin use dind and use the official plugins/docker image as base.

I believe the issue is related to how this implementation loads the credentials. Instead of using NewEnvCredentials it should look something like below. This code snippet comes from the Drone enterprise version edition which support ECR pipeline images and is confirmed to work with static credentials, metadata, iam, etc.

	var creds *credentials.Credentials
	if key != "" {
		creds = credentials.NewStaticCredentials(key, secret, "")
	}
	sess := session.New(&aws.Config{
		Region:      aws.String(region),
		Credentials: creds,
	})

	service := ecr.New(sess, aws.NewConfig().WithRegion(region))

Note that I am out of office for the next few days with limited internet and will not be able to test this patch, but can merge a verified PR via the GitHub user interface. I would therefore ask that interested parties create and test a patch and submit a PR once they verify it is working. I will then be happy to merge. Thanks!

@danielkrainas

This comment has been minimized.

Copy link

danielkrainas commented Nov 2, 2017

@bradrydzewski not familiar with the aws sdk's behavior when passed nil creds, but if it tries to procure them from the typical places, then that would make sense.

@danielkrainas

This comment has been minimized.

Copy link

danielkrainas commented Nov 2, 2017

Looks like #159 will solve this problem.

@gtaylor

This comment has been minimized.

Copy link
Contributor

gtaylor commented Nov 7, 2017

I'll admit to being a bit disappointed that this regression has been in place for six days now. This one is pretty major, in that anyone using the plugins/ecr without static credentials will experience complete breakage.

In the future I hope we'll revert sooner in the face of severe breakage like this.

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Nov 7, 2017

The pull request is merged and the image is updated in Docekrhub.

If you continue to experience issues with this newer version of the plugin you can fallback to the older version with the plugins/ecr:1.0 tag.

If you are using plugins/ecr or plugins/ecr:latest I would recommend changing to plugins/ecr:17.05 so that future breaking changes or regressions do not impact your production installs. I believe all plugins are now tagged (thanks @tboerger) and plugin documentation will be updated to recommend using tagged versions instead of latest.

@gtaylor

This comment has been minimized.

Copy link
Contributor

gtaylor commented Nov 7, 2017

Confirmed that 17.05 (post-#159) and 1.0 both work. Thanks, Brad and @AKoetsier.

@sethpollack sethpollack referenced this pull request Nov 24, 2017

Closed

add ecr support #152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment