extensions: Add extensions metadata.yaml by phlax · Pull Request #16618 · envoyproxy/envoy

phlax · 2021-05-21T16:32:46Z

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message: extensions: Add extensions metadata.yaml
Additional Description:

This is a breakout pr from #16496 as that will require a lot of change

This PR:

adds an extensions_metdata.yaml file
uses the new metadata file in protodoc
removes generate_extension_db.py
adds validate_extension_db.py to
- ensure that old <> new metadata match
- consolidate existing metadata validation in one place

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2021-05-21T16:32:51Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #16618 was opened by phlax.

see: more, trace.

Signed-off-by: Ryan Northey <ryan@synca.io>

phlax · 2021-05-21T18:53:30Z

@htuch this is hopefully ready for review

Signed-off-by: Ryan Northey <ryan@synca.io>

dmitri-d · 2021-05-21T21:50:41Z

docs/requirements.txt

    #   -r docs/requirements.txt
    #   sphinx
    #   sphinx-rtd-theme
+    #   sphinx-tabs


Nit: if you could make changes to docs/requirements.txt in a separate PR, it would make life slightly easier for maintainers downstream.

@dmitri-d on this point i would like to push back - adding yaml to the docs build requirements is not needed without this PR - it makes sense to add it here i think

the extraneous changes here are produced by pip-compile not sure why they hadnt been set before - but im guessing that dependabot may be to blame for at least the hash ordering

dmitri-d · 2021-05-21T21:59:10Z

Could you mention how to update metadata in https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#adding-new-extensions?

dmitri-d · 2021-05-21T22:29:12Z

looks good, other than a couple of nits above.

htuch

Great, thanks, a few comments.
/wait

htuch · 2021-05-23T13:18:08Z

source/extensions/extensions_metadata.yaml

+  categories:
+  - envoy.wasm.runtime
+  security_posture: unknown
+  status: alpha


There were a bunch of comments around these status, i.e. that this alpha/unknown status can't change until we do deliberate review and change to threat model, for Wasm runtimes. Can you move these as well to this file? Also pleaes check we haven't missed other places there are similar comments we are missing.

this PR hasnt removed the other comments yet - but yep - i will go through and add any comments i can find from the existing metadata registration

@htuch i have gone through all of the BUILD files with wasm in the path (and quite a few of the others) and none contain comments

can you point me to any specific examples please ?

Hah, not merged yet (I thought it had), see https://github.com/envoyproxy/envoy/pull/16057/files#diff-85815a26c8f765868f9c242a1968320d41e9c5798cbe94358339f3d99d0a70c2R16. We will need to update that PR.

source/extensions/extensions_metadata.yaml

htuch · 2021-05-23T13:20:43Z

tools/extensions/validate_extensions.py

+# Validate extension metadata
+
+# This script expects a copy of the envoy source to be located at /source
+# Alternatively, you can specify a path to the source dir with `ENVOY_SRCDIR`


I think it should default to using $PWD and then in Docker set /source via ENVOY_SRCDIR. The only place we expect to see /source is in Docker.

this is very temporary - in the next PR most of this will be gone

also - re "$PWD" - this cant do that because its inside the bazel build dir - a path must be passed, there cant be a default that is not hardcoded.

this is one of the reasons for doing this - we wont need the envoy_srcdir to be set at all once this code is trimmed down - the bazel env should have anything that is required

also worth mentioning this is just a repurposing of the previous generate_extension_db script - hence the inclusion of the /source default etc

Why do we need to merge this specific part of the PR if it goes in the next one?

its just maintaining the equivalence of generate_extension_db - ie ensuring it works in the same circumstances (of being called from docs/build.sh)

htuch · 2021-05-23T13:21:48Z

tools/extensions/validate_extensions.py

+
+
+if __name__ == '__main__':
+    sys.exit(validate_extensions())


Does this file need unit tests?

yes - can i do it in the next PR when much of this code will be removed - im more than happy to shift this to a Checker class and add the appropriate tests

htuch · 2021-05-23T13:22:41Z

tools/extensions/validate_extensions.py

+    }
+
+
+def compare_old_and_new(old_db, new_db):


Is this just to validate we're doing the right thing as scaffolding? Should it be deleted before merging?

yes and no - its to ensure that transitioning from the old macro way of setting the metadata and the new yaml way nothing is lost

