Skip to content

Conversation

@sergiitk
Copy link
Member

@sergiitk sergiitk commented Apr 22, 2022

Proto updates

  1. cncf/xds: Sort xds/import.sh protos alphabetically
  2. cncf/xds: Sync protos to cncf/xds@d92e9ce (commit 2021-12-16, corresponding to envoy cl/440193522). It's a no-op for used protos, but helpful to import the latest matcher.proto
  3. cncf/xds: Import xds/type/matcher/v3/matcher.proto with dependencies
  4. envoyproxy/protoc-gen-validate: Sync protos to bufbuild/protoc-gen-validate@dfcdc5e (commit 2022-03-10, corresponding to envoy cl/440193522) to pick up ignore_empty field required for the following envoy sync
  5. envoyproxy/envoy Sync protos to envoyproxy/envoy@e33f444 (commit 2022-04-07, cl/440193522). This is the minimal version needed to pick up ClusterSpecifierPlugin.is_optional.
    a. Generated code: AggregatedDiscoveryServiceGrpc was regenerated from the updated proto. This is a no-op, just a minor change to the docblocks.
    b. Deprecated fields had to be taken care of manually, see "Manual updates to the code" below.
  6. envoyproxy/envoy Sync protos to the latest imported version envoyproxy/envoy@5d74719 (commit 2022-04-08, cl/443359189). Not needed for anything specific, just the last version, and was easy to import.

Manual updates to the code

As the result of envoyproxy/envoy@e33f444 sync:

  1. Deprecated ConfigSource.path replaced with the ConfigSource.path_config_source in test fake resources. The ConfigSource.path isn't in active code paths, so no prod code changes needed.
  2. Suppress CertificateValidationContext.match_subject_alt_names deprecations in test files. Surprisingly, we don't report deprecations in prod files, despite the fact this field is used in prod code a few times.

@sergiitk sergiitk force-pushed the envoy-proto-sync-2022-04-21 branch from ddfb464 to b18caf3 Compare April 22, 2022 17:48
@sergiitk sergiitk marked this pull request as ready for review April 22, 2022 19:19
@sergiitk sergiitk requested a review from sanjaypujare April 22, 2022 19:20
@sergiitk
Copy link
Member Author

@sanjaypujare FYI field CertificateValidationContext.match_subject_alt_names got deprecated. Seems to be relevant to some of the work you did: https://github.com/grpc/grpc-java/blame/8e65700edc62168fdc85b5b20a298f0280aa23c1/xds/src/main/java/io/grpc/xds/ClientXdsClient.java#L602

@sanjaypujare
Copy link
Contributor

@sanjaypujare FYI field CertificateValidationContext.match_subject_alt_names got deprecated. Seems to be relevant to some of the work you did: https://github.com/grpc/grpc-java/blame/8e65700edc62168fdc85b5b20a298f0280aa23c1/xds/src/main/java/io/grpc/xds/ClientXdsClient.java#L602

Correct but unless Traffic Director stops using the deprecated fields we cannot make the change unilaterally on our side

@sergiitk
Copy link
Member Author

@sanjaypujare Yes, just a heads up. What we could do is to add support for the new field, while keeping the deprecated field. Up to you though, I just wanted to inform you of the deprecation.

@sanjaypujare
Copy link
Contributor

... add support for the new field, while keeping the deprecated field. ...

Already done! Although not tested with a real xDS control plane since we don't have any to test with

@sergiitk
Copy link
Member Author

Wait, with what field did it get replaced? I tried to update tests to replace it with match_typed_subject_alt_names in unit tests, but tests don't pass with it.

@sergiitk sergiitk changed the title xds: Envoy proto sync 2022-04-21 xds: Envoy proto sync 2022-04-08 Apr 22, 2022
@sergiitk sergiitk changed the title xds: Envoy proto sync 2022-04-08 xds: Envoy proto sync to 2022-04-08 Apr 22, 2022
@sanjaypujare
Copy link
Contributor

Wait, with what field did it get replaced? I tried to update tests to replace it with match_typed_subject_alt_names in unit tests, but tests don't pass with it.

I may have misunderstood or misspoke. May be the subject_alt_name needs to be implemented because it's a recent addition. Good point. BTW all other languages also need the change

@sergiitk
Copy link
Member Author

Ah, glad we figured it out. Let me know if there's anything I can do to help with the review. Going through each commit is probably the easiest.

@sanjaypujare
Copy link
Contributor

I tried to update tests to replace it with match_typed_subject_alt_names in unit tests,

BTW, I don't think you should remove or update any tests using deprecated fields just yet. We depend on these tests to ensure interoperability with other components (e.g. Traffic Director) until we verify that the deprecated fields have been completely replaced by these other components.

@sergiitk
Copy link
Member Author

I didn't remove them, just added @SuppressWarnings("deprecation") to the tests. See b18caf3.

Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

LGTM

@sergiitk sergiitk merged commit b1720f1 into grpc:master Apr 25, 2022
@sergiitk sergiitk deleted the envoy-proto-sync-2022-04-21 branch April 25, 2022 23:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants