googleapis / google-cloud-go

Google Cloud Client Libraries for Go.
https://cloud.google.com/go/docs/reference
Apache License 2.0
3.78k stars 1.3k forks source link

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

Open frankyn opened 1 month ago

frankyn commented 1 month ago

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 (https://github.com/googleapis/google-api-go-client/pull/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

quartzmo commented 1 month ago

cc @xmenxk

xmenxk commented 1 month ago

@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 commented 1 month ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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

xmenxk commented 3 weeks ago

sure, I can take this

frankyn commented 1 week ago

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.