-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: universe/universe_domain calls honors HTTP_PROXY envvar where it did not before #10544
Comments
Hey thanks for the report. I think you are right this is is likely related to using our new auth library but it looks like the options are passed through from my quick testing. And I am unable to reproduce this locally when using service account credentials. Here is another example that does not relate to proxies that shows the option is being passed through: package main
import (
"context"
"fmt"
"log"
"log/slog"
"os"
trace "cloud.google.com/go/trace/apiv2"
"cloud.google.com/go/trace/apiv2/tracepb"
"google.golang.org/api/option"
"google.golang.org/grpc"
"google.golang.org/grpc/metadata"
"google.golang.org/protobuf/encoding/protojson"
"google.golang.org/protobuf/reflect/protoreflect"
)
func main() {
if err := run(); err != nil {
slog.Error("shutting down", "err", err)
os.Exit(1)
}
}
func run() error {
ctx := context.Background()
projectID := "project-id"
c, err := trace.NewClient(ctx,
option.WithGRPCDialOption(grpc.WithUnaryInterceptor(loggingUnaryInterceptor())),
)
if err != nil {
return err
}
defer c.Close()
req := &tracepb.BatchWriteSpansRequest{
Name: fmt.Sprintf("projects/%s", projectID),
Spans: []*tracepb.Span{},
}
err = c.BatchWriteSpans(ctx, req)
if err != nil {
panic(err)
}
return nil
}
func loggingUnaryInterceptor() grpc.UnaryClientInterceptor {
return func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
err := invoker(ctx, method, req, reply, cc, opts...)
log.Printf("Invoked method: %v", method)
md, ok := metadata.FromOutgoingContext(ctx)
if ok {
log.Println("Metadata:")
for k, v := range md {
log.Printf("Key: %v, Value: %v", k, v)
}
}
reqb, merr := protojson.Marshal(req.(protoreflect.ProtoMessage))
if merr == nil {
log.Printf("Request: %s", reqb)
}
return err
}
} Looking at the error it is a failed standard HTTP client call that failed. That option for gRPC does not affect the underlying HTTP client that is used to talk to the metadata service. I will have to look some more but perhaps the client being used now is honoring the environment variable where it was not before. |
I think this is the case the old code path would have used the default client in the metadata package that creates a transport: google-cloud-go/compute/metadata/metadata.go Lines 67 to 75 in 978d4a1
The new codepath creates a client for this check and does not declare a transport so it falls back to Go's default: google-cloud-go/auth/internal/internal.go Lines 184 to 185 in 978d4a1
Will have to investigate this more though. |
When running your example, I see:
Please let me know if there is any other useful information that I can provide. |
Yeah thanks. My main point there is that the options are being passed through fine, sorry for the confusion. I think we can "fix" this, restore to the old behaviour. But I am not sure the old behaviour is actually a great one of not honoring HTTP_PROXY. |
Thanks. Yes, I understood your comment. I think the ideal behavior would be a consistent way to disable the use of the proxy for the client, but I recognize that is not a simple ask. The change in behavior between the new and old version was certainly unexpected in a patch release. |
When this code was first introduced we did not have the context methods in the metadata package so we set the timeout on the client. Now that we do we should use context to set the timeout and use the default client the metadata package provides. Fixes: googleapis#10544
I did create a PR to restore the previous behavior, at least for now. This may change in a future release though as I don't think it is intentional that the metadata package is ignoring standard proxy envvars. |
🤖 I have created a release *beep* *boop* --- ## [0.7.2](https://togithub.com/googleapis/google-cloud-go/compare/auth/v0.7.1...auth/v0.7.2) (2024-07-22) ### Bug Fixes * **auth:** Use default client for universe metadata lookup ([#10551](https://togithub.com/googleapis/google-cloud-go/issues/10551)) ([d9046fd](https://togithub.com/googleapis/google-cloud-go/commit/d9046fdd1435d1ce48f374806c1def4cb5ac6cd3)), refs [#10544](https://togithub.com/googleapis/google-cloud-go/issues/10544) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- ## [0.7.2](https://togithub.com/googleapis/google-cloud-go/compare/auth/v0.7.1...auth/v0.7.2) (2024-07-22) ### Bug Fixes * **auth:** Use default client for universe metadata lookup ([googleapis#10551](https://togithub.com/googleapis/google-cloud-go/issues/10551)) ([d9046fd](https://togithub.com/googleapis/google-cloud-go/commit/d9046fdd1435d1ce48f374806c1def4cb5ac6cd3)), refs [googleapis#10544](https://togithub.com/googleapis/google-cloud-go/issues/10544) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Client
trace
Environment
GCE
Go Environment
$ go version
$ go env
Code
e.g.
Expected behavior
With
cloud.google.com/go/[email protected]
and earlier, this code completes without error when thehttp_proxy
environment variable is set. It does not try to connect to the proxy asgrpc.WithNoProxy()
is set.Actual behavior
As of 1.10.8, this code now tries to connect to a proxy set via
http_proxy
. For instance, if I set it to an invalid value, e.g.,255.255.255.255
, I get an error like:Additional context
As mentioned, this started after upgrading to 1.10.8 or newer. It appears to be related to the new auth.
In our use case, we have
http_proxy
andhttps_proxy
set. All external requests go through our proxy server. For requests that we consider "internal", such as those to the GCP metadata server, we explicitly disable the proxy for clients making these requests. Although settingno_proxy=169.254.169.254
would be a workaround, we would prefer to disable the proxy on a case-by-case basis.The text was updated successfully, but these errors were encountered: