added docker support #357

Justin-DynamicD · 2017-08-07T19:06:11Z

added two new install_methods: auto and docker (neither set to default).

docker now uses the official docker container to deploy but still calls config_hash and mounts the config_dir for consistency. Image can be chosen, but network and certain key variables are hardcoded per consul recomendations for use in a container by the host. See: https://hub.docker.com/_/consul/

auto will look for the [docker0] network interface. If found, it uses docker, on failure it falls back to the url default.

The default remains at "url", disabling this feature.

solarkennedy · 2017-08-08T02:41:07Z

manifests/params.pp

@@ -40,7 +41,7 @@
  $ui_package_ensure     = 'latest'
  $ui_package_name       = 'consul_ui'
  $user                  = 'consul'


Many of the complexity in this PR revolves around not using the user and group variables.
Can you simply set them to be undef in params.pp if the install method is docker instead? (then we won't need case statements throughout the codebase, and instead only have them in one place.

Same with manage_group and manage_user

Also init_style

Not sure why I didn't think of doing that in the prams vs trying to remap it in the init.pp ... lost in the moment. But yes: I deliberately attempted to clear user statements as they aren't needed out of the container.

I'll rework it tonight/next few days. Thanks for the input.

solarkennedy · 2017-08-08T02:43:15Z

manifests/init.pp

 #
 # [*install_method*]
-#   Valid strings: `package` - install via system package
+#   Valid strings: `auto`    - use docker if available, fallback to url if not


I'm going to veto this.
I think it adds complexity and surprise.
Just have a default method in the module and make users set it "automatically" themselves with their own puppet code.

oh fine, take the fun out of it!

solarkennedy · 2017-08-08T02:48:10Z

manifests/run_service.pp

  if $::consul::join_wan {
    exec { 'join consul wan':
      cwd       => $::consul::config_dir,
      path      => [$::consul::bin_dir,'/bin','/usr/bin'],
      command   => "consul join -wan ${consul::join_wan}",
+      onlyif    => "/usr/bin/test ! -e ${consul::config_dir}/docker_used",


Can you get away with a normal if statement here instead of the docker_used workaround?

probably need to assemble $command variable then add a prefix if install_method == docker. If you look close, in order to run consul in the container I need to call it slightly differently (prefix it with "docker exec -t consul <...>"). So ... yeah if we "veto auto" we can get away from that.

Justin-DynamicD · 2017-08-10T17:38:02Z

took way to long to get things to pass, but there you go.

Justin-DynamicD · 2017-08-11T21:03:46Z

Running internally at the company against Ubuntu 14, 16, and CentOS 7.9 in both docker and non-docker configs. Found a minor bug where a container env variable was writing a new local.json. Removing to allow the user complete control over config through the module.

solarkennedy · 2017-08-14T14:47:46Z

manifests/install.pp

@@ -4,16 +4,19 @@
 #
 class consul::install {

-  if $::consul::data_dir {
-    file { $::consul::data_dir:
+  if ($::consul::data_dir) and ($::consul::install_method != 'docker') {


Why is there no data_dir? I would prefer to have a permanently mounted volume outside the container. That way if I have to nuke docker for some reason, puppet can reproduce the image and run and all it has to do is pickup the data on disk

Mostly because of the nature of consul+puppet itself makes it unnecessary. In a production environment you should have 3/5/n masters so all info will replicate, and with a puppet config all checks and services will populate on first run so it's simply unnecessary to store that information beyond the life-cycle of the container (remember volumes will persists in upgrades, just not when you deliberately rm the container)

I was basically thinking about how to leave the host as "clean" as possible.

solarkennedy · 2017-08-14T14:49:42Z

manifests/params.pp

@@ -11,7 +11,8 @@
  $config_defaults       = {}
  $config_dir            = '/etc/consul'
  $config_hash           = {}
-  $config_mode           = '0660'
+  $config_mode           = '0664'


Can you leave it at 660 to continue to allow potentially sensitive configs unreadable by random users of the system or hacked nobodys or whatever?

this will break container volume mounts as the container itself does not share the users on the host, and thus counts as "other users". It was set as read-only to prevent tweaking. Keep in mind that in a container scenario the surface area of attack will be the containers themselves (that hosts all open ports). I can add a "config_mode_real" so it can be flipped back and forth based on the deployment mode?

solarkennedy · 2017-08-14T14:50:26Z

manifests/reload_service.pp

@@ -19,12 +19,16 @@
      $http_addr = $::consul::http_addr
    }

+    case $::consul::install_method {
+      'docker': { $command = "docker exec -t consul consul reload -http-addr=${http_addr}:${consul::http_port}" }


does docker exec need a -t here? shouldn't need a tty right?

That's a good question. I have it stuck in my head that's run into issues without the TTY enabled ... but honestly I could be completely wrong. Ill give a test internally and see what's up with that.

solarkennedy · 2017-08-14T14:51:11Z

manifests/run_service.pp

+      $docker_command = 'agent'
+    }
+
+    # Docker Install


remove the comment, doesn't really provide value here

solarkennedy · 2017-08-14T14:53:10Z

manifests/check.pp

@@ -85,11 +85,12 @@

  $escaped_id = regsubst($id,'\/','_','G')
  File[$::consul::config_dir]
-  -> file { "${consul::config_dir}/check_${escaped_id}.json":
+  -> file { "${consul::config_dir}/check_${escaped_id}.json" :


unless this is a new change in the style, can you remove the space before the colon?

Formatting habbit brought on from a vscode plugin for puppet. Can be pulled.

solarkennedy · 2017-08-14T14:53:32Z

manifests/run_service.pp

+    }
+
+    # Docker Install
+    docker::run { 'consul' :


space before the colon

solarkennedy · 2017-08-14T14:53:58Z

manifests/config.pp

    purge   => $purge,
    recurse => $purge,
  }
-  -> file { 'consul config.json':
+  -> file { 'consul config.json' :


solarkennedy · 2017-08-14T14:54:12Z

manifests/install.pp

-  if $::consul::data_dir {
-    file { $::consul::data_dir:
+  if ($::consul::data_dir) and ($::consul::install_method != 'docker') {
+    file { $::consul::data_dir :


solarkennedy · 2017-08-14T14:54:25Z

manifests/watch.pp

@@ -134,11 +134,12 @@
  }

  File[$::consul::config_dir]
-  -> file { "${consul::config_dir}/watch_${id}.json":
+  -> file { "${consul::config_dir}/watch_${id}.json" :


Justin-DynamicD · 2017-08-22T00:17:02Z

Spaces removed and data_dir added.

Justin-DynamicD · 2017-08-31T22:32:31Z

This should be ready for another review. Please let me know.

solarkennedy · 2017-09-07T14:18:48Z

This is still on my radar and I will review.

Can you confirm: are you running this code (in the PR) for real in your infrastructure? or is it a variant of it?

Justin-DynamicD · 2017-09-07T16:17:33Z

This code is the code deployed at my company. We are using a mix of Ubuntu (14 and 16) as well as a few CentOS boxes, and I deploy both Docker and URL mode so I have a pretty good cross section, but we are a small shop of only ~80 boxes on premise. It's currently used in tandem with Registrator to do most of the service registration, but I do have a few specific service callouts being done.

solarkennedy · 2017-10-03T01:53:53Z

Alright, hitting the button...

Justin-DynamicD · 2017-10-05T16:37:17Z

Sweet. Ill remove my branch and open a new one If i come up with more ideas.

added docker support

Justin King and others added 17 commits August 4, 2017 21:18

initial install concepts merged

dc61648

import discovery ... cool

8b5f1d3

few more fixes

401c77d

commit changes

ee5f61a

flip it

0329610

remove version

76e04ee

step up version

61e1cb9

cleanup init_style

9d25228

back pedel unless statements

cc369ba

misconfigured startup flag

640f3a4

update watch

b1472ab

simple fix of file permissions

2059494

remove miused http_addr

b4e26a6

attempting to trigger a reload

3ba5e09

docker restart by reload

1867055

cleanup wan docker oversight

16b6d37

test commit

d5e0c00

solarkennedy reviewed Aug 8, 2017

View reviewed changes

Justin King and others added 6 commits August 8, 2017 22:49

push new build

e0ebc34

merge again

fbe4370

sync up

b2acdb8

fix real

bf7f73b

version bump

908e64c

oops

ffaba1d

Justin-DynamicD closed this Aug 9, 2017

Justin-DynamicD reopened this Aug 9, 2017

Justin King added 2 commits August 9, 2017 15:02

format changes

aeb5d36

whitespace is the enemy

ea3fcda

Justin King and others added 5 commits August 9, 2017 22:22

revert version default to 0.7.4

986abdd

move config folder to original pp

ca85547

restore "real" users

6530e5b

he told me enough, he told me you killed him

6c82e3d

No, *I* am your father.

b2a037b

remove local_config generation from env variable

c0c2fe1

solarkennedy reviewed Aug 14, 2017

View reviewed changes

Justin-DynamicD closed this Aug 21, 2017

format changes and mount data volume

272d51a

Justin-DynamicD reopened this Aug 22, 2017

Justin King added 2 commits August 21, 2017 17:30

remove unnesisary comment

583a6fa

correct service management flag

0f47406

solarkennedy merged commit d19ee1f into voxpupuli:master Oct 3, 2017

spuder pushed a commit to spuder/puppet-consul that referenced this pull request Feb 25, 2020

Merge pull request voxpupuli#357 from Justin-DynamicD/master

0d650e0

added docker support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added docker support #357

added docker support #357

Justin-DynamicD commented Aug 7, 2017

solarkennedy Aug 8, 2017

solarkennedy Aug 8, 2017

solarkennedy Aug 8, 2017

Justin-DynamicD Aug 8, 2017 •

edited

Loading

solarkennedy Aug 8, 2017

Justin-DynamicD Aug 8, 2017

solarkennedy Aug 8, 2017

Justin-DynamicD Aug 8, 2017

Justin-DynamicD commented Aug 10, 2017

Justin-DynamicD commented Aug 11, 2017

solarkennedy Aug 14, 2017

Justin-DynamicD Aug 14, 2017 •

edited

Loading

solarkennedy Aug 14, 2017

Justin-DynamicD Aug 14, 2017

solarkennedy Aug 14, 2017

Justin-DynamicD Aug 14, 2017 •

edited

Loading

solarkennedy Aug 14, 2017

solarkennedy Aug 14, 2017

Justin-DynamicD Aug 14, 2017

solarkennedy Aug 14, 2017

Justin-DynamicD Aug 14, 2017

solarkennedy Aug 14, 2017

Justin-DynamicD Aug 14, 2017

solarkennedy Aug 14, 2017

Justin-DynamicD Aug 14, 2017

solarkennedy Aug 14, 2017

Justin-DynamicD Aug 14, 2017

Justin-DynamicD commented Aug 22, 2017

Justin-DynamicD commented Aug 31, 2017

solarkennedy commented Sep 7, 2017

Justin-DynamicD commented Sep 7, 2017

solarkennedy commented Oct 3, 2017

Justin-DynamicD commented Oct 5, 2017

added docker support #357

added docker support #357

Conversation

Justin-DynamicD commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-DynamicD Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-DynamicD commented Aug 10, 2017

Justin-DynamicD commented Aug 11, 2017

Choose a reason for hiding this comment

Justin-DynamicD Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-DynamicD Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-DynamicD commented Aug 22, 2017

Justin-DynamicD commented Aug 31, 2017

solarkennedy commented Sep 7, 2017

Justin-DynamicD commented Sep 7, 2017

solarkennedy commented Oct 3, 2017

Justin-DynamicD commented Oct 5, 2017

Justin-DynamicD Aug 8, 2017 •

edited

Loading

Justin-DynamicD Aug 14, 2017 •

edited

Loading

Justin-DynamicD Aug 14, 2017 •

edited

Loading