api: add API_VERSIONING.md. #8399

htuch · 2019-09-26T19:14:03Z

This captures the API versioning guidelines implied by
https://docs.google.com/document/d/1xeVvJ6KjFBkNjVspPbY_PwEDHC7XPi0J5p1SqUXcCl8/edit#heading=h.xgk8xel154p (from #6271)
and splits apart the Envoy API and internal implementation breaking change policies.

Some of the policy decisions (e.g. not allowing vNalpha to be hand edited, how we do manual breaking
changes, etc.) have been added to these guidelines based on recent experience with protoxform and
mechanical major version upgrade work, they are not part of the original stable API versioning work.

Risk level: Low
Testing: Formatting and docs build.

Fixes #8371

Signed-off-by: Harvey Tuch [email protected]

This captures the API versioning guidelines implied by https://docs.google.com/document/d/1xeVvJ6KjFBkNjVspPbY_PwEDHC7XPi0J5p1SqUXcCl8/edit#heading=h.xgk8xel154p and splits apart the Envoy API and internal implementation breaking change policies. Some of the policy decisions (e.g. not allowing vNalpha to be hand edited, how we do manual breaking changes, etc.) have been added to these guidelines based on recent experience with protoxform and mechanical major version upgrade work, they are not part of the original stable API versioning work. Risk level: Low Testing: Formatting and docs build. Fixes envoyproxy#8371 Signed-off-by: Harvey Tuch <[email protected]>

repokitteh-read-only · 2019-09-26T19:14:08Z

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

🐱

Caused by: #8399 was opened by htuch.

see: more, trace.

htuch · 2019-09-26T19:16:08Z

CC @markdroth @dfawley (please tag Eric as well, I forget his GH username) for review as well, thanks. This largely formalizes the earlier work on the stable versioning Doc, and makes it accessible to Envoy devs. There have been some changes to flow based on how this policy is likely to work out for v3.

markdroth · 2019-09-26T19:17:19Z

@ejona86

markdroth

This looks pretty good to me. I have a few comments, all fairly minor.

CONTRIBUTING.md

api/API_VERSIONING.md

markdroth · 2019-09-26T19:34:12Z

api/API_VERSIONING.md

+`envoy.config.bootstrap.v4`. The `envoy.config.bootstrap.v3` package will become the previous stable
+major version and support for `envoy.config.bootstrap.v2` will be dropped from the Envoy
+implementation. Note that some transitively referenced package, e.g.
+`envoy.config.filter.network.foo.v2` may remain at version 2 during this release, if no changes were


Is there any reason not to bump the version number of all packages when we introduce a new major version, even if the protos themselves did not change? It seems like that might make the dependency story a bit easier to deal with.

Yeah. I think this will be too cumbersome to management server vendors. We have O(100) protos. If we ask management server vendors to rewrite their logic for O(100) protos on each major version upgrade, this is a lot more expensive than rewriting for O(10) protos.

Within Envoy, we plan on doing some fancy reflection gymnastics to do runtime rewriting from v2 to v3alpha (I'm working on this right now), when API messages are mostly unchanged. Internally, Envoy will always consume from the latest API, e.g. v3alpha. I think it's not reasonable to expect every management server to invent clever techniques like this though, we should assume the bulk of them will cope with churn via manual toil.

markdroth · 2019-09-26T19:36:21Z

api/API_VERSIONING.md

+There will be no major `vN` initiative to address technical debt beyond that enabled by the above
+process.
+
+# One Definition Rule (ODR)


If the major version is always part of the package name, doesn't that prevent ODR violations? I don't quite understand the problem here -- can you give a concrete example to illustrate?

envoy.foo.v4 refers to envoy.bar.v4 and envoy.baz.v3. envoy.bar.v4 refers to envoy.blah.v2 while envoy.baz.v3 refers to envoy.blah.v3. Voila, transitively envoy.foo.v4 references envoy.blah.v2 and envoy.blah.v3.

Sure, but since envoy.blah.v2 and envoy.blah.v3 are different namespaces, how does that cause an ODR violation?

It doesn't at a C++ level, but it does at a higher level, since by allowing these multiple definitions, envoy.blah now needs to maintain an arbitrary number of stable versions, rather than just the trailing two versions. That includes all the code backing these versions. By forcing an "ODR-style" rule here, we avoid this quagmire.

mattklein123

Thanks this is great. Flushing out some minor comments.

/wait

CONTRIBUTING.md

api/API_VERSIONING.md

docs/root/configuration/overview/v2_overview.rst

Signed-off-by: Harvey Tuch <[email protected]>

htuch · 2019-09-27T14:45:07Z

api/API_VERSIONING.md

+minimize API churn by deferring application of discretionary changes until a major version cycle
+where the respective message is undergoing a mandatory change.
+
+The Envoy API structure helps with minimizing churn between versions. Developers should architect


@alyssawilk I think this covers what we discussed the other day, an advisory statement on organizing to minimize churn.

Signed-off-by: Harvey Tuch <[email protected]>

mattklein123

LGTM, thanks a ton for putting this together. Will defer to others for further review.

alyssawilk

Very cool. Couple of thoughts below

CONTRIBUTING.md

alyssawilk · 2019-09-30T16:31:02Z

api/API_VERSIONING.md

+In everyday discussion and GitHub labels, we refer to the `v2`, `v3`, `vN`, `...` APIs. This has a
+specific technical meaning. Any given message in the Envoy API, e.g. the `Bootstrap` at
+`envoy.config.bootstrap.v3.Boostrap`, will transitively reference a number of packages in the Envoy
+API. These may be at `vN`, `v(N-1)`, etc. The Envoy API is technically a DAG of versioned package


I think whole paragraph might be usefully replaced with an example set of configurations.

I think we'll have something more tangible to put here once we have the final cut of the v3alpha API and have updated the Envoy config examples, so I suggest punting until then.

Signed-off-by: Harvey Tuch <[email protected]>

htuch · 2019-10-03T15:45:24Z

I chatted with @alyssawilk and she's happy with this PR, so if I don't get any more feedback I'll merge in a couple of hours.

dfawley

Thanks for the heads-up, @htuch. Just one comment otherwise this looks great to me!

api/API_VERSIONING.md

Signed-off-by: Harvey Tuch <[email protected]>

This captures the API versioning guidelines implied by https://docs.google.com/document/d/1xeVvJ6KjFBkNjVspPbY_PwEDHC7XPi0J5p1SqUXcCl8/edit#heading=h.xgk8xel154p and splits apart the Envoy API and internal implementation breaking change policies. Some of the policy decisions (e.g. not allowing vNalpha to be hand edited, how we do manual breaking changes, etc.) have been added to these guidelines based on recent experience with protoxform and mechanical major version upgrade work, they are not part of the original stable API versioning work. Risk level: Low Testing: Formatting and docs build. Fixes envoyproxy#8371 Signed-off-by: Harvey Tuch <[email protected]>

htuch requested review from alyssawilk and mattklein123 September 26, 2019 19:14

htuch assigned mattklein123 and alyssawilk Sep 26, 2019

markdroth reviewed Sep 26, 2019

View reviewed changes

mattklein123 requested changes Sep 26, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 26, 2019

Review feedback.

2fb24c5

Signed-off-by: Harvey Tuch <[email protected]>

repokitteh-read-only bot removed the waiting label Sep 27, 2019

A word to the wise on package organization.

adc9ae5

Signed-off-by: Harvey Tuch <[email protected]>

htuch commented Sep 27, 2019

View reviewed changes

Feedback on proto status annotations.

a28e026

Signed-off-by: Harvey Tuch <[email protected]>

mattklein123 previously approved these changes Sep 27, 2019

View reviewed changes

markdroth previously approved these changes Sep 27, 2019

View reviewed changes

htuch mentioned this pull request Sep 29, 2019

protoxform tests #8428

Closed

alyssawilk reviewed Sep 30, 2019

View reviewed changes

mattklein123 added the waiting label Sep 30, 2019

htuch added 2 commits October 2, 2019 15:17

Review feedback.

38c8041

Signed-off-by: Harvey Tuch <[email protected]>

Merge remote-tracking branch 'upstream/master' into api-versioning-md

58553dc

htuch dismissed stale reviews from markdroth and mattklein123 via 58553dc October 2, 2019 19:19

repokitteh-read-only bot removed the waiting label Oct 2, 2019

dfawley reviewed Oct 3, 2019

View reviewed changes

api/API_VERSIONING.md Outdated Show resolved Hide resolved

Review feedback.

ea09200

Signed-off-by: Harvey Tuch <[email protected]>

dfawley approved these changes Oct 3, 2019

View reviewed changes

htuch merged commit 3801c68 into envoyproxy:master Oct 3, 2019

htuch deleted the api-versioning-md branch October 3, 2019 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: add API_VERSIONING.md. #8399

api: add API_VERSIONING.md. #8399

htuch commented Sep 26, 2019

repokitteh-read-only bot commented Sep 26, 2019

htuch commented Sep 26, 2019

markdroth commented Sep 26, 2019

markdroth left a comment

markdroth Sep 26, 2019

htuch Sep 26, 2019

markdroth Sep 26, 2019

htuch Sep 26, 2019

markdroth Sep 26, 2019

htuch Sep 26, 2019

mattklein123 left a comment

htuch Sep 27, 2019

mattklein123 left a comment

alyssawilk left a comment

alyssawilk Sep 30, 2019

htuch Oct 2, 2019

htuch commented Oct 3, 2019

dfawley left a comment

api: add API_VERSIONING.md. #8399

api: add API_VERSIONING.md. #8399

Conversation

htuch commented Sep 26, 2019

repokitteh-read-only bot commented Sep 26, 2019

htuch commented Sep 26, 2019

markdroth commented Sep 26, 2019

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Oct 3, 2019

dfawley left a comment

Choose a reason for hiding this comment