Skip to content

add typed router configs in examples#17065

Merged
phlax merged 7 commits intoenvoyproxy:mainfrom
greymatter-io:examples-add-router-type
Jun 24, 2021
Merged

add typed router configs in examples#17065
phlax merged 7 commits intoenvoyproxy:mainfrom
greymatter-io:examples-add-router-type

Conversation

@justincely
Copy link
Copy Markdown
Contributor

@justincely justincely commented Jun 21, 2021

Signed-off-by: Justin Ely justincely@gmail.com

Following discussion in #15017, the router should include the typed config.

Commit Message: add typed router configs in examples
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Justin Ely <justincely@gmail.com>
Signed-off-by: Justin Ely <justincely@gmail.com>
@phlax phlax self-assigned this Jun 21, 2021
@phlax
Copy link
Copy Markdown
Member

phlax commented Jun 21, 2021

@justincely im wondering about this - mostly i have been trying to keep the configs as simple as possible

so im wondering if its necessary - at least in all of the examples

@phlax
Copy link
Copy Markdown
Member

phlax commented Jun 21, 2021

and whether we can have some kinda default here

@justincely
Copy link
Copy Markdown
Contributor Author

@phlax I guess that depends on how the discussion plays out in that issue. @htuch mentioned that a typed_config without a type URL is rather invalid. And the internals will fail these configs through the asserts if you don't do an opt build.

I rather like the idea of defaults personally though

@phlax
Copy link
Copy Markdown
Member

phlax commented Jun 22, 2021

I rather like the idea of defaults personally though

im wondering if we can remove the typed_config: {} altogether

@justincely
Copy link
Copy Markdown
Contributor Author

ohhh, i see what you're saying. Sure - i'll give that a try. I wouldn't expect that to work, just from prior art, but that would be awesome if that wasn't needed.

@phlax
Copy link
Copy Markdown
Member

phlax commented Jun 22, 2021

I wouldn't expect that to work

i dont think it does work now - but i think it would be preferable to adding empty or default config - not sure if that makes sense or whether not including at all is possible or would mean something different

Signed-off-by: Justin Ely <justincely@gmail.com>
@justincely
Copy link
Copy Markdown
Contributor Author

so, interestingly it seems to work fine. Just deleted the typed_config and the examples I've tried work fine. Pushing it up now, feel free to test and check. I did the gzip and the fault_injection examples. Config dumps just show an empty config, but server doesn't crash and handles traffic fine.

@justincely
Copy link
Copy Markdown
Contributor Author

though, neither of us thinking this worked leads me to believe this should be documented better :)

@phlax
Copy link
Copy Markdown
Member

phlax commented Jun 22, 2021

so, interestingly it seems to work fine

wow! theres a lot of these dotted around the code base

$ git grep "typed_config: {}"
...

. Just deleted the typed_config and the examples I've tried work fine. Pushing it up now, feel free to test and check.

if it passes ci - then i think we should remove

though, neither of us thinking this worked leads me to believe this should be documented better :)

yep!

@justincely
Copy link
Copy Markdown
Contributor Author

@phlax do you want me to remove all empty typed config? I just did the router in the examples; since that was the specifics of the discussion, but I can do the others if you think it's prudent?

@phlax
Copy link
Copy Markdown
Member

phlax commented Jun 23, 2021

do you want me to remove all empty typed config?

i think yes - the problem is that if you leave some they inevitably grow and spread through copypasta

also - i think its a better pattern/clearer to remove these so probs best to just remove everywhere

Signed-off-by: Justin Ely <justincely@gmail.com>
Signed-off-by: Justin Ely <justincely@gmail.com>
@phlax
Copy link
Copy Markdown
Member

phlax commented Jun 23, 2021

the one thing im not sure about is removing from test data - probs its best - but if we can get ci to pass ill ping for confirmation

Signed-off-by: Justin Ely <justincely@gmail.com>
@justincely
Copy link
Copy Markdown
Contributor Author

sounds good

Signed-off-by: Justin Ely <justincely@gmail.com>
@phlax
Copy link
Copy Markdown
Member

phlax commented Jun 24, 2021

seems like test pass

the one thing im not sure about is removing from test data

im thinking if tests pass this shouldnt be a problem

Copy link
Copy Markdown
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @justincely

@phlax phlax merged commit a20c7fc into envoyproxy:main Jun 24, 2021
@justincely justincely deleted the examples-add-router-type branch June 24, 2021 12:56
chrisxrepo pushed a commit to chrisxrepo/envoy that referenced this pull request Jul 8, 2021
Signed-off-by: Justin Ely <justincely@gmail.com>
Signed-off-by: chris.xin <xinchuantao@qq.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Signed-off-by: Justin Ely <justincely@gmail.com>
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

Successfully merging this pull request may close these issues.

2 participants