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

consul validate only accepts .json or .hcl files => impossible to roll out + validate configs e.g. with puppet #3620

Closed
jenshadlich opened this issue Oct 26, 2017 · 15 comments

Comments

@jenshadlich
Copy link

jenshadlich commented Oct 26, 2017

consul version for both Client and Server

Client: v1.0.0
Server: v1.0.0

consul info for both Client and Server

Client:

agent:
	check_monitors = 0
	check_ttls = 0
	checks = 0
	services = 0
build:
	prerelease = 
	revision = 51ea240
	version = 1.0.0
consul:
	known_servers = X
	server = false
runtime:
	arch = amd64
	cpu_count = 2
	goroutines = 44
	max_procs = 2
	os = linux
	version = go1.9.1

Operating system and Environment details

Linux / Ubuntu 14.04

Description of the Issue (and unexpected/desired result)

My use puppet to roll out consul configurations. Puppet appends a random string to the filename e.g. my-service.json20171026-6787-5on04gwhich breaks the validation. This makes the validation pointless because we now have to disable this step. It would be good to control the filename check e.g. by command line argument.

Execution of '/usr/local/bin/consul validate /etc/consul.d/client/my-service.json20171026-6787-5on04g' returned 1: Config validation failed: Missing or invalid file extension for "/etc/consul.d/client/my-service.json20171026-6787-5on04g". Please use ".json" or ".hcl".

Reproduction steps

Create a config file with a file extension other than .json or .hcl and validate.

@jenshadlich
Copy link
Author

I guess this behaviour came in with #3480

@magiconair
Copy link
Contributor

@jenshadlich this is not a mistake and was always working like that inside a config directory. Only for the main config file the .json extension was not required. You can change your puppet roll-out rules to put the random id before the extension, e.g. my-service-20171026-6787-5on04g.json

@magiconair magiconair self-assigned this Oct 26, 2017
@magiconair
Copy link
Contributor

and yes, this was added as part of #3480 also to make it unambiguous to the parser whether it should parse a JSON or a HCL file.

@jenshadlich
Copy link
Author

@magiconair thanks for the feedback. Can you please tell me what I need to do make puppet do that? (has to work with puppt 3.7 btw)

@magiconair
Copy link
Contributor

No, unfortunately not. However, my guess is that you can customize the format of the random string that is being added and if all fails you could just add another .json extension so that you would get my-service.json20171026-6787-5on04g.json

@magiconair
Copy link
Contributor

A better option is to validate the entire config directory instead of the file since validating incomplete config fragments is also no longer supported. You would probably get a data_dir missing error or something similar.

@jenshadlich
Copy link
Author

jenshadlich commented Oct 26, 2017

@magiconair I'm afraid that it's not possible to simply tell puppet to add a .json extension when using the validate_cmd (it creates a temp file). I found really awkward ways to work around it.

It would be great if you could provide a way to disable the file extension check or allow forcing it via --json or something similar.

In theory the puppet code is very straight forward like this (if not validate would be so strict):

file { "${consul_clientconfig_dir}/${name}.json":
    ...
    content      => template("${module_name}/template.json.erb"),
    validate_cmd => "${consul_binary_dir}/consul validate %",
    notify       => Exec['consul_reload']
    ...
}

All workarounds will create a mess and are harder to read and maintain.

@magiconair
Copy link
Contributor

@jenshadlich This will break as soon as someone wants to use .hcl files since the only backwards compatible way of doing this would be to assume that files w/o extension are JSON. Lets try to find a way that works here since this will solve this for everybody. This should have never worked like it did before in the first place IMO.

@magiconair
Copy link
Contributor

We could loosen the requirement on where the extension has to be. Instead of at the and we could just accept it anywhere. This makes it more ambiguous but for all practical purposes we should be fine unless someone creates a JSON file and calls it foo.hcl.json.

@magiconair
Copy link
Contributor

The question about -json and -hcl flags is whether they should apply to all files, just the main file or just the ones without extensions.

@jenshadlich
Copy link
Author

jenshadlich commented Oct 26, 2017

@magiconair I understand your concerns. In my case the validation shall take place for exactly one file. So -json or -hcl flags would work perfectly.

Validating the extension anywhere feels a bit strange even though it would also do the job. About the scope of the flags: I personally think the most logical thing is that it applies to all files. I would assume that the client knows what is is doing when setting those flags.

Another option / a little tweak could to fallback to consul's preferred config file format (I don't really care if it's .json or .hcl unless it's deterministic) if no matching extension is available.

@martinseener
Copy link

Yeah, i also think that relying on a filename's extension is so DOS-era. The "validate" command should instead try to determine the type from the content (like it's done for most pictures for example).
Nevertheless, we use the following validate_cmd string for validation at the moment. It basically creates a symlink to the temporary file which is invalidated afterwards automatically, since puppet only creates a temporary file, thus it's a bit more secure: "${::consul::bin_dir}/consul validate $(ln -s % /tmp/consul-config.json && echo \"/tmp/consul-config.json\") > /dev/null"

But you can see it in the puppet module which my colleague referenced already

magiconair added a commit that referenced this issue Oct 27, 2017
Starting with Consul 1.0 all config files must have a '.json' or '.hcl'
extension to make it unambigous how the data should be parsed. Some
automation tools generate temporary files by appending a random string
to the generated file which obfuscates the extension and prevents the
file type detection.

This patch adds a -config-format option which can be used to override
the auto-detection behavior by forcing all config files or all files
within a config directory independent of their extension to be
interpreted as of this format.

Fixes #3620
@magiconair
Copy link
Contributor

@martinseener I need to think about that. IMHO, config files should have only one format but if they can be in multiple formats you need to have a way of telling them apart unambiguously. Usually, I prefer things to be explicit so that operators know what they are dealing with and don't have to guess.

I've written a PR which adds a -config-format (hcl|json) option which forces the the format independent of the extension. We could also return to the 'it's JSON unless .hcl' approach but then the people who want to use only HCL would have a problem.

I'll discuss this with the team internally to see what they think.

@martinseener
Copy link

@magiconair sure! i think the PR your wrote is a good approach and should ease some things. Thanks alot for that!

@jenshadlich
Copy link
Author

@magiconair great! many thanks!

slackpad pushed a commit that referenced this issue Oct 31, 2017
* config: refactor ReadPath(s) methods without side-effects

Return the sources instead of modifying the state.

* config: clean data dir before every test

* config: add tests for config-file and config-dir

* config: add -config-format option

Starting with Consul 1.0 all config files must have a '.json' or '.hcl'
extension to make it unambigous how the data should be parsed. Some
automation tools generate temporary files by appending a random string
to the generated file which obfuscates the extension and prevents the
file type detection.

This patch adds a -config-format option which can be used to override
the auto-detection behavior by forcing all config files or all files
within a config directory independent of their extension to be
interpreted as of this format.

Fixes #3620
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants