Skip to content
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

Create latest_container resolver #250

Merged
merged 10 commits into from
Aug 24, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,19 @@ A set of possible attributes is available in the [AWS documentation](https://doc

Any value can be an array of possible matches.

### Latest Container from Repository

Looks up the latest Container Image from an ECR repository. We use this instead of a "latest" tag, because if the tag is latest a CloudFormation update will not trigger a deployment of the container, since nothing has changed.
As such this resolver will never return the latest tag for a container.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a fun thing to find out the hard way.


```yaml
container_image_id:
latest_container:
repository_name: nginx # Required. The name of the repository
registry_id: 012345678910 # The AWS Account ID the repository is located in. Defaults to the current account's default registry
region: us-east-1 # Defaults to the region the stack is located in
```

### Environment Variable

Lookup an environment variable:
Expand Down
2 changes: 2 additions & 0 deletions lib/stack_master.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require 'aws-sdk-acm'
require 'aws-sdk-cloudformation'
require 'aws-sdk-ec2'
require 'aws-sdk-ecr'
require 'aws-sdk-s3'
require 'aws-sdk-sns'
require 'aws-sdk-ssm'
Expand Down Expand Up @@ -74,6 +75,7 @@ module ParameterResolvers
autoload :Env, 'stack_master/parameter_resolvers/env'
autoload :ParameterStore, 'stack_master/parameter_resolvers/parameter_store'
autoload :OnePassword, 'stack_master/parameter_resolvers/one_password'
autoload :LatestContainer, 'stack_master/parameter_resolvers/latest_container'
end

module AwsDriver
Expand Down
49 changes: 49 additions & 0 deletions lib/stack_master/parameter_resolvers/latest_container.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
module StackMaster
module ParameterResolvers
class LatestContainer < Resolver
array_resolver class_name: 'LatestContainers'

def initialize(config, stack_definition)
@config = config
@stack_definition = stack_definition
end

def resolve(parameters)
@region = parameters['region'] || @stack_definition.region
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe space this function out a bit to make it easier to understand logical groupings, eg:

      def resolve(parameters)
        if parameters['repository_name'].nil?
          raise ArgumentError, "repository_name parameter is required but was not supplied"
        end

        @region = parameters['region'] || @stack_definition.region
        ecr_client = Aws::ECR::Client.new(region: @region)

        images = fetch_images(parameters['repository_name'], parameters['registry_id'], ecr_client)
        return nil if images.empty?

        if !parameters['tag'].nil?
          images.select! { |image| image.image_tags.any? { |tag| tag == parameters['tag'] } }
        end
        images.sort! { |image_x, image_y| image_y.image_pushed_at <=> image_x.image_pushed_at }
        latest_image = images.first

        # aws_account_id.dkr.ecr.region.amazonaws.com/repository@sha256:digest
        "#{latest_image.registry_id}.dkr.ecr.#{@region}.amazonaws.com/#{parameters['repository_name']}@#{latest_image.image_digest}"
      end

ecr_client = Aws::ECR::Client.new(region: @region)
if parameters['repository_name'].nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is registry_id also a required parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as per the documentation it's only needed if you're fetching a container from a different account

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NM, I see the default now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no I don't, the docs say it should default but I don't see the defaulting happening.
And no test for what happens when it's not...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API handles this for us. If we don't specify a registry ID, it defaults to the current account

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, no need for tests then either :-)

raise ArgumentError, "repository_name parameter is required but was not supplied"
end
images = fetch_images(parameters['repository_name'], parameters['registry_id'], ecr_client)
return nil if images.empty?
images.sort! { |image_x, image_y| image_y.image_pushed_at <=> image_x.image_pushed_at }
latest_image = images.first
latest_tag = latest_image.image_tags.delete_if { |tag| tag == "latest" }.first
# aws_account_id.dkr.ecr.region.amazonaws.com
return "#{latest_image.registry_id}.dkr.ecr.#{@region}.amazonaws.com/#{parameters['repository_name']}:#{latest_tag}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the tag safe to use here? Wouldn't we be better with image_digest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only an issue if you use mutable tags like "production". I don't think that's good practice so while I wouldn't object to such a change I don't think we should prioritise it

Copy link
Contributor Author

@patrobinson patrobinson Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, recent events have made me question this view.
It seems as though we use our repositories not just to store the latest images but also as a cache for our CI pipelines. It's quite common for pipelines to push up images from a test environment. So finding the latest image is probably a terrible idea and what we should be doing instead is tagging latest (or production or staging), but fetching the sha for that tag and returning it.

end

private

def fetch_images(repository_name, registry_id, ecr)
images = []
next_token = nil
while
resp = ecr.describe_images({
repository_name: repository_name,
registry_id: registry_id,
next_token: next_token,
})

images.push(resp.image_details)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this will create an array of arrays containing image_details responses https://docs.aws.amazon.com/sdkforruby/api/Aws/ECR/Client.html#describe_images-instance_method

Maybe images += resp.image_details?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I flatten it later but that's a better way

next_token = resp.next_token

if resp.next_token.nil?
break
end
end
Copy link
Contributor

@stevehodgkiss stevehodgkiss Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: begin/end/while is an alternative.

begin
  # ..
end while next_token.nil?

Copy link
Contributor Author

@patrobinson patrobinson Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks one of my tests... and not the pagination test I just added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, looks like it should be !next_token.nil?

images.flatten
end
end
end
end
59 changes: 59 additions & 0 deletions spec/stack_master/parameter_resolvers/latest_container_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
RSpec.describe StackMaster::ParameterResolvers::LatestContainer do
let(:config) { double(base_dir: '/base') }
let(:stack_definition) { double(stack_name: 'mystack', region: 'us-east-1') }
subject(:resolver) { described_class.new(config, stack_definition) }
let(:ecr) { Aws::ECR::Client.new(stub_responses: true) }

before do
allow(Aws::ECR::Client).to receive(:new).and_return(ecr)
end

context 'when matches are found' do
before do
ecr.stub_responses(:describe_images, image_details: [
{ registry_id: '012345678910', image_digest: 'decafc0ffee', image_pushed_at: Time.utc(2015,1,2,0,0), image_tags: ['v1'] },
{ registry_id: '012345678910', image_digest: 'deadbeef', image_pushed_at: Time.utc(2015,1,3,0,0), image_tags: ['v2'] }
])
end

it 'returns the latest one' do
expect(resolver.resolve({'repository_name' => 'foo'})).to eq '012345678910.dkr.ecr.us-east-1.amazonaws.com/foo:v2'
end
end

context 'when there are multiple tags including latest' do
before do
ecr.stub_responses(:describe_images, image_details: [
{ registry_id: '012345678910', image_digest: 'decafc0ffee', image_pushed_at: Time.utc(2015,1,2,0,0), image_tags: ['v1'] },
{ registry_id: '012345678910', image_digest: 'deadbeef', image_pushed_at: Time.utc(2015,1,3,0,0), image_tags: ['latest', 'v2'] }
])
end

it 'does not return the latest tag' do
expect(resolver.resolve({'repository_name' => 'foo'})).to eq '012345678910.dkr.ecr.us-east-1.amazonaws.com/foo:v2'
end
end

context 'when no matches are found' do
before do
ecr.stub_responses(:describe_images, image_details: [])
end

it 'returns nil' do
expect(resolver.resolve({'repository_name' => 'foo'})).to be_nil
end
end

context 'when registry_id is passed in' do
before do
ecr.stub_responses(:describe_images, image_details: [
{ registry_id: '012345678910', image_digest: 'decafc0ffee', image_pushed_at: Time.utc(2015,1,2,0,0), image_tags: ['v1'] },
])
end

it 'passes registry_id to describe_images' do
expect(ecr).to receive(:describe_images).with(repository_name: "foo", registry_id: "012345678910", next_token: nil)
resolver.resolve({'repository_name' => 'foo', 'registry_id' => "012345678910"})
end
end
end
1 change: 1 addition & 0 deletions stack_master.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Gem::Specification.new do |spec|
spec.add_dependency "aws-sdk-s3", "~> 1"
spec.add_dependency "aws-sdk-sns", "~> 1"
spec.add_dependency "aws-sdk-ssm", "~> 1"
spec.add_dependency "aws-sdk-ecr", "~> 1"
spec.add_dependency "diffy"
spec.add_dependency "erubis"
spec.add_dependency "colorize"
Expand Down