...and for that reason i think its important to keep it until the macro way has been removed completely

If you want to preserve this over two PRs, then can you mark everything with a comment that will be gone in the next PR, so we don't have to review it with scrutiny?

ive added TODOs to all of the soon-to-be-removed code

tools/extensions/validate_extensions.py

source/extensions/extensions_metadata.yaml

Signed-off-by: Ryan Northey <ryan@synca.io>

lizan · 2021-05-24T17:06:05Z

I'm concerned about this as it is centralizing those metadata into a single file, which means it is harder for downstream builds to define those metadata in the extensions living there. While they won't included in our doc, but it is a good practice anyway. It is also making it harder when we move extensions to contrib/, separating some extensions from standard builds. Thoughts?

phlax · 2021-05-24T17:10:59Z

Thoughts?

im very conscious of this - afaia the main downstream is envoy-mobile - so i have done quite a bit of work to figure out what will change and ease the transition

i dont think it will make it harder to separate contrib - but will defo require a slightly different path

Signed-off-by: Ryan Northey <ryan@synca.io>

htuch

Let's merge this as is and iterate on next step. Generally prefer to see everything complete before merging, but this will make the larger lift presumably easier with a checkpoint.

Signed-off-by: Ryan Northey <ryan@synca.io>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 21, 2021

phlax changed the title ~~extensions: Add extensions metadata.yaml~~ [WIP] extensions: Add extensions metadata.yaml May 21, 2021

phlax marked this pull request as draft May 21, 2021 16:33

phlax force-pushed the extensions-metadata-yaml branch from c127c6a to 0e0042b Compare May 21, 2021 18:01

extensions: Add extensions metadata.yaml

12bc41e

Signed-off-by: Ryan Northey <ryan@synca.io>

phlax force-pushed the extensions-metadata-yaml branch from 04f8c6f to 12bc41e Compare May 21, 2021 18:51

phlax changed the title ~~[WIP] extensions: Add extensions metadata.yaml~~ extensions: Add extensions metadata.yaml May 21, 2021

phlax requested a review from htuch May 21, 2021 18:53

phlax marked this pull request as ready for review May 21, 2021 18:53

tools/

2a68e27

Signed-off-by: Ryan Northey <ryan@synca.io>

dmitri-d reviewed May 21, 2021

View reviewed changes

htuch suggested changes May 23, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label May 23, 2021

phlax added 2 commits May 24, 2021 14:40

returns-1-on-fail

225e23f

Signed-off-by: Ryan Northey <ryan@synca.io>

explicit-stable-status

82686b1

Signed-off-by: Ryan Northey <ryan@synca.io>

repokitteh-read-only bot removed the waiting label May 24, 2021

add-new-extensions-metadata-info

d350bd1

Signed-off-by: Ryan Northey <ryan@synca.io>

phlax requested a review from htuch May 24, 2021 14:06

phlax added 5 commits May 26, 2021 12:14

pr-feedback

2f720e1

Signed-off-by: Ryan Northey <ryan@synca.io>

Merge branch 'main' into extensions-metadata-yaml

740f274

Signed-off-by: Ryan Northey <ryan@synca.io>

improve-comparison

8deede1

Signed-off-by: Ryan Northey <ryan@synca.io>

update-metatdata

7e42100

Signed-off-by: Ryan Northey <ryan@synca.io>

format

a69c5df

Signed-off-by: Ryan Northey <ryan@synca.io>

phlax force-pushed the extensions-metadata-yaml branch from 3efd11d to a69c5df Compare May 26, 2021 14:44

htuch approved these changes May 27, 2021

View reviewed changes

phlax merged commit f481a00 into envoyproxy:main May 27, 2021

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

extensions: Add extensions metadata.yaml (envoyproxy#16618)

49ff984

Signed-off-by: Ryan Northey <ryan@synca.io>

mmizutani mentioned this pull request Dec 31, 2021

[WIP] tooling: Add jsonschema dump of API (...cont) #15100

Closed

Conversation

phlax commented May 21, 2021

Uh oh!

repokitteh-read-only bot commented May 21, 2021

Uh oh!

phlax commented May 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitri-d commented May 21, 2021

Uh oh!

dmitri-d commented May 21, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phlax May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lizan commented May 24, 2021

Uh oh!

phlax commented May 24, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phlax May 26, 2021 •

edited

Loading