Skip to content

Fix invalid config example#18093

Closed
exoego wants to merge 2 commits intoenvoyproxy:mainfrom
exoego:patch-1
Closed

Fix invalid config example#18093
exoego wants to merge 2 commits intoenvoyproxy:mainfrom
exoego:patch-1

Conversation

@exoego
Copy link
Copy Markdown

@exoego exoego commented Sep 13, 2021

Commit Message: docs: fix invalid config example
Additional Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link
Copy Markdown

Hi @exoego, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #18093 was opened by exoego.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18093 was opened by exoego.

see: more, trace.

@exoego exoego marked this pull request as ready for review September 13, 2021 07:47
@phlax
Copy link
Copy Markdown
Member

phlax commented Sep 13, 2021

hi @exoego thanks for this

we have a system to check standalone yaml files which should catch this (there is also one for snippets in docs but not sure why but it doesnt always catch issues)

would you be willing to move this snippet to a full config and replace the code-block with a literalinclude ?

if not i can land this and follow up myself, but as it stands i have no immediate way of testing the change

@phlax phlax self-assigned this Sep 13, 2021
@exoego
Copy link
Copy Markdown
Author

exoego commented Sep 13, 2021

@phlax I will try that!

@phlax
Copy link
Copy Markdown
Member

phlax commented Sep 14, 2021

this is failing the example configs test here https://dev.azure.com/cncf/envoy/_build/results?buildId=88372&view=logs&j=8c169225-0ae8-53bd-947f-07cb81846cb5&t=6b0ace90-28b2-51d9-2951-b1fd35e67dec&l=22272

its kinda why i wanted to move it to this validation, as it picks up more issues

in this case tho, im not sure if its the tests or the config that is at issue - checking the fail log in test artefacts it says

unknown file: Failure
C++ exception with description "Function call: customStatNamespaces
    The mock function has no default action set, and its return type has no default value set." thrown in the test body.

ill investigate a bit further, but it would also be helpful if one of us could try starting envoy with the config in this PR to see if it works outside tests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im thinking this might be the issue - iirc we have a wasm bin in the examples on a different path, checking...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, checking the other example wasm yaml files i think lib/envoy_filter_http_wasm_example.wasm should work

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I tried filename: "lib/envoy_filter_http_wasm_example.wasm" and filename: "../../../../../../../examples/wasm-cc/lib" but both does not work...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it needs to be just lib/envoy_filter_http_wasm_example.wasm

Copy link
Copy Markdown
Member

@phlax phlax Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also you will need a complete configuration - ie including a cluster

checking for existing docs that use a wasm example, there is one here docs/root/configuration/http/http_filters/wasm_filter.rst

if it has the required code, you could use the same example yaml file that this uses (in that case you do need to figure out the ../../)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im wondering if this is related #18127 given the error message relates to customStatNamespaces

Signed-off-by: TATSUNO Yasuhiro <ytatsuno.jp@gmail.com>
runtime: "envoy.wasm.runtime.v8"
code:
local:
filename: "../../../../../../../examples/wasm-cc/lib/envoy_filter_http_wasm_example.wasm"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by my calculation the relative path here is "../../../../examples/...

but im not sure if that is the error in ci

ill try testing this config when i get a chance

also im wondering whether it would be easier (and possible) to just link to the existing yaml file rather than linking to the wasm bin in a new one

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking on this further

this wont work

the path that needs to be correct is in the config_validation tests

that must be lib/x/y/z

therefore it would be a lot easier just pointing the literalinclude to the existing yaml example file (as done elsewhere i think for this reason)

Copy link
Copy Markdown
Author

@exoego exoego Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phlax
Unfortunately, there is no example yaml containing envoy.filters.network.wasm.
It seems a lot work to create an example for envoy.filters.network.wasm.
Is it ok to simply rewrite wasm_filter.rst and not including?

Comment thread docs/root/configuration/listeners/network_filters/_include/wasm_filters.yaml Outdated
@phlax
Copy link
Copy Markdown
Member

phlax commented Sep 16, 2021

@exoego see my last comment above

the easiest way for this to work is have the literalinclude point to the existing wasm yaml example file (and not having the yaml file point to the binary as here)

if that example file does not cover what is required, then you can just use lib/...wasm for the file

where it is tested in ci the path will always be lib/...wasm

@phlax
Copy link
Copy Markdown
Member

phlax commented Oct 4, 2021

@exoego it would be good to land this update/fix - let me know if there is anything i can do help with that

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 3, 2021

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 3, 2021
@phlax phlax removed the stale stalebot believes this issue/PR has not been touched recently label Nov 4, 2021
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 4, 2021

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 4, 2021
@github-actions
Copy link
Copy Markdown

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot closed this Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants