config: shadow-based upgrading/downgrading for versioned messages. #9502
Conversation
This PR replaces the previous unknown field smuggling style of upgrading with shadow-based. Ultimately, we want to move away from shadows, but for 1.13.0 this simplifies by removing smuggling. In addition, we make two major changes that are backported from the WiP v3alpha branch: 1. Rather than make translateOpaqueConfig() smart and version/type aware, we switch to an iterative loadFromJson() that first attempts to load as v2 and then falls back to v3. This relies on the property that any v3 configuration that is ingestible as v2 has the same semantics in v2/v3, which holds due to the highly structured vN/v(N+1) mechanical transforms. 2. Support for downgrading to earlier versions is introduced to make it easier for tests that are within the v(N-1) subset of vN to continue to validate v(N-1), while still mostly using vN messages. This is particularly useful for ensuring that e2e integration tests are using v2 resources. As new v3 tests are introduced, tests will need to eschew downgrading, but this should be a viable approach for 1.13.0. Risk level: Low Testing: There's a bunch of new tests that need to be added for version converter, loadFroJson/unpackTo() and translateOpaqueConfig(). Putting this PR out for comment to get feedback. Part of envoyproxy#8082 Signed-off-by: Harvey Tuch <[email protected]>
htuch
requested review from
alyssawilk,
lizan,
mattklein123 and
snowp
as code owners
Signed-off-by: Harvey Tuch <[email protected]>
|
Tests added, ready for review in full. |
Signed-off-by: Harvey Tuch <[email protected]>
|
@mattklein123 @lizan would it be possible to get a review on this one? This is the last substantial non-tooling related PR prior to the mega-upgrade? |
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
There was a problem hiding this comment.
Very impressive. In general this makes sense to me and is conceptually a lot easier to understand. I'm flushing out some random small comments but will need to take one more pass through to make sure I understand it. Would definitely appreciate a review from @lizan also.
/wait
| message.SerializeToString(&s); | ||
| downgraded_message->msg_->ParseFromString(s); |
There was a problem hiding this comment.
Q: When we do this pattern is there any concern around non-deterministic ordering of the conversion? Might this get confusing when using dump tools or anything else which depends on the output? Should we be using the deterministic serialization options? Also maybe move this pattern to a utility function where we do it so we can better explain what we are doing?
There was a problem hiding this comment.
Since both descriptors are fully known, as are any extensions/options that should be material to representation or other debug use, and we're remaining at the abstraction level of messages, I think it's safe to do this. But, I would be keen to hear what @lizan has to say on this. I'll factor it out to a separate utility meanwhile.
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
…nvoyproxy#9502) * config: shadow-based upgrading/downgrading for versioned messages. This PR replaces the previous unknown field smuggling style of upgrading with shadow-based. Ultimately, we want to move away from shadows, but for 1.13.0 this simplifies by removing smuggling. In addition, we make two major changes that are backported from the WiP v3alpha branch: 1. Rather than make translateOpaqueConfig() smart and version/type aware, we switch to an iterative loadFromJson() that first attempts to load as v2 and then falls back to v3. This relies on the property that any v3 configuration that is ingestible as v2 has the same semantics in v2/v3, which holds due to the highly structured vN/v(N+1) mechanical transforms. 2. Support for downgrading to earlier versions is introduced to make it easier for tests that are within the v(N-1) subset of vN to continue to validate v(N-1), while still mostly using vN messages. This is particularly useful for ensuring that e2e integration tests are using v2 resources. As new v3 tests are introduced, tests will need to eschew downgrading, but this should be a viable approach for 1.13.0. Risk level: Low Testing: additional unit tests added to cover opaque config conversion, Any unpacking, JSON loading and version conversion. Part of envoyproxy#8082 Signed-off-by: Harvey Tuch <[email protected]> Signed-off-by: Prakhar <[email protected]>
This PR replaces the previous unknown field smuggling style of upgrading
with shadow-based. Ultimately, we want to move away from shadows, but
for 1.13.0 this simplifies by removing smuggling.
In addition, we make two major changes that are backported from the
WiP v3alpha branch:
Rather than make translateOpaqueConfig() smart and version/type
aware, we switch to an iterative loadFromJson() that first attempts
to load as v2 and then falls back to v3. This relies on the property
that any v3 configuration that is ingestible as v2 has the same
semantics in v2/v3, which holds due to the highly structured
vN/v(N+1) mechanical transforms.
Support for downgrading to earlier versions is introduced to make it
easier for tests that are within the v(N-1) subset of vN to continue
to validate v(N-1), while still mostly using vN messages. This is
particularly useful for ensuring that e2e integration tests are using
v2 resources. As new v3 tests are introduced, tests will need to
eschew downgrading, but this should be a viable approach for 1.13.0.
Risk level: Low
Testing: additional unit tests added to cover opaque config conversion, Any unpacking, JSON loading and version conversion.
Part of #8082
Signed-off-by: Harvey Tuch [email protected]