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

Docker resource #1566

Merged
merged 5 commits into from Apr 24, 2017

Conversation

Projects
None yet
3 participants
@chris-rock
Copy link
Member

chris-rock commented Mar 15, 2017

This PR ships 3 new docker resources:

docker_container

describe docker_container('an-echo-server') do
  it { should exist }
  it { should be_running }
  its('id') { should_not eq '' }
  its('image') { should eq 'busybox:latest' }
  its('repo') { should eq 'busybox' }
  its('tag') { should eq 'latest' }
  its('ports') { should eq [] }
  its('command') { should eq 'nc -ll -p 1234 -e /bin/cat' }
end

describe docker_container(id: 'e2c52a183358') do
  it { should exist }
  it { should be_running }
end

docker_image

describe docker_image('alpine:latest') do
  it { should exist }
  its('id') { should_not eq '' }
  its('image') { should eq 'alpine:latest' }
  its('repo') { should eq 'alpine' }
  its('tag') { should eq 'latest' }
end

describe docker_image('alpine:latest') do
  it { should exist }
end

describe docker_image(id: '4a415e366388') do
  it { should exist }
end

docker

describe docker.containers do
  its('images') { should_not include 'u12:latest' }
end

describe docker.images do
  its('repositories') { should_not include 'inssecure_image' }
end

describe docker.version do
  its('Server.Version') { should cmp >= '1.12'}
  its('Client.Version') { should cmp >= '1.12'}
end

describe docker.object(id) do
  its('Configuration.Path') { should eq 'value' }
end

docker.containers.ids.each do |id|
  # call docker inspect for a specific container id
  describe docker.object(id) do
    its(%w(HostConfig Privileged)) { should cmp false }
    its(%w(HostConfig Privileged)) { should_not cmp true }
  end
end

This resource started as a port from https://github.com/dev-sec/cis-docker-benchmark and was refactored to cover more use cases.

Fixes #71

@arlimus arlimus changed the title add docker resource WIP: add docker resource Mar 16, 2017

@chris-rock chris-rock force-pushed the chris-rock/docker-resource branch from 1c42800 to a55200d Mar 21, 2017

@chris-rock chris-rock force-pushed the chris-rock/docker-resource branch 3 times, most recently from c4923ab to b1d391c Apr 17, 2017

@adamleff
Copy link
Contributor

adamleff left a comment

This is so cool. I can't wait to see this firmed up and ready-to-go.


# docker

Use the `docker` InSpec audit resource to test configuration data for docker daemon.

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

I suppose it's not just "configuration data"... maybe "configuration and runtime data"?


A `docker` resource block declares the configuration data to be tested:

describe csv('file') do

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

Just a reminder note to actually flesh this out with real/accurate docs before we merge. Lots of file/csv references in here, though I totally realize this is a WIP.

.add(:exists?) { |x| !x.entries.empty? }
filter.connect(self, :containers)

attr_reader(:containers)

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

Can we remove the ( ) here? It's customary to not include them when declaring attributes (i.e. attr_reader :containers)

.add(:exists?) { |x| !x.entries.empty? }
filter.connect(self, :images)

attr_reader(:images)

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

Same comment as above - can we remove the ( )?

.add(:running_for, field: 'runningfor')
.add(:sizes, field: 'size')
.add(:status, field: 'status')
.add(:exists?) { |x| !x.entries.empty? }

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

This feels like we need a default exists? accessor in FilterTable that can be added with .add_accessor. Obviously don't address this as part of this PR, but I've seen this multiple times so it feels ripe for consolidation.


# checks if the system is Debian 8+, Ubuntu 16.04+ and RHEL 7+
def is_systemd_system
return true if inspec.os.redhat? && inspec.os.release.to_i >= 7

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

This kinda makes me wish init system detection was part of the os resource because we're now defining what makes a systemd system here and in the service resource.

This comment has been minimized.

@chris-rock

chris-rock Apr 19, 2017

Member

difficult, since init system is not necessarily tight to a specific operating system. Chef is the best example and uses runit. I know we need to extend this in future. Maybe we kick this out of the resource, let me think about it

This comment has been minimized.

