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

auth: directpath no longer emits warning if misconfigured using new auth support #11001

Open
frankyn opened this issue Oct 16, 2024 · 9 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@frankyn
Copy link
Member

frankyn commented Oct 16, 2024

Client

Storage

Code and Dependencies

package main

import (
	"context"
	"log"

	"cloud.google.com/go/storage"
)

func main() {
	ctx := context.Background()

	// Creates a gRPC enabled client
        // If used outside of GCE a warning used to be emitted.
	client, err := storage.NewGRPCClient(ctx)
	if err != nil {
		log.Fatalf("Failed to create client: %v", err)
	}
	defer client.Close()
}

Expected behavior

Warnings were introduced into google-api-go-client (googleapis/google-api-go-client#2225) to tell users that DirectPath is misconfigured client side; such as running gRPC API client outside of GCE.

Actual behavior

New google-cloud-go auth support reimplemented this logic but didn't bring along the warning.
https://github.com/googleapis/google-cloud-go/blob/main/auth/grpctransport/directpath.go#L96

@frankyn frankyn added the triage me I really want to be triaged. label Oct 16, 2024
@quartzmo quartzmo added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. and removed triage me I really want to be triaged. labels Oct 17, 2024
@quartzmo
Copy link
Member

cc @xmenxk

@xmenxk
Copy link
Contributor

xmenxk commented Oct 17, 2024

@frankyn do we expect only customers to set the EnableDirectPathXds option, if so then it make sense.

But I've seen Google's per-product client libraries e.g. spanner, storage setting the EnableDirectPathXds=true by default, in that case, I suspect this would be too noisy and maybe confusing.
If I'm a customer just running Storage client code on-prem, I'd be getting "WARNING: DirectPath is misconfigured. DirectPath is only available in a GCE environment."
even though I never asked to use directpath.

@frankyn
Copy link
Member Author

frankyn commented Oct 17, 2024

That's a good point; it's not longer required in Go Storage; we've enabled it by default. The reason we did it at first is it's hard to tell if there's misconfiguration client side that can be detected and surfaced to users. If a customer uses invalid credentials or attempt using gRPC + DP outside of GCE fall back to Cloud Path will occur but doesn't notify the user this occurred.

If I'm a customer just running Storage client code on-prem, I'd be getting "WARNING: DirectPath is misconfigured. DirectPath is only available in a GCE environment."
even though I never asked to use directpath.

For GCS, gRPC is a new transport option so customers must opt-in to it but direct path is enabled by default when using gRPC.

I suspect this would be too noisy and maybe confusing.
The implementation used a rate limiter to reduce noise but agree that it's noisy.

@tritone
Copy link
Contributor

tritone commented Oct 31, 2024

We recently ran into an issue where directPath was falling back to cloudpath silently for certain auth types (see #11062). It would have been much easier to diagnose this if a warning were present. At the very least the storage client needs some way to check programmatically whether directPath is working on the transport.

@frankyn
Copy link
Member Author

frankyn commented Oct 31, 2024

@xmenxk is it possible to introduce a flag to detect this at least in Storage? We don't expect customers to use GCS gRPC API outside of GCP.

@xmenxk
Copy link
Contributor

xmenxk commented Oct 31, 2024

Yes I don't have any objection on adding back such logs. Only suggestion is for the language of the warning, instead of saying misconfigured, maybe we can just say disabled, or something along that line.

@quartzmo

@quartzmo
Copy link
Member

quartzmo commented Nov 1, 2024

@xmenxk SGTM. Do you want to create a PR for this?

@xmenxk
Copy link
Contributor

xmenxk commented Nov 1, 2024

sure, I can take this

@frankyn frankyn assigned xmenxk and unassigned quartzmo Nov 1, 2024
@frankyn
Copy link
Member Author

frankyn commented Nov 15, 2024

Short update, I lost a day debugging an issue by not having a warning when a non-default GCE service account was used.
Have this soon would be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

4 participants