Add support for top level configuration #79

jalvz · 2020-11-03T14:27:06Z

Closes #70

elasticmachine · 2020-11-03T14:30:57Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: [Pull request #79 updated]
Start Time: 2020-11-18T18:47:19.915+0000
Duration: 2 min 9 sec

ruflin

Overall LGTM. How will the files inside look exactly? Can there be more than 1 file? What is the content? Would be good to also directly specify this here. If not in the spec, in the PR description.

ycombinator · 2020-11-04T13:02:06Z

versions/1/spec.yml

+      type: folder
+      name: input
+      required: true
+      additionalContents: false


Related to @ruflin's comment, it would be good to flesh out contents here with a spec for the file(s) that could be be contained in this folder.

jalvz · 2020-11-05T12:56:21Z

Thanks both, good idea. Added now, let me know what do you think

ruflin · 2020-11-05T13:11:27Z

versions/1/agent/spec.yml

+    required: true
+    additionalContents: false
+    contents:
+    - description: Package-level template file


Probably a better description needed here.

The part I was also looking for is to define, if the content of the file itself is an array or not. Also, are there some required like type? Let me give you 3 examples:

inputs: - type: foo value: bar

- type: foo value: bar

type: foo value: bar

Which option is it?

@ycombinator Do we have examples already where we also validate the content of certain assets?

What I am trying spec out is the optional existence of /<package>/<version>/agent/input/template.yml.hbs. I was expecting to adhere to the same constraints than data streams templates: why a type or any other field must be defined inside? Is not up to the integration to make sense of whatever is in there?

There is nothing defined for data_stream/<name>/agent/stream/ contents that I can see either (even thou the default file name is stream.yml.hbs)

On the stream.yml.hbs files the rules is, it can't be an array and alway only contain a single input. Is this the same for input templates? Is the format the last option from above?

Ah, I wasn't aware of that. Yes, it would be the same then, last option.

@jen-huang How will this work on the Kibana side. How will Kibana know which file to use? Convention? Or do we need to reference the file in the input part in the manifest.yml? As inputs is an array there, could there be multiple files in this directory?

I don't have a preference about the actual term, but this is the concern:

manifest.yml

policy_templates: - name: apm-server inputs: - type: apm vars: - name: name default: my-default-name template_path: ./agent/input/template.yml.hbs

agent/input/template.yml.hbs

name: {{name}}

elastic-agent.yml

inputs: - id: 0e682c50-183a-11eb-916d-71d55143d422 name: ?????? revision: 1 type: apm

What will be the value of the ?????? field? my-default-name, as per the template variable, or whatever the user set in UI when creating the integration?

Instead, forcing a (eg.) config key:

elastic-agent-2.yml

inputs: - id: 0e682c50-183a-11eb-916d-71d55143d422 name: apm-1 # user defined in UI revision: 1 type: apm config: # everything from the template name: my-default-name

Makes sense?

@jalvz I think the problem you describe is something that could be run into today with streams & stream templates. We generate streams in elastic-agent.yml like this:

streams: - id: logfile-system.auth data_stream: dataset: system.auth type: logs paths: - /var/log/auth.log* - /var/log/secure* ...

If the package author has id or data_stream.* fields in their stream template, we would run into the conflict problem you described.

For inputs, we have more of these kind of "reserved" generated field names:

inputs: - id: b8bc5300-edfd-11ea-905a-819b5c00fe02 name: system-1 revision: 1 type: logfile use_output: default meta: package: name: system version: 0.5.3 data_stream: namespace: default streams: ...

I'm not sure if adding a config field at the input level is the right approach though, given same nature of the problem on the stream level. I guess for streams, we've relied on package authors not using id and data_stream.* in their templates. I wonder if this is something we can enforce via a blocklist during package validation? I recall we added package validation to enforce template syntax correctness, maybe a blocklist can be added there?

@jalvz @ruflin WDYT?

I see @jen-huang, thanks.

In the spirit of developer-friendliness, I think we should prevent clashes from happening - for someone outside the ingest management team this problem is not obvious at all (in fact, I didn't realize it can happen with streams already) and consequences are pretty much unknown?

The issue with blocklists is to remember to update them when new fields are added, and more importantly how to make them work backwards: what if a new field foo is added to the spec but I already have it in a template?

I think this deserves a discussion before moving the spec to GA, maybe considering a breaking change. I agree with @ycombinator that the more strict the better, and currently there is no definition at all for the stream templates AFAICS.

Is there any other problem with requiring a single top level key in the templates (named config or whatever) other than it is not done for streams? Alternatively, Kibana could "inject" such key behind the scenes, based eg. on the template file name or something like that. But it wouldn't be so explicit.

WDYT?

We definitively need the validation that these keys are not used and if a package uses them, it should be reject. We have a spec versioning, so if we add new fields as "reserved" we will increase the package spec and the new package will follow the new spec. I doubt that at the moment we have anything around that could not be fixed without a breaking change, but we should check. If we validate, we will find it.

Assuming we don't have any conflicts, what is the best format we should use?

Sure, it will be detected but it will likely force the package maintainer to change the template and deal with 2 variants of the same configuration. To that end the best preventive action would be to come up with keys that will never ever crash (assuming you know a clash in a future version can happen), so why don't be explicit and require it upfront?

OTOH, json-schema is already meant for validation, so adding more validation alongisde means that the json-schema spec becomes less reliable (a developer might be easily confused after carefully following the spec and then find out that their package doesn't work).

Anyways, since that is a separate problem I filed #85 and removed the config bit from this PR, let me know how it looks now.

jalvz · 2020-11-11T10:51:08Z

@jen-huang @ycombinator @ruflin does this look right now? something else missing?

ruflin · 2020-11-12T08:32:07Z

What we miss is the definition of the base format of the template file, see conversation above with @ycombinator . But maybe it is best to move forward even without it, so we can still change it (see comment below).

@ycombinator We also have a bit a chicken / egg problem here. We are adding something new to the spec but haven't fully tested it with Kibana and an actual package yet, so it might still change. What is our best approach here?

@jen-huang @ycombinator If you are fine with the changes, lets get them in.

ycombinator · 2020-11-12T08:49:38Z

Ideally the spec will be defined first and it's changes rolled out into elastic-package and the integrations repo. This ensures that all existing and new packages will conform to the spec. Then we make the corresponding functionality changes in Kibana, etc.

Also ideally the spec can be as strict as possible so as to catch as many issues as possible early on in a package's development.

Given that, I would suggest for this PR:

to spec out the base definition of the template file (per Add support for top level configuration #79 (comment))
get @jen-huang's LGTM on this PR from a "do we think this will work for Kibana" perspective.
then we merge it, create/modify a package with the changes, and try it out in Kibana. If something needs to be tweaked, we make another PR to the spec and rinse/repeat. But hopefully this step can be avoided as much as possible.

jen-huang

LGTM for Kibana support

ruflin · 2020-11-18T12:14:22Z

@jalvz Any chance you could post here the final content / structure of the template file? I think we are aligned but want to make sure we have it also written in YAML here.

Seems there is a conflict with the generated file.

mtojek

Correct me if I'm wrong, but I understand that this change is backward compatible (it's just an extension). We should be safe with adding it.

Please add a new folder with a sample package for testing purposes in:
https://github.com/elastic/package-spec/tree/master/code/go/internal/validator/test/packages

and enable it in this file: https://github.com/elastic/package-spec/blob/master/code/go/pkg/validator/validator_test.go

jalvz · 2020-11-18T14:53:16Z

23e436d

mtojek

I'm afraid that the CI still fails for this PR.

mtojek · 2020-11-18T15:19:18Z

versions/1/agent/agent.spec.yml

+spec:
+  # Everything under here follows JSON schema (https://json-schema.org/), written as YAML for readability
+  type: object
+  additionalProperties: true


I understand that this schema file describes the *.yml.hbs file, which is Handlebars template. I think it's not possible to easily define a JSON schema for this file as the JSON format can be broken by template placeholders.

mtojek · 2020-11-18T15:22:47Z

versions/1/agent/spec.yml

+      type: file
+      pattern: '^.+.yml.hbs$'
+      required: true
+      $ref: "./agent.spec.yml"


As stated above, the .yml.hbs file is not a strict JSON/YAML file if you plan to use placeholders. You can't use (reference) a schema for this.

right, thanks

mtojek

Ship it!

mtojek · 2020-11-18T15:59:21Z

code/go/internal/validator/test/packages/input_template/agent/input/template.yml.hbs

@@ -0,0 +1 @@
+{}


nit: It would be nice to put a real content in at least one file, so this template.yml.hbs won't be so mysterious.

@jalvz Can you open a PR with this? Because it is exactly what I'm looking for to have an example.

@mtojek These test packages are great. Example and testing in one go.

BTW, you could even put a short APM Example in here to make it more concrete.

@ruflin I added 7673874, is not that what you were looking for?
I don't think I can use this file as a real hbs example because it is referenced in the test (correct me if Im wrong)

Also didn't want to add an APM-like config because this can be anything, it has exactly the same structure as the stream templates.

Got it. I missed that you added two template files. The part I stumble over is that you used group. This is just an example I assume. It can all be on the top level like foo: bar?

If we could use apm example here, @mtojek will know best. My preference is always to have a real example if possible.

jalvz · 2020-11-18T18:52:41Z

@jalvz Any chance you could post here the final content / structure of the template file?

7673874

Merging now, thanks all

Follow up to elastic/package-spec#79. Kibana needs `template_path` to ascertain which input template file to read from to build the agent YAML. This PR lets the registry serve that field at the input level, if defined.

jen-huang · 2020-11-19T23:19:26Z

Hi all, I opened elastic/package-registry#655 to allow the package registry to serve the new template_path field on input level.

Add support for top level configuration

cc6a407

make update

7a1dd4a

jalvz mentioned this pull request Nov 4, 2020

[Fleet] Add support for new template path above data stream folders elastic/kibana#82599

Closed

ruflin reviewed Nov 4, 2020

View reviewed changes

ycombinator reviewed Nov 4, 2020

View reviewed changes

jalvz added 5 commits November 5, 2020 12:00

spec out contents

b21e9aa

make update

26463bd

split spec

b81ba33

Merge remote-tracking branch 'elastic/master' into top-level-vars

579d6c2

make update

e0cee2e

ruflin reviewed Nov 5, 2020

View reviewed changes

ph added the Team:Fleet Label for the Fleet team label Nov 6, 2020

jalvz added 6 commits November 9, 2020 11:37

make regex stricter

d0a53df

add template_path property to the spec

7da2203

Merge remote-tracking branch 'elastic/master' into top-level-vars

c7f74bc

make update

ec9ac45

update description

0dd7916

make update

865880a

jalvz added 6 commits November 12, 2020 10:53

Spec out contents of template file

e4e2e92

make update

e8f7f89

remove config key

3dee9ca

make update

0d978ee

Merge remote-tracking branch 'elastic/master' into top-level-vars

bda6234

make update

327abde

jen-huang approved these changes Nov 17, 2020

View reviewed changes

mtojek self-requested a review November 18, 2020 13:58

mtojek suggested changes Nov 18, 2020

View reviewed changes

Add test

23e436d

jalvz added 2 commits November 18, 2020 15:54

Merge remote-tracking branch 'elastic/master' into top-level-vars

a93da15

make update

cce099d

mtojek suggested changes Nov 18, 2020

View reviewed changes

and remove spec

9640d2c

mtojek self-requested a review November 18, 2020 15:57

mtojek approved these changes Nov 18, 2020

View reviewed changes

mtojek reviewed Nov 18, 2020

View reviewed changes

Add example file

7673874

jalvz merged commit 133f96e into elastic:master Nov 18, 2020

jen-huang mentioned this pull request Nov 19, 2020

Allow template_path field on inputs elastic/package-registry#655

Merged

jalvz mentioned this pull request Nov 23, 2020

Integrate with Elastic Agent elastic/apm-server#4004

Closed

15 tasks

rw-access pushed a commit to rw-access/package-spec that referenced this pull request Mar 23, 2021

Improve command logging (elastic#79)

66b92d5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for top level configuration #79

Add support for top level configuration #79

jalvz commented Nov 3, 2020

elasticmachine commented Nov 3, 2020 •

edited

Loading

Build stats

ruflin left a comment

ycombinator Nov 4, 2020

jalvz commented Nov 5, 2020

ruflin Nov 5, 2020

jalvz Nov 5, 2020

ruflin Nov 6, 2020

jalvz Nov 6, 2020

ruflin Nov 6, 2020

jalvz Nov 16, 2020

jen-huang Nov 16, 2020

jalvz Nov 17, 2020

ruflin Nov 17, 2020

jalvz Nov 17, 2020

jalvz commented Nov 11, 2020

ruflin commented Nov 12, 2020

ycombinator commented Nov 12, 2020

jen-huang left a comment

ruflin commented Nov 18, 2020

mtojek left a comment

jalvz commented Nov 18, 2020

mtojek left a comment

mtojek Nov 18, 2020

mtojek Nov 18, 2020

jalvz Nov 18, 2020

mtojek left a comment

mtojek Nov 18, 2020

ruflin Nov 19, 2020

ruflin Nov 19, 2020

jalvz Nov 19, 2020

ruflin Nov 19, 2020

jalvz commented Nov 18, 2020

jen-huang commented Nov 19, 2020

Add support for top level configuration #79

Add support for top level configuration #79

Conversation

jalvz commented Nov 3, 2020

elasticmachine commented Nov 3, 2020 • edited Loading

💚 Build Succeeded

Build stats

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalvz commented Nov 5, 2020

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

jalvz commented Nov 11, 2020

ruflin commented Nov 12, 2020

ycombinator commented Nov 12, 2020

jen-huang left a comment

Choose a reason for hiding this comment

ruflin commented Nov 18, 2020

mtojek left a comment

Choose a reason for hiding this comment

jalvz commented Nov 18, 2020

mtojek left a 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

mtojek left a 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

jalvz commented Nov 18, 2020

jen-huang commented Nov 19, 2020

elasticmachine commented Nov 3, 2020 •

edited

Loading