Skip to content

config: C++ proto reflection based conversion of vN -> v(N+1)alpha.#8431

Merged
htuch merged 10 commits intoenvoyproxy:masterfrom
htuch:v3alpha-converter
Oct 4, 2019
Merged

config: C++ proto reflection based conversion of vN -> v(N+1)alpha.#8431
htuch merged 10 commits intoenvoyproxy:masterfrom
htuch:v3alpha-converter

Conversation

@htuch
Copy link
Copy Markdown
Member

@htuch htuch commented Sep 29, 2019

The basic idea is that vN and v(N+1)alpha messages of the same name are
duck typed and reflection is used to upgrade from vN to v(N+1)alpha.
v(N+1) now has an Any field (when a deprecation has happened) to smuggle
the vN deprecated fields across for any code that might still depend on
it.

Risk level: Low (unused library)
Testing: Unit tests added.

Signed-off-by: Harvey Tuch htuch@google.com

The basic idea is that vN and v(N+1)alpha messages of the same name are
duck typed and reflection is used to upgrade from vN to v(N+1)alpha.
v(N+1) now has an Any field (when a deprecation has happened) to smuggle
the vN deprecated fields across for any code that might still depend on
it.

Risk level: Low (unused library)
Testing: Unit tests added.

Signed-off-by: Harvey Tuch <htuch@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8431 was opened by htuch.

see: more, trace.

@lizan
Copy link
Copy Markdown
Member

lizan commented Sep 30, 2019

Do we really need the Any? At policy level we shouldn't allow a field is deprecated without alternative, at implementation level I think you might be able to use the UnknownFieldSet in message to smuggle it, no?

@lizan lizan self-assigned this Oct 1, 2019
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
ASSERT(target_field_descriptor != nullptr);

// These properties are guaranteed by protoxform.
ASSERT(prev_field_descriptor->type() == target_field_descriptor->type());
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.

Is there anyway we can check whether the descriptor of next_message is translated by protoxform from that of prev_message? This seems generally true for configs in this repo but not outside.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think this will be done by code outside of the version converter.. e.g. the HCM filter factory instantiation will need to be explicitly aware of the envoy.foo.v2, envoy.foo.v3alpha, envoy.foo.v4 structure to be able to stage a series of upgrades anyway, so there will be some additional code to recognize types that are in the core Envoy API/UDPA and interpret the version numbers.

htuch added 2 commits October 3, 2019 18:31
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch htuch merged commit 989dc72 into envoyproxy:master Oct 4, 2019
@htuch htuch deleted the v3alpha-converter branch October 4, 2019 19:47
nandu-vinodan pushed a commit to nandu-vinodan/envoy that referenced this pull request Oct 17, 2019
…nvoyproxy#8431)

The basic idea is that vN and v(N+1)alpha messages of the same name are
duck typed and reflection is used to upgrade from vN to v(N+1)alpha.
v(N+1) now has an Any field (when a deprecation has happened) to smuggle
the vN deprecated fields across for any code that might still depend on
it.

Risk level: Low (unused library)
Testing: Unit tests added.

Signed-off-by: Harvey Tuch <htuch@google.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.

3 participants