This is an automated email from the ASF dual-hosted git repository.
AlexStocks pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/dubbo-go-pixiu.git
The following commit(s) were added to refs/heads/develop by this push:
new 22afda40d fix(grpcproxy): load local proto files under the default
AUTO strategy (#1032)
22afda40d is described below
commit 22afda40d593c35ed355829676249eafd4fbd829
Author: Yui(ゆい) <[email protected]>
AuthorDate: Sun Sep 6 18:01:31 2026 +0900
fix(grpcproxy): load local proto files under the default AUTO strategy
(#1032)
`initDescriptorSource` only called `initFileDescriptorSource` for the LOCAL
strategy, so under AUTO — the default one (`default:"auto"` on
`descriptor_source_strategy`) — the local proto files were never loaded.
`getDescriptorCompose` then assigned that nil `*fileSource` to the
`DescriptorSource` interface field, which yields a non-nil interface
holding a
nil pointer. The documented `file + reflection` fallback therefore
dereferenced
nil instead of returning an error, and `compositeSource.FindSymbol`
panicked on
every request whose server-reflection lookup failed.
Load the local proto files for AUTO as well as LOCAL, keep a nil file
source out
of the interface, and guard the `compositeSource` fallbacks so a missing
file
source surfaces an error. `getFileDescriptorCompose` now reports an unusable
`path` instead of handing back a nil source, and `AllExtensionsForType`
returns
the file-source result rather than computing and discarding it when there
is no
reflection source.
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
pkg/filter/http/grpcproxy/descriptor.go | 18 +++-
pkg/filter/http/grpcproxy/descriptor_source.go | 51 ++++++----
pkg/filter/http/grpcproxy/descriptor_test.go | 129 +++++++++++++++++++++++++
3 files changed, 174 insertions(+), 24 deletions(-)
diff --git a/pkg/filter/http/grpcproxy/descriptor.go
b/pkg/filter/http/grpcproxy/descriptor.go
index 495b371d6..2e0abcdff 100644
--- a/pkg/filter/http/grpcproxy/descriptor.go
+++ b/pkg/filter/http/grpcproxy/descriptor.go
@@ -111,14 +111,22 @@ func (dr *Descriptor) getDescriptorCompose(ctx
context.Context, cfg *Config) (De
cs := &compositeSource{}
cs.reflection, err = dr.getServerDescriptorSourceCtx(ctx, cfg)
- cs.file = dr.getFileSource()
+ // Never store a nil *fileSource: it would leave cs.file a non-nil
interface
+ // holding a nil pointer, so the fallback in compositeSource would
dereference
+ // it instead of reporting that the local proto files are unavailable.
+ if fs := dr.getFileSource(); fs != nil {
+ cs.file = fs
+ }
return cs, err
}
func (dr *Descriptor) initDescriptorSource(cfg *Config) *Descriptor {
- if cfg.DescriptorSourceStrategy.String() == LOCAL {
+ switch strings.ToLower(cfg.DescriptorSourceStrategy.String()) {
+ case LOCAL, AUTO:
+ // AUTO is `file + reflection`, so the local proto files have
to be loaded
+ // as well, otherwise the fallback of the reflection lookup has
no source.
dr.initFileDescriptorSource(cfg)
}
@@ -248,7 +256,11 @@ func (dr *Descriptor) getMethodDescriptor(source
DescriptorSource, cc *grpc.Clie
func (dr *Descriptor) getFileDescriptorCompose(ctx context.Context, cfg
*Config) (DescriptorSource, error) {
dr.initFileDescriptorSource(cfg)
- return dr.getFileSource(), nil
+ fs := dr.getFileSource()
+ if fs == nil {
+ return nil, errors.New("the local proto file descriptor source
is not available")
+ }
+ return fs, nil
}
func (dr *Descriptor) initFileDescriptorSource(cfg *Config) *Descriptor {
diff --git a/pkg/filter/http/grpcproxy/descriptor_source.go
b/pkg/filter/http/grpcproxy/descriptor_source.go
index 06e6bf9df..7cf05711f 100644
--- a/pkg/filter/http/grpcproxy/descriptor_source.go
+++ b/pkg/filter/http/grpcproxy/descriptor_source.go
@@ -160,36 +160,45 @@ func (cs *compositeSource) FindSymbol(fullyQualifiedName
string) (desc.Descripto
}
}
+ if cs.file == nil {
+ return nil, fmt.Errorf("could not found symbol %v",
fullyQualifiedName)
+ }
return cs.file.FindSymbol(fullyQualifiedName)
}
func (cs *compositeSource) AllExtensionsForType(typeName string)
([]*desc.FieldDescriptor, error) {
if cs.reflection == nil {
- fileExts, err := cs.file.AllExtensionsForType(typeName)
- if err != nil {
- return fileExts, nil
- }
- } else {
- exts, err := cs.reflection.AllExtensionsForType(typeName)
- if err != nil {
- return cs.file.AllExtensionsForType(typeName)
- }
- tags := make(map[int32]bool)
- for _, ext := range exts {
- tags[ext.GetNumber()] = true
+ if cs.file == nil {
+ return nil, nil
}
+ return cs.file.AllExtensionsForType(typeName)
+ }
- fileExts, err := cs.file.AllExtensionsForType(typeName)
- if err != nil {
- return exts, nil
- }
- for _, ext := range fileExts {
- if !tags[ext.GetNumber()] {
- exts = append(exts, ext)
- }
+ exts, err := cs.reflection.AllExtensionsForType(typeName)
+ if err != nil {
+ if cs.file == nil {
+ return nil, err
}
+ return cs.file.AllExtensionsForType(typeName)
+ }
+ if cs.file == nil {
return exts, nil
}
- return nil, nil
+
+ tags := make(map[int32]bool)
+ for _, ext := range exts {
+ tags[ext.GetNumber()] = true
+ }
+
+ fileExts, err := cs.file.AllExtensionsForType(typeName)
+ if err != nil {
+ return exts, nil
+ }
+ for _, ext := range fileExts {
+ if !tags[ext.GetNumber()] {
+ exts = append(exts, ext)
+ }
+ }
+ return exts, nil
}
diff --git a/pkg/filter/http/grpcproxy/descriptor_test.go
b/pkg/filter/http/grpcproxy/descriptor_test.go
new file mode 100644
index 000000000..5e5876b3b
--- /dev/null
+++ b/pkg/filter/http/grpcproxy/descriptor_test.go
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package grpcproxy
+
+import (
+ "context"
+ "os"
+ "path/filepath"
+ "testing"
+ "time"
+)
+
+import (
+ "github.com/jhump/protoreflect/desc" //nolint:staticcheck //
legacy descriptor API used by grpcproxy.
+ "github.com/jhump/protoreflect/desc/protoparse" //nolint:staticcheck //
legacy parser used by grpcproxy.
+
+ "github.com/stretchr/testify/require"
+
+ "google.golang.org/grpc"
+ "google.golang.org/grpc/credentials/insecure"
+)
+
+import (
+ ct "github.com/apache/dubbo-go-pixiu/pkg/context"
+)
+
+const testGreeterProto = `syntax = "proto3";
+package test;
+
+service Greeter {
+ rpc Hello(Request) returns (Response);
+}
+
+message Request {}
+message Response {}
+`
+
+// writeTestProtoDir writes a proto file to a temporary directory usable as
Config.Path.
+func writeTestProtoDir(t *testing.T) string {
+ t.Helper()
+
+ dir := t.TempDir()
+ require.NoError(t, os.WriteFile(filepath.Join(dir, "test.proto"),
[]byte(testGreeterProto), 0o600))
+ return dir
+}
+
+// TestDescriptorAutoStrategyFallsBackToLocalProtoFiles covers the AUTO
strategy, which is
+// the default one, against a backend that does not implement the server
reflection API.
+// The documented `file + reflection` fallback has to resolve the method from
the local
+// proto files instead of dereferencing an uninitialized file source.
+func TestDescriptorAutoStrategyFallsBackToLocalProtoFiles(t *testing.T) {
+ cfg := &Config{DescriptorSourceStrategy: AUTO, Path:
writeTestProtoDir(t)}
+ descriptor := (&Descriptor{}).initDescriptorSource(cfg)
+ require.NotNil(t, descriptor.getFileSource())
+
+ conn, err := grpc.NewClient(startTestGRPCServer(t),
grpc.WithTransportCredentials(insecure.NewCredentials()))
+ require.NoError(t, err)
+ t.Cleanup(func() { require.NoError(t, conn.Close()) })
+
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ source, err := descriptor.getDescriptorSource(context.WithValue(ctx,
ct.ContextKey(GrpcClientConnKey), conn), cfg)
+ require.NoError(t, err)
+
+ method, err := descriptor.getMethodDescriptor(source, conn,
"test.Greeter", "Hello")
+ require.NoError(t, err)
+ require.Equal(t, "Hello", method.GetName())
+}
+
+// TestDescriptorLocalStrategyReportsUnloadableProtoFiles keeps a
misconfigured `path` from
+// producing a descriptor source that only fails once it is dereferenced.
+func TestDescriptorLocalStrategyReportsUnloadableProtoFiles(t *testing.T) {
+ cfg := &Config{DescriptorSourceStrategy: LOCAL, Path:
filepath.Join(t.TempDir(), "missing")}
+ descriptor := (&Descriptor{}).initDescriptorSource(cfg)
+
+ source, err := descriptor.getDescriptorSource(context.Background(), cfg)
+ require.Error(t, err)
+ require.Nil(t, source)
+}
+
+// TestCompositeSourceFindSymbolWithoutFileSource keeps the fallback safe when
the local
+// proto files are unavailable altogether.
+func TestCompositeSourceFindSymbolWithoutFileSource(t *testing.T) {
+ cs := &compositeSource{}
+ _, err := cs.FindSymbol("test.Greeter")
+ require.Error(t, err)
+}
+
+// TestCompositeSourceAllExtensionsForTypeWithoutReflection asserts the
extensions found in
+// the local proto files are returned instead of being computed and then
discarded.
+func TestCompositeSourceAllExtensionsForTypeWithoutReflection(t *testing.T) {
+ files, err := (protoparse.Parser{
+ Accessor: protoparse.FileContentsFromMap(map[string]string{
+ "extension.proto": `syntax = "proto2";
+package test;
+
+message Request {
+ extensions 100 to 200;
+}
+
+extend Request {
+ optional string note = 100;
+}
+`,
+ }),
+ }).ParseFiles("extension.proto")
+ require.NoError(t, err)
+
+ cs := &compositeSource{file: &fileSource{files:
map[string]*desc.FileDescriptor{"extension.proto": files[0]}}}
+ exts, err := cs.AllExtensionsForType("test.Request")
+ require.NoError(t, err)
+ require.Len(t, exts, 1)
+ require.Equal(t, "test.note", exts[0].GetFullyQualifiedName())
+}