@aaronlippold

aaronlippold Apr 19, 2017

Member

agree, and another example is cases where I have seen folks put in systemd on older distros, so perhaps detecting it in a more generic way is the way to go.

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

@chris-rock Oh sure, totally. The Chef example is a good example, but it's also an example of where an installed application is providing its own process supervision vs. what the OS default is. It's pretty hard for someone to take a CentOS 7 box and convert it to be upstart natively, so it still feels like there's a pretty strong connection to the OS and the expected init system.

We're at least making that assumption now in two resources... Not sure the right way to solve it, but duplicating it feels wrong. 🙂 Shouldn't block this, but we should probably add an issue to solve init detection if more resources are going to rely on it.

This comment has been minimized.

@chris-rock

chris-rock Apr 21, 2017

Member

@adamleff Yes indeed, that is why we assume the default in our resources which is the only sane thing. If customers have a custom one, they can use runit_service etc. Detection of the init system is not easy, especially since they have fallbacks implemented, too. I am going to remove that feature for now, since I am not considering it as stable

# TODO: handle short ids
def initialize(opts = {})
# fallback for simple strings
if opts.is_a?(String)

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

I'm confused about this section... aren't all IDs and names strings? I don't think there's anything that not a string...

This comment has been minimized.

@chris-rock

chris-rock Apr 19, 2017

Member

Yes, that is not finalized

def container_info
return @info if defined?(@info)
opts = @opts
@info = inspec.docker.containers.where { id == opts[:id] || names == opts[:name] }

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

Should this be name == opts[:id] instead of names? I think we use the field name in the #where block rather than the accessor name.

This comment has been minimized.

@chris-rock

chris-rock Apr 19, 2017

Member

No, since docker only exposes names, see https://github.com/chef/inspec/pull/1566/files#diff-a405d35eca4d46fa85c19029d6f3b79bR23. Technically multiple names could be attached, but I have not seen that in real world ...

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

Yup, you're right - I was totally looking at something different.

@opts = { image: opts } if opts.is_a?(String)

unless @opts[:image].nil?
unless @opts[:image].index(':').nil?

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

Would you be OK changing this to an if !... else instead of unless ... else? I think most Rubyists try to shy away from unless ... else flows.

This comment has been minimized.

@chris-rock

chris-rock Apr 19, 2017

Member

I am okay with that...

# do sanitizion of input values
@opts = { image: opts } if opts.is_a?(String)

unless @opts[:image].nil?

This comment has been minimized.

@adamleff

adamleff Apr 19, 2017

Contributor

I'd love to see this broken out into some helper methods for easier comprehension and easier testing...

@opts[:repo] = parse_repo_from_image(@opts[:image])
@opts[:tag] = parse_tag_from_image(@opts[:image])
# ... etc ...

This comment has been minimized.

@chris-rock

chris-rock Apr 19, 2017

Member

Great idea!

@chris-rock chris-rock force-pushed the chris-rock/docker-resource branch from b1d391c to 7a96926 Apr 24, 2017

@chris-rock chris-rock changed the title WIP: add docker resource Docker resource Apr 24, 2017

chris-rock added some commits Mar 15, 2017

add docker resource
Signed-off-by: Christoph Hartmann <chris@lollyrock.com>
refactor docker resource
Signed-off-by: Christoph Hartmann <chris@lollyrock.com>
update docker docs
Signed-off-by: Christoph Hartmann <chris@lollyrock.com>
split up resources and add unit tests
Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

@chris-rock chris-rock force-pushed the chris-rock/docker-resource branch from 7a96926 to 9810cac Apr 24, 2017

@adamleff
Copy link
Contributor

adamleff left a comment

Superb work, @chris-rock. This is gonna be great.

@adamleff adamleff merged commit 218bda9 into master Apr 24, 2017

3 checks passed

DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adamleff adamleff deleted the chris-rock/docker-resource branch Apr 24, 2017

@adamleff adamleff removed the in progress label Apr 24, 2017

nsdavidson added a commit to nsdavidson/inspec that referenced this pull request May 5, 2017

Docker resource (inspec#1566)
* add docker, docker_container, and docker_image resources

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment