docs: mark matching API and related features as alpha by snowp · Pull Request #16210 · envoyproxy/envoy

snowp · 2021-04-28T14:44:00Z

Commit Message: Marks the matching API more clearly as experimental and updates the composite filter security posture to reflect this.
Additional Description:
Risk Level: Low, docs only
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Snow Pettersen <snowp@lyft.com>

repokitteh-read-only · 2021-04-28T14:44:08Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16210 was opened by snowp.

see: more, trace.

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch · 2021-04-29T00:49:58Z

api/envoy/config/common/matcher/v3/matcher.proto

 // might repeat several times until the final OnMatch (or no match) is decided.
 //
-// This API is a work in progress.
+// This API is currently considered experimental and is a work in progress.


Can you add a [#experimental: tag and render this as a warning box? @phlax could you help @snowp with this if possible? It should be doable in https://github.com/envoyproxy/envoy/blob/main/tools/protodoc/protodoc.py.

Also, can https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/security/threat_model#threat-model be updated to explicitly state anything flagged as experimental is out-of-scope?

Is https://github.com/envoyproxy/envoy/pull/16210/files#diff-7e7afbba95a0599ef388b3eb10e44c6cf3520dd65c24ef0f8d77e790d683a537L80-R83 not sufficient for updating the threat model? Anywhere else I should point out that experimental features are not covered?

the cache filter probs wants to be marked as experimental too i think

apologies - i misread comment - i can help adding the annotation + parser if its helpful to

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch · 2021-05-02T20:33:13Z

docs/root/intro/arch_overview/security/threat_model.rst

-it should be hardened with this model in mind. Security issues related to core code will usually
-trigger the security release process as described in this document.
+Anything in the Envoy core may be used in both untrusted and trusted deployments, with the exception
+of features explicitly marked as experimental. As a consequence, it should be hardened with this model


"marked as experimental; experimental features are only supported in trusted deployments and do not qualify for treatment under the threat model below" or something like that.

htuch · 2021-05-02T20:33:50Z

tools/protodoc/protodoc.py

+    experimental_warning = ''
+    if annotations.EXPERIMENTAL_ANNOTATION in comment.annotations:
+        experimental_warning = (
+            '.. warning::\n   This API is experimental and is not covered by the security posture.\n\n'


"not covered by the threat model" (and link back to the RST for this)

htuch

Just a general comment, we now have multiple notions of WiP:

This experimental protodoc tag.
The not-implemented-hide protodoc tag (sort of overlaps but not exactly).
alpha status on the extension.
work_in_progress proto annotation.

Is it clear which to use and where?

snowp · 2021-05-04T18:29:37Z

I think the need for experimental comes in here for core protos that are experimental: the protos live in a file with existing stable protos (maybe it shouldn't have, that way we could have just marked this as alpha/work-in-progress). I also think the WIP tag only works for extension docs, which isn't going to be applicable to these core protos.

Perhaps we should add docs somewhere or attempt to reconcile these variations? For example, we could have a message level WIP annotation instead of this new experimental tag?

phlax · 2021-05-04T18:32:39Z

Perhaps we should add docs somewhere or attempt to reconcile these variations?

seems like a good idea - but it would be good to get something one way or another, so im +1 to adding it for now unless we can resolve upstream quickly

For example, we could have a message level WIP annotation instead of this new experimental tag?

my feeling is that experimental needs to be distinct from WIP

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch · 2021-05-05T02:04:24Z

Maybe replace WIP annotations with experimental at file level? Docs coverage in api/STYLE.md would be fantastic.

htuch

LGTM modulo api/STYLE.md update on the range of experimental/WiP/alpha etc.

Signed-off-by: Snow Pettersen <snowp@lyft.com>

alyssawilk · 2021-05-06T12:39:38Z

api/STYLE.md

  implementation. These indicate that the entity is not implemented in Envoy and the entity
  should be hidden from the Envoy documentation.

+* Use a `[#experimental:]` annotation in comments for messages that are considered experimental


I thought that generally alpha things weren't considered to be in the threat model. Maybe we should just tag this alpha instead?

@yanavlasov for thoughts

This gets to my comment above #16210 (review). I think the issue is we can only tag as alpha at file level. @snowp points out that sometimes we want to tag at feature level.

Right, but can we call them both alpha, rather than alpha and experimental? Unless there are actual differences, but I think the description above could apply to either so I'd lean towards calling it alpha and shipping :-)

Yes, I'm good with more consistent terminology here, that helps reduce some of the confusion.

Signed-off-by: Snow Pettersen <snowp@lyft.com>

alyssawilk

nice! Just a few more comments for all the shepherds' reviews matter more here =P

alyssawilk · 2021-05-10T16:58:06Z

api/STYLE.md

  should be hidden from the Envoy documentation.

+* Use a `[#alpha:]` annotation in comments for messages that are considered alpha
+  and are not subject to the threat model. This differs from the work-in-progress/alpha tagging


I think we need to update this comment given it's now alpha tagging :-)

I think also worth calling out that things tagged alpha are not yet subject to the stable API policies and security release policies.

Hmm we still have extension level alpha tagging which differs from this alpha tag which is the only way to mark messages within core as alpha. What kind of change were you thinking about?

I was trying to resolve the two, and have alpha tagging for extensions be functionally the same as alpha tagging for messages and fields, which is basically that both are WIP, not stable APIs and less trusted code.

alyssawilk · 2021-05-10T16:58:38Z

docs/root/configuration/http/http_filters/composite_filter.rst


+.. attention::
+
+   The composite filter is experimental and is currently under active development.


optional: experimental -> alpha here and below?

alyssawilk

oh and PR name/description experimental -> alpha?

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch · 2021-05-12T02:24:06Z

docs/root/intro/arch_overview/advanced/matching/matching_api.rst


+.. attention::
+
+   The matching API is experimental and is currently under active development.


Nit: s/experimental/alpha/

htuch · 2021-05-12T02:27:46Z

tools/protodoc/protodoc.py

+    alpha_warning = ''
+    if annotations.ALPHA_ANNOTATION in comment.annotations:
+        experimental_warning = (
+            '.. warning::\n   This API is alpha and is not covered by the :ref:`threat model <arch_overview_threat_model>`.\n\n'


FWIW, we have a broader issue which we can resolve after we land this PR. We have both implementation and API alpha/experimental/unknown robustness status. An API is not really insecure in and of itself, this is an implementation attribute.

We really need a way to mark in the protos client specific implementation status details, since Envoy vs. gRPC support for a feature might have different implementation status.

I started to explore this in #11085, but it's not really something I have b/w for. Maybe @phlax this can be part of the docs/tooling roadmap?

CC @markdroth

I agree, it would be nice to have some generic solution for this kind of thing. Matt has been saying for a while that we need a way to have client-specific docs in the proto files, not just for API status but for any type of documentation. Although as the number of xDS clients grows, I'm not sure how feasible it will be to document all of the in the same proto files. But this is definitely something we need to come up with a good strategy for.

htuch

LGTM modulo nit. Will leave final sign-off/merge to @alyssawilk

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch · 2021-05-21T03:30:28Z

/lgtm api

Marks the matching API more clearly as experimental and updates the composite filter security posture to reflect this. Signed-off-by: Snow Pettersen <snowp@lyft.com>

Snow Pettersen added 3 commits April 28, 2021 14:37

docs: mark matching API and associated protos as experimental

7677de0

Signed-off-by: Snow Pettersen <snowp@lyft.com>

proto_format

7f549fb

Signed-off-by: Snow Pettersen <snowp@lyft.com>

update posture of composite to reflect experimental nature

4a8325a

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp assigned htuch Apr 28, 2021

snowp requested a review from mattklein123 as a code owner April 28, 2021 14:44

repokitteh-read-only bot added the api label Apr 28, 2021

explitly exclude experimental core features from sec posture

7c7b8d3

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch reviewed Apr 29, 2021

View reviewed changes

Snow Pettersen added 3 commits April 30, 2021 14:38

add experimental annotation

19d292b

Signed-off-by: Snow Pettersen <snowp@lyft.com>

proto format

1dc8ca4

Signed-off-by: Snow Pettersen <snowp@lyft.com>

format

57aa455

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch reviewed May 2, 2021

View reviewed changes

Snow Pettersen added 2 commits May 4, 2021 20:47

feedback

c5b2cd3

Signed-off-by: Snow Pettersen <snowp@lyft.com>

spelling

a4712bb

Signed-off-by: Snow Pettersen <snowp@lyft.com>

htuch reviewed May 5, 2021

View reviewed changes

add section about experimental to api/STYLE

34290c9

Signed-off-by: Snow Pettersen <snowp@lyft.com>

alyssawilk reviewed May 6, 2021

View reviewed changes

rename experimental -> alpha

4931d16

Signed-off-by: Snow Pettersen <snowp@lyft.com>

alyssawilk self-assigned this May 10, 2021

alyssawilk reviewed May 10, 2021

View reviewed changes

experimental -> alpha

3684c37

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp changed the title ~~docs: mark matching API and related features as experimental~~ docs: mark matching API and related features as alpha May 11, 2021

htuch reviewed May 12, 2021

View reviewed changes

aplha, update style

7291160

Signed-off-by: Snow Pettersen <snowp@lyft.com>

alyssawilk approved these changes May 20, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 21, 2021

htuch merged commit b603923 into envoyproxy:main May 21, 2021


		.. attention::

		The composite filter is experimental and is currently under active development.


		.. attention::

		The matching API is experimental and is currently under active development.

Conversation

snowp commented Apr 28, 2021

Uh oh!

repokitteh-read-only bot commented Apr 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

snowp commented May 4, 2021

Uh oh!

phlax commented May 4, 2021

Uh oh!

htuch commented May 5, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented May 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants