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

examples/features/csm_observability: Add xDS Credentials #7875

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Nov 26, 2024

This PR adds xDS Credentials to the CSM Observability example. It also switches the server to be xDS Enabled.

RELEASE NOTES:

  • examples: Add xDS Credentials to csm_observability and switch server to be xDS enabled

@zasweq zasweq requested a review from dfawley November 26, 2024 03:12
@zasweq zasweq requested a review from yashykt November 26, 2024 03:12
@zasweq zasweq added the Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability label Nov 26, 2024
@zasweq zasweq added this to the 1.69 Release milestone Nov 26, 2024
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.97%. Comparing base (dcba136) to head (adab125).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7875      +/-   ##
==========================================
+ Coverage   81.91%   81.97%   +0.05%     
==========================================
  Files         377      377              
  Lines       38065    38120      +55     
==========================================
+ Hits        31180    31247      +67     
+ Misses       5581     5566      -15     
- Partials     1304     1307       +3     

see 32 files with indirect coverage changes

@yashykt
Copy link
Member

yashykt commented Nov 26, 2024

please also backport this to 1.68 and push a new image on dockerhub

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Please add something to the release notes for this.

@@ -56,7 +57,11 @@ func main() {
cleanup := csm.EnableObservability(context.Background(), opentelemetry.Options{MetricsOptions: opentelemetry.MetricsOptions{MeterProvider: provider}})
defer cleanup()

cc, err := grpc.NewClient(*target, grpc.WithTransportCredentials(insecure.NewCredentials()))
creds, err := xdscreds.NewClientCredentials(xdscreds.ClientOptions{FallbackCreds: insecure.NewCredentials()})
Copy link
Member

Choose a reason for hiding this comment

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

insecure? This doesn't seem proper to include in an example. Maybe we fall back to TLS? Or some kind of "erroring creds"? What do the other languages do for this?

Copy link
Member

Choose a reason for hiding this comment

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

We probably should at least add comments indicating that the fallback credentials are here for demonstration purposes only and should not be used this way in production as it is insecure. It would be nice if we have a guide to link to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments for client and server side.

Copy link
Member

Choose a reason for hiding this comment

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

Actually this example is for CSM, and the CSM guide recommends setting fallback to insecure, so better to not have this text... But maybe something like:

// Set up xds credentials that fall back to insecure as described in:
// https://cloud.google.com/service-mesh/docs/service-routing/security-proxyless-setup#workloads_are_unable_to_communicate_in_the_security_setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -67,7 +70,14 @@ func main() {
if err != nil {
log.Fatalf("Failed to listen: %v", err)
}
s := grpc.NewServer()
creds, err := xdscreds.NewServerCredentials(xdscreds.ServerOptions{FallbackCreds: insecure.NewCredentials()})
Copy link
Member

Choose a reason for hiding this comment

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

As above. If xdscreds is unavailable, using insecure seems....insecure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline; this is part of the CSM requirements.

@zasweq
Copy link
Contributor Author

zasweq commented Nov 26, 2024

Added release notes

@zasweq zasweq assigned dfawley and unassigned dfawley Nov 26, 2024
@dfawley dfawley assigned zasweq and unassigned dfawley Nov 26, 2024
@zasweq zasweq assigned dfawley and unassigned zasweq Nov 26, 2024
@dfawley dfawley assigned zasweq and unassigned yashykt and dfawley Dec 2, 2024
@zasweq zasweq added the Type: Behavior Change Behavior changes not categorized as bugs label Dec 2, 2024
@zasweq zasweq merged commit ab189b0 into grpc:master Dec 2, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants