Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: v2 transport API fatal-by-default. #14223

Merged
merged 15 commits into from
Dec 7, 2020

Conversation

htuch
Copy link
Member

@htuch htuch commented Dec 1, 2020

This is a followup to #13950 in which the transport API is also
fatal-by-default.

Risk level: High (this will break anyone who is still using v2 and has
not enabled CLI or runtime override)
Testing: Various tests updated as described above. New unit test added
for bootstrap to server_test and to ads_integration_test for
dynamic rejection behavior. api_version_integration_test continues to
provide the definitive cross-version transport API integration test.
Release Notes: Same as #13950.

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

Verified

This commit was signed with the committer’s verified signature.
This is a followup to envoyproxy#13950 in which the transport API is also
fatal-by-default.

Risk level: High (this will break anyone who is still using v2 and has
  not enabled CLI or runtime override)
Testing: Various tests updated as described above. New unit test added
 for bootstrap to server_test and to ads_integration_test for
 dynamic rejection behavior. api_version_integration_test continues to
 provide the definitive cross-version transport API integration test.
Release Notes: Same as envoyproxy#13950.

Signed-off-by: Harvey Tuch <htuch@google.com>
@lizan
Copy link
Member

lizan commented Dec 1, 2020

Also I don't think this covers RLS, ALS, Metrics Service etc? Will that in a follow up?

@htuch
Copy link
Member Author

htuch commented Dec 1, 2020

@lizan yep, RLS etc. is the next patch.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Small error/release note request.

/wait

source/common/config/subscription_factory_impl.cc Outdated Show resolved Hide resolved
htuch added 5 commits December 2, 2020 10:14

Verified

This commit was signed with the committer’s verified signature.
…l-by-default

Verified

This commit was signed with the committer’s verified signature.
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14223 was synchronize by htuch.

see: more, trace.

@htuch
Copy link
Member Author

htuch commented Dec 2, 2020

This isn't ready for review yet; I need to deprecate V2/AUTO in the v3 ConfigSource.
/wait

Signed-off-by: Harvey Tuch <htuch@google.com>
@@ -33,7 +33,7 @@ services:
- envoymesh

ext_authz-opa-service:
image: openpolicyagent/opa:0.21.0-istio
image: openpolicyagent/opa:0.24.0-istio
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about this change; I couldn't get the OPA test to pass locally, is the OPA Envoy support v2 only? CC @lizan

htuch added 2 commits December 3, 2020 11:52
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for slogging through this. Very much looking forward to all of the v2 code being deleted and putting this behind us.

My only high level question is are we losing a bunch of coverage as part of this PR?

/wait-any

@htuch
Copy link
Member Author

htuch commented Dec 3, 2020

@mattklein123 I don't think it technically reduces coverage, since previously we were verifying the v2 path and ignoring v3, now we verify v3 and not v2, in places where there is only a single test. The v2 -> v3 conversion is mechanical and verified in other places. It doesn't seem a good use of time to modify all tests to cover v2 as well given the fact that it's on the way out. We still have v2/v3 transport tests (api_version_integration_test) and for things like RLS/ext_authz, they seem to have tests for the different transport versions, so it's mostly in unit tests I think and places where the v2/v3 choice is arbitrary.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch htuch added the waiting label Dec 4, 2020
@htuch
Copy link
Member Author

htuch commented Dec 6, 2020

All tests passing, ready for another review pass, thanks.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for slogging through this. LGTM save for one question.

/wait-any

examples/ext_authz/run_envoy.sh Outdated Show resolved Hide resolved
@repokitteh-read-only repokitteh-read-only bot removed the api label Dec 7, 2020
@htuch htuch merged commit 9093131 into envoyproxy:master Dec 7, 2020
@htuch htuch deleted the v2-transport-fatal-by-default branch December 7, 2020 23:19
mpuncel added a commit to mpuncel/envoy that referenced this pull request Dec 8, 2020
* master: (41 commits)
  event: Remove a source of non-determinism by always running deferred deletion before post callbacks (envoyproxy#14293)
  Fix TSAN bug in integration test (envoyproxy#14327)
  tracing: Add hostname to Zipkin config.  (envoyproxy#14186) (envoyproxy#14187)
  [conn_pool] fix use after free in H/1 connection pool (envoyproxy#14220)
  lua: update deprecated lua_open to luaL_newstate (envoyproxy#14297)
  extension: use bool_flag to control extension link (envoyproxy#14240)
  stats: Factor out creation of cluster-stats StatNames from creation of the stats, to save CPU during xDS updates (envoyproxy#14028)
  test: add scaled timer integration test (envoyproxy#14290)
  [Win32 Signals] Add term and ctrl-c signal handlers (envoyproxy#13954)
  config: v2 transport API fatal-by-default. (envoyproxy#14223)
  matcher: fix UB bug caused by dereferencing a bad optional (envoyproxy#14271)
  test: putting fake upstream config in a struct (envoyproxy#14266)
  wasm: use Bazel rules from Proxy-Wasm Rust SDK. (envoyproxy#14292)
  docs: fix typo (envoyproxy#14237)
  dependencies: allowlist CVE-2018-21270 to prevent false positives. (envoyproxy#14294)
  typo in redis doc (envoyproxy#14248)
  access_loggers: removed redundant dep (envoyproxy#14274)
  fix http2 flaky test (envoyproxy#14261)
  test: disable flaky xds_integration_test. (envoyproxy#14287)
  http: add functionality to configure kill header in KillRequest proto (envoyproxy#14288)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
htuch added a commit to htuch/envoy that referenced this pull request Dec 14, 2020
This is a followup to envoyproxy#14223, covering remaining uses of the
transport_api_version field.

Risk level: High (this will break anyone who is still using v2 and has
   not enabled CLI or runtime override)
Testing: Various tests updated, some exemplar tests added to
  server_test.
Release Notes: Same as envoyproxy#13950.

Signed-off-by: Harvey Tuch <htuch@google.com>
htuch added a commit that referenced this pull request Dec 15, 2020
This is a followup to #14223, covering remaining uses of the
transport_api_version field.

Risk level: High (this will break anyone who is still using v2 and has
not enabled CLI or runtime override)
Testing: Various tests updated, some exemplar tests added to
server_test.
Release Notes: Same as #13950.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants