config: support extrinsic yaml anchor declarations

[goaway]

Commit Message: config: support extrinsic yaml anchor declarations
Additional Description: This relatively simple change adds the ability to declare yaml anchors outside the bounds of Envoy configuration structure, for later aliasing and merging, using a new !ignore tag. This allows for bootstrap and static configuration to include variable content without requiring secondary pre-parsing or interpolation, by leveraging the yaml parser cheaply and directly. In the case of Envoy Mobile, this dramatically improves startup latency, and it likely has applications elsewhere.
Risk Level: Moderate
Testing: Locally with Envoy Mobile, and Unit test in CI
Docs Changes: Configuration examples

Signed-off-by: Mike Schore [email protected]

[goaway]

@snowp

[wrowe]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16543 (comment) was created by @wrowe.

see: more, trace.

[wrowe]

Is there an appropriate docs change you've identified so this is highlighted in the release? Seems rather buried.

You may need to merge main again to pick up newer coverage thresholds for quic and pass CI.

[goaway]

Merged main and added documentation*.

[snowp]

Thanks this looks great to me! Maybe include a version note about this?

@phlax this change might be interesting to you if you want to take a look, maybe there are use cases for this in existing documentation/examples

[phlax]

thanks @goaway i defo like the idea but i had some issues with trying to load the config with the python parser

i would really appreciate if you could move the config (edit: this) to a literalinclude as it means the config will be tested loading in Envoy

[phlax]

the issue with this afaict is that it means the config is not loadable with a standard yaml parser - at least trying to load it with pyyaml failed

yaml.constructor.ConstructorError: could not determine a constructor for the tag '!ignore'
  in "<unicode string>", line 1, column 18:
    dynamic_sockets: !ignore

this ticket yaml/pyyaml#266 suggests it should be possible with a full yaml loader (ie not safe_load) but i couldnt seem to make it work

[goaway]

I'm not a subject matter expert, but based on my read of https://yaml.org/spec/1.2/spec.html this a correct and adherent usage of an application-specific local tag. This tag informs Envoy's (application-specific) loader of the type of the deserialized representation (in this case "nothing"). In theory, a validator should not complain, because this is valid yaml. In practice, a given YAML loader might complain, based on how it's configured, because it believes it should be doing something with the tag, and hasn't been told what to do. From my own Googling, various YAML loading tools, including pyyaml, provide various mechanisms for informing the library of how to handle custom local tags.

See also 3.3.2 and 3.3.3:
https://yaml.org/spec/1.2/spec.html#id2769212

Especially:

That said, tag resolution is specific to the application. YAML processors should therefore provide a mechanism allowing the application to override and expand these default tag resolution rules.

I haven't attempted, but this stack overflow post appears to explain how to configure pyyaml to handle a custom tag:
https://stackoverflow.com/questions/43058050/creating-custom-tag-in-pyyaml

[phlax]

yep - i didnt try too hard to get it to work tbh and yaml parsers can a bit "ymmv" - defo not a blocker

[goaway]

Actually, looks like it's going to be necessary to pass bazel-bin/configs/example_configs_validation - let me see what I can come up with. :)

[goaway]

Thank you all for the comments so far!

@snowp, what format would I use for adding the release notes?

[phlax]

...what format would I use for adding the release notes?

see docs/root/version_history/current.rst

[goaway]

Thank you @phlax!

[wrowe]

Merging main should get you past the format_pre breakage.

[goaway]

I think the CI failures might be spurious - this was green before the docs change. Not sure if I can /retest

[mattklein123]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16543 (comment) was created by @mattklein123.

see: more, trace.

[phlax]

My personal preference is to go with the YAML standard vs. the magic prefix approach, since the latter is by definition "magical" and is more likely to break someone.

i think there is some confusion here

the !ignore syntax is much more likely to break someone - it may be part of the standard - but it is not standard

the x-definitions way is "standard" yaml - there isnt even any need for the x- part we could just make it definitions - so there is not much magical here - its just ignored on the basis of having an x- prefix much like http headers are.

Prefixing with !ignore is no more/less magical than prefixing with x-

Using x- to denote extension fields is more standard broadly - eg that is what is used in docker-compose - but also more widely x- is used for custom field - for example in http headers

this patch solves a very real problem for Envoy Mobile so my preference would be to merge it.

i appreciate that and welcome the addition of this - but its a significant change and should be thought about properly and not rushed

If we are concerned about user confusion I think we could talk about leaving it undocumented, though my preference would still be to document it since it seems like it's a feature of the YAML standard.

my concerns have nothing to do with docs

broadly my concerns are as follows

• its not supported out-of-the box by parsers - at least not by the python parser and certainly not in safe mode (which you want as yaml also allows for embedded external code execution)
• that it wont work with yaml-language-server and therefore wont be usable once we have added vscode support

for clarity - here are 2 examples showing the different approaches

!ignore dynamic_sockets:
  &admin_address
  address: 127.0.0.1
  port_value: 9901

admin:
  address:
    socket_address: *admin_address

x-dynamic_sockets:
  &admin_address
  address: 127.0.0.1
  port_value: 9901

admin:
  address:
    socket_address: *admin_address

i have tried again parsing using both load and safe_load - afaict hacking SafeLoader and SafeDumper is required so this pushes more work onto anyone wanting to parse config files (at least if they support this syntax)

wrt using this with yaml-language-server - i have checked and wasnt able to find any evidence that the !ignore syntax is supported (or to the contrary) - the x-format will work inherently

there is still a further challenge of expressing this in jsonschema to make it work with yaml-language-server with either way

overall, im -1 to the approach implemented here because:

• its more likely to break end users parsing of config
• it pushes the implementation of a custom parser on to end users - i think unnecessarily
• extension fields with a x- prefix is more standard (eg docker-compose/http/etc)
• !ignore is much less likely to work with vscode or other editors

[goaway]

Not to belabor the point, but we are not hacking PyYAML. We may differ on our notion of "out-of-the-box", but we are making use of an official public interface as intended, supported and documented. Please see:
https://pyyaml.org/wiki/PyYAMLDocumentation

safe_load is documented to work by not building non-standard Python types by default, but allowing one to explicitly register known types to be loaded. This is what makes it "safe".

[phlax]

We may differ on our notion of "out-of-the-box",

my intended meaning of out-of-the-box is that there is no need for any custom classes

ie yaml.safe_load(content) just works

but we are making use of an official public interface as intended,

yes - but with a custom class with a fair amount of complexity to do so

[phlax]

...also without reading the Envoy source code - there is not any obvious way for an end user to know how to do this

[phlax]

discussing this further with @goaway offline - i did a bit of a survey of other projects/services that use yaml config to see how they dealt with the same problem to define and allow anchors

this is roughly what i found (please feel free to correct if my quick research is wrong):

ansible:
uses vars for block of definitions

kubernetes:
allows definitions but they are done inline - no provision for separate block afaict

docker-compose:
uses x- prefix for ignored blocks used for definitions

travis:
uses _ prefix for ignored blocks used for definitions

bitbucket:
uses definitions for block of definitions

circleci:
allows definitions without code block (&anchorname is placed after the defining config)

azure:
doesnt allow anchors

i didnt find any examples that use ! syntax for anchor definitions

[mattklein123]

OK thanks @phlax I think I understand the issue better now. I didn't realize that !ignore is magic either here. Here is my suggestion:

1. Pick whatever magic string (x- prefix or whatever) we think is most standard in the industry.
2. Clearly document/warn that use of this feature may break generic parsing of the YAML and/or require special code at the parsing site. Potentially also link to the internal Python changes in the validator to wire this up.

I think that would be a reasonable compromise to move this forward.

The other alternative would be to just not document it at all, though that would not be my first choice.

[mattklein123]

OK sorry for the back and forth on this. I just discussed this further with @goaway and I better understand this issue.

I agree that using tags vs. magic strings is a better way of handling this. As such my opinion is we should stick with the !ignore approach. Either way the YAML loader if verifiers, etc. will need to be modified for whatever we choose.

So concretely I think we should:

1. Keep the code change
2. Add a warning in the documentation that discusses that use of this feature may break projects that load Envoy YAML without further modification, and also link to the example changes in the Python validation code in the project. I think this is sufficient to move forward.

[mattklein123]

Awesome, thanks. @phlax let's chat early in the morning tomorrow on Slack and resolve any remaining issues so we can move this forward. It's fairly urgent we get this merged so if we can't agree to this quickly we will need to carry a patch in Envoy Mobile.

[mattklein123]

docs needs fixing

[phlax]

not sure this is any different from python lexer - i think if not just use python as that is what is used everywhere else

[phlax]

actually - checking - seems like neither are used anywhere atm - i still think just use python

[phlax]

just to confirm the !ignore syntax wont work with yaml-language-server

it throws a couple of errors

it might be possible to edit vscodes' settings.json and workaround - but the point is we just want to distribute envoy.schema.json and not have to tell people to add additional settings beyond registering the schema

[phlax]

a fixed key such as definitions would obv work out-of-the-box at least as far as validation goes

the x-syntax could also be expressed with pattern properties - https://json-schema.org/understanding-json-schema/reference/object.html#pattern-properties

[mattklein123]

@phlax objection noted but I think it's time to move this forward. I think it's fine if this feature does not work with the language server. That's why we have the warning. We can ship and iterate on this later if needed. (@goaway please do a follow up for the docs comments but I don't think we need to wait for more CI on this.)