Puppet masterless provisioner manifest_file directory support has many interconnected issues

[Rican7]

⚠️ The following is kind of a brain dump after wrestling with something for hours through the night on a couple of occasions, haha. Sorry if it's at all unclear.

---

So I've personally been waiting for v0.8.0 for quite a bit now for really just one basic feature, supporting provisioning by manifest directory instead of pointing to a main file (#1446 / #1771). To work around this lack of behavior I previously had to actually copy and paste the internal provisioning execution template string and slightly modify it to allow for directories. It was gross, haha, but it worked.

Excited for the new release, I installed v0.8.0, removed the modified execute_command string from my config, changed the manifest_file to a directory instead of a dummy "run_me_first" file with imports, and ran Packer. Unfortunately, I was met with errors at provisioning runtime. :/

After spending about 4-6 hours wrestling with the issue, I think I've discovered the issues that are causing the failure of this new feature/fix. Interestingly enough, I don't think its just one issue that's causing the failure.

So where the error keeps arising is actually when the provisioner uploads the manifests.

See, the first sign of issue is a WARNING that's emitted during the upload:

WARNING: manifest_file should be a file. Use manifest_dir for directories

This looks to have originated from #2258 (commit 8990c38), where for some reason a warning was put in place to denote a "deprecation" of setting the manifest_file configuration attribute as a directory. Now, that makes sense in that there's also a manifest_dir attribute... except when I tried to remove the manifest_file attribute and leave the manifest_dir attribute as the actual directory instead, I received an error:

Template validation failed. Errors are shown below.

Errors validating build 'amazon-ebs'. 1 error(s) occurred:

• A manifest_file must be specified.

So while it seemed as if using a directory path for the manifest_file attribute was deprecated and manifest_dir was intended as its replacement, it doesn't actually seem like its possible to replace without hitting the validation error. :/

Hmm, ok, so I tried to be a tiny bit tricky instead to see if I could bypass the validation by setting the manifest_dir to the directory correctly and setting the manifest_file attribute to the value .. I figured this would attempt to just run the directory specified in manifest_dir and everything would work... but I was met with another error when the provisioner attempted to upload the manifests:

Build 'amazon-ebs' errored: Error uploading manifests: scp: /tmp/packer-provisioning/puppet/manifests/.: Is a directory

Yea, so while it looks like #1771 allowed for the manifest_file attribute to be set as a directory path, it looks like the scp communication tool either had a regression within the months time since that fix or never quite worked, as it won't actually upload the directory specified. Yea, damn.

Ok, so I tried one last thing. I figured that the last error maybe had something to do with the current directory workaround I was using to bypass the config validation issue, so I tried to set the manifest_file to the same value as the manifest_dir, very similarly to how I originally had it set in v0.7.5 except without the trailing file name (so /path/ instead of /path/file.pp)... but that errored out at the same exact scp location. Except this time the error was even more strange:

Build 'amazon-ebs' errored: Error uploading manifests: scp: /tmp/packer-provisioning/puppet/manifests/provisioning/puppet/manifests: No such file or directory

After a second of looking at it, I laughed. Do you see what happened there? It appended the manifest_file path onto the staging directory, so I ended up with a redundant and non-existent path. This looks to be because of another change made in #2258 (commit 742e556) that made it so that the path wasn't "basenamed" if the manifest_file attribute value was a directory instead of a file. Now, while that makes sense, it also means that the manifest_file can't be set to a path value that contains any children, or else they'll be incorrectly appended to the remote path. Yea, damn.

So yea, this has been quite the adventure, haha. I love this tool/product, so let me know if there's anything I can do to clarify any of this or help in any way. Thanks!

[cbednarski]

So while it seemed as if using a directory path for the manifest_file attribute was deprecated and manifest_dir was intended as its replacement, it doesn't actually seem like its possible to replace without hitting the validation error. :/

I reviewed this a few weeks ago when it was patched and my recollection is we're aiming for the following:

• The deprecation notice should complain, but not make your config invalid.
• Your config must specify either a manifest_file or a manifest_dir which includes a manifest file. I think in this case you should not need to specify the manifest_file option.
• For legacy reasons we support the value of manifest_dir being set in manifest_file instead (previously the manifest_dir option didn't exist and manifest_file was overloaded)

Because of the legacy case the code here is pretty confusing. Also paths are hard. First there's relative vs. absolute paths. Then there's symlinks. Then there's Windows. I will take another look at this.

@Rican7 I only pretend to know puppet; can you share a simple config that shows what you expect to work? It will help me debug your failure case, write a test, and perhaps clarify the documentation.

[Rican7]

The deprecation notice should complain, but not make your config invalid.

Yea, right now you actually can't follow the directions of the deprecation notice without it causing a validation error, since manifest_file is required as part of a puppet-masterless configuration.

Your config must specify either a manifest_file or a manifest_dir which includes a manifest file.

Yea, that'd make more sense. Unfortunately it doesn't work that way right now.

For legacy reasons we support the value of manifest_dir being set in manifest_file instead (previously the manifest_dir option didn't exist and manifest_file was overloaded)

Yea, makes sense. I see what you were trying to achieve with the deprecation/warning message, it just seems that the actual implementation is out of sync with the intent. :/

I only pretend to know puppet; can you share a simple config that shows what you expect to work?

Sure, I'll follow up in a bit.

[Rican7]

Orignally:

Alright, so originally (for v0.7.5), I had this working config (stripped all non-puppet provisioning bits):

{
  "type": "puppet-masterless",
    "staging_directory": "/tmp/packer-provisioning/puppet",
    "manifest_dir": "./provisioning/puppet/manifests/",
    "manifest_file": "./provisioning/puppet/manifests/_packer_bootstrap.pp",
    "module_paths": [
      "./provisioning/puppet/modules"
    ],
    "facter": {
      "puppet_provisioning_dir": "/tmp/packer-provisioning/puppet",
    }
}

In order for this to work around the manifest file/directory issue, I had a "bootstrap" file for puppet that contained:

# Packer required Puppet bootstrap
#
# TODO: Remove this file once Packer correctly supports running all manifests
# in a directory (instead of requiring a specific "main" file)
# (rel: https://github.com/mitchellh/packer/pull/1771)
import 'default.pp'
import '[a-z]*.pp'

Hopefully, with the v0.8.x puppet manifest directory fix:

So, what I believe the intended fix was supposed to enable/support was to be able to change that config to something like this:

{
  "type": "puppet-masterless",
    "staging_directory": "/tmp/packer-provisioning/puppet",
    "manifest_dir": "./provisioning/puppet/manifests/",
    "manifest_file": "./provisioning/puppet/manifests/",
    "module_paths": [
      "./provisioning/puppet/modules"
    ],
    "facter": {
      "puppet_provisioning_dir": "/tmp/packer-provisioning/puppet",
    }
}

... with the manifest file being a directory. Although, based on the warning message it seemed more like the intent was to support this:

{
  "type": "puppet-masterless",
    "staging_directory": "/tmp/packer-provisioning/puppet",
    "manifest_dir": "./provisioning/puppet/manifests/",
    "module_paths": [
      "./provisioning/puppet/modules"
    ],
    "facter": {
      "puppet_provisioning_dir": "/tmp/packer-provisioning/puppet",
    }
}

... which *has no manifest_file attribute at all.

Also, with this configuration, I'd expect for the manifests pointed in that local directory to be uploaded to /tmp/packer-provisioning/puppet/manifests.

I hope that makes sense.

[cbednarski]

I've done a bit of research on this. Here's what I've found so far:

1. As you observed, packer currently requires manifest_file to be specified.
2. manifest_dir is used to fill in the --manifestdir option to puppet apply.
3. --manifestdir was deprecated in puppet 3.6
4. --manifestdir will be removed entirely in puppet 4.0
5. In recent versions of puppet (presumably 3.6+) you can specify either a file or a directory as the argument to puppet apply.

What does this mean for Packer?

Packer is in a weird spot because we've created an abstraction around the manifest_file that is actually leaky. In puppet this is not really a file, it's simply the argument to the apply command. And manifest_dir -- which looks like it should be equivalent -- is actually a flag under the hood.

So what's actually broken?

Well, packer treats manifest_file and manifest_dir as special cases, and has different logic to upload a single file vs. upload a directory. Also, packer sends one of these options as an argument and the other as a flag. As a result, only manifest_file passes packer's validation rules BUT it can't actually be a directory because other logic assumes it is a file.

How do we fix it?

There are two paths forward:

1. Continue with the overloaded usage around manifest_file. Make it do what it should when it's actually a directory. Add lots of documentation and warnings to help users muddle through.
2. Continue with the manifest_dir option and allow it to overload manifest_file. Add lots of documentation and warnings to help users muddle through.

I don't know how this changes in puppet 4.0 but I suspect it works mostly the same as 3.8, except that the --manifestdir flag is outright removed. We actually ignore this if it is not set so it is probably easier to do #1.

I think this is a mess either way because either the config just lies to you and makes you confused or the internal logic is ridiculous.

So unfortunately, I think we have to solve this with docs explaining that you should just use manifest_file always, manifest_dir is not what you or I think it is, and let's hope puppet 4.0 doesn't make things worse.

TL;DR I'm going to fix manifest_file so it works with directories.

• Fix manifest_file so it works with directories
• Update docs to explain why manifest_file is a dirty, dirty lie
• Update docs to explain why manifest_dir isn't actually what I think it is, and also don't use it

[Rican7]

Wow, @cbednarski, I'm very impressed. You handled this very well.

The amount of research you did to not only look into Packer's internal handling, but also into Puppet's different versions and deprecations is awesome. Your explanation is solid and your plan makes a lot of sense.

Thank you, again, for taking the time to both look into this and execute on a plan to correct the behavior.

👏 👍

[Rican7]

👏
Thanks again!