deps: move from github.com/golang/protobuf to google.golang.org/protobuf/proto #7059

Clement-Jean · 2024-03-21T01:37:57Z

@dfawley @arvindbr8 this is the next step after #6919 since #6969 is closed.

I'm still unsure about the remaining deprecated dependency in channelz/service/service_test.go. First, this line:

proto.RegisterType((*OtherSecurityValue)(nil), "grpc.credentials.OtherChannelzSecurityValue")

it seems to be possible to use protoregistry.GlobalTypes.RegisterMessage instead of proto.RegisterType. I just don't know how yet.

Second, it seems that proto.UnmarshalText(want[i], w) and prototext.Unmarshall(string(want[i]), w) are not doing the same thing. The first one accepts '\x00' in the string and the second does not. Any idea why?

RELEASE NOTES: none

codecov · 2024-03-21T01:40:55Z

Codecov Report

Merging #7059 (190126d) into master (fc3f327) will decrease coverage by 1.28%.
Report is 8 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7059      +/-   ##
==========================================
- Coverage   82.47%   81.19%   -1.28%     
==========================================
  Files         305      345      +40     
  Lines       31383    33925    +2542     
==========================================
+ Hits        25884    27547    +1663     
- Misses       4442     5205     +763     
- Partials     1057     1173     +116

Files	Coverage Δ
channelz/internal/protoconv/channel.go	`86.04% <100.00%> (ø)`
channelz/internal/protoconv/server.go	`71.87% <100.00%> (ø)`
channelz/internal/protoconv/socket.go	`89.15% <100.00%> (ø)`
channelz/internal/protoconv/sockopt_linux.go	`85.88% <100.00%> (ø)`
channelz/internal/protoconv/subchannel.go	`89.28% <100.00%> (ø)`
credentials/credentials.go	`87.87% <ø> (ø)`

... and 66 files with indirect coverage changes

dfawley · 2024-03-22T16:13:52Z

it seems to be possible to use protoregistry.GlobalTypes.RegisterMessage instead of proto.RegisterType. I just don't know how yet.

This seems reasonable. But I think it's also possible, if you want, to create a proto file and import it instead of doing it dynamically like the test currently does.

Second, it seems that proto.UnmarshalText(want[i], w) and prototext.Unmarshall(string(want[i]), w) are not doing the same thing. The first one accepts '\x00' in the string and the second does not. Any idea why?

I'm not sure. Can you do prototext.Marshal() of the output of the specific proto.UnmarshalText? Maybe that will show you the format prototext.Unmarshal wants?

Clement-Jean · 2024-03-25T07:18:54Z

This seems reasonable. But I think it's also possible, if you want, to create a proto file and import it instead of doing it dynamically like the test currently does.

For now, I found a simpler solution that does pretty much the same as before:

func init() {
  // Ad-hoc registering the proto type here to facilitate Unmarshal of OtherSecurityValue.
  m := (*OtherSecurityValue)(nil)
  s := "grpc.credentials.OtherChannelzSecurityValue"
  mt := protoimpl.X.LegacyMessageTypeOf(m, protoreflect.FullName(s))
  if err := protoregistry.GlobalTypes.RegisterMessage(mt); err != nil {
    panic(err)
  }
}

We are still using legacy way of registering messages but not the deprecated dependency. The main problem I have with having a proto file or registering a descriptor is that it is not linked to OtherSecurityValue.

I'm not sure. Can you do prototext.Marshal() of the output of the specific proto.UnmarshalText? Maybe that will show you the format prototext.Unmarshal wants?

Doing:

err := proto.UnmarshalText(want[i], w)
if err != nil {
  t.Fatal(err)
}
_, err = prototext.Marshal(w)
if err != nil {
  t.Fatal(err)
}

there are not errors returned...

dfawley · 2024-03-25T15:31:15Z

there are not errors returned...

We wouldn't expect errors. I was curious what the output would look like as that should really be something that would be accepted by prototext.Unmarshal.

Clement-Jean · 2024-03-26T01:22:42Z

The issue seems to be coming from the function called addr which doesn't return valid UTF8 strings. And prototext.Unmarshal seems to be expecting UTF8 strings.

Clement-Jean · 2024-03-26T01:53:06Z

I believe we could also remove this line in vet.sh:

# - Ensure all ptypes proto packages are renamed when importing.
not git grep "\(import \|^\s*\)\"github.com/golang/protobuf/ptypes/" -- "*.go"

Other than that, if there is no way to remove the dependency in reflection/grpc_testing_not_regenerate/testv3.go, I think we are pretty much done.

dfawley · 2024-03-26T16:29:54Z

The issue seems to be coming from the function called addr which doesn't return valid UTF8 strings. And prototext.Unmarshal seems to be expecting UTF8 strings.

Good find! Thanks for the fix.

I believe we could also remove this line in vet.sh:

I'm fine with that, but if we do that we should replace it with something that looks for "github.com/golang/protobuf" and forbids it anywhere but in reflection/grpc_testing_not_regenerate.

dfawley · 2024-03-26T16:35:53Z

channelz/service/service_test.go

+	"google.golang.org/protobuf/protoadapt"
+	"google.golang.org/protobuf/reflect/protoreflect"
+	"google.golang.org/protobuf/reflect/protoregistry"
+	"google.golang.org/protobuf/runtime/protoimpl"


WARNING: This package should only ever be imported by generated messages. The compatibility agreement covers nothing except for functionality needed to keep existing generated messages operational. Breakages that occur due to unauthorized usages of this package are not the author's responsibility.

I'd rather not use this package if it's at all possible to avoid it, unless you are extremely sure backward compatibility will be guaranteed for all our uses of it.

yes, I agree that using protoimpl is half a solution. I'm figuring out how to imrpove this.

Clement-Jean · 2024-04-02T06:46:01Z

@dfawley I found other references to github.com/golang/protobuf and I changed the vet.sh. Could you check?

The GetSocket test seems to fail and not consistently (flaky). Not sure why yet.

edit

It seems to be the order into which the value is encoded in OtherChannelzSecurityValue. If we look at when it fails, we have the following:

service_test.go:800: Socket 6 did not match expected.  -got +want:   (*grpc_channelz_v1.Socket)(Inverse(protocmp.Transform, protocmp.Message{
  "@type": s"grpc.channelz.v1.Socket",
  "data":  protocmp.Message{"@type": s"grpc.channelz.v1.SocketData", "last_local_stream_created_timestamp": protocmp.Message{"@type": s"google.protobuf.Timestamp"}, "last_message_received_timestamp": protocmp.Message{"@type": s"google.protobuf.Timestamp"}, "last_message_sent_timestamp": protocmp.Message{"@type": s"google.protobuf.Timestamp"}, ...},
  "ref":   protocmp.Message{"@type": s"grpc.channelz.v1.SocketRef", "name": string("6"), "socket_id": int64(123)},
  "security": protocmp.Message{
    "@type": s"grpc.channelz.v1.Security",
    "other": protocmp.Message{
       "@type": s"grpc.channelz.v1.Security.OtherSecurity",
       "name":  string("YYYY"),
       "value": protocmp.Message{
          "@type":    s"google.protobuf.Any",
          "type_url": string("type.googleapis.com/grpc.credentials.OtherChannelzSecurityValue"),
          "value": []uint8{
            -       0x0a, 0x03, 0x01, 0x02, 0x03, 0x12, 0x03, 0x04, 0x05, 0x06, // -|..........|
            +       0x12, 0x03, 0x04, 0x05, 0x06, 0x0a, 0x03, 0x01, 0x02, 0x03, // +|..........|
          },
        },
      },
    },
}))

And the values in value seem to be inverted. The 5 first in got should be the last 5 and inversely for the 5 last.

Clement-Jean · 2024-04-03T03:35:11Z

@dfawley It was because protobuf does not ensure encoding order of fields. Going with a generated proto message is easier.

dfawley

This looks great to me. Just one change to request. Thanks!

dfawley · 2024-04-03T21:08:28Z

channelz/service/service_test.proto

+
+package grpc.credentials;
+
+option go_package = "google.golang.org/grpc/channelz/service";


Can this go into service_test? If not, please put it in a subdirectory so it doesn't get pulled into the production build. (Even if it is eventually pruned out (?), it still feels wrong.)

Yeah, that's why I wanted to keep it as a descriptor in the test file at first... I think I will move the info back into the test file like it was and create the ProtoReflect function on OtherChannelzSecurityValue.

dfawley · 2024-04-03T21:15:04Z

credentials/credentials.go

@@ -287,5 +287,5 @@ type ChannelzSecurityValue interface {
 type OtherChannelzSecurityValue struct {
 	ChannelzSecurityValue
 	Name  string
-	Value protoadapt.MessageV1


This may be a breaking change. It is probably fine since it's experimental, and it's unlikely anyone is using it....so I think I'm OK with it. Just wanted to call it out.

dfawley

Thanks!

arvindbr8

LGTM. Thanks @Clement-Jean for making huge strides in fixing #5316.

gRPConf 2024 Registration is now open! Please find more details at grpc.io. Hope to see you there.

move channelz to google.golang.org/protobuf

2f40596

check timestamp validity

4dc482f

dfawley added the Type: Dependencies Updating/adding/removing dependencies label Mar 22, 2024

dfawley added this to the 1.64 Release milestone Mar 22, 2024

dfawley assigned Clement-Jean Mar 22, 2024

dfawley added the Status: Requires Reporter Clarification label Mar 22, 2024

replace proto with prototext and protoimpl

21fd935

dfawley reviewed Mar 26, 2024

View reviewed changes

dfawley mentioned this pull request Mar 26, 2024

channelz: refactor to move proto API to a separate package #7065

Merged

Clement-Jean added 3 commits March 27, 2024 10:18

Merge branch 'master' into remove_old_proto_pkg

5d2ed71

move to well-known types and solve conflict

18d5f31

rm protoimpl

5d342be

dfawley removed the Status: Requires Reporter Clarification label Mar 27, 2024

dfawley assigned dfawley and unassigned Clement-Jean Mar 27, 2024

Clement-Jean added 2 commits April 2, 2024 14:30

remove ptypes

54a4823

update check for github.com/golang/protobuf

a8f1787

remove flaky test by using proto file

39fb9b5

dfawley requested changes Apr 3, 2024

View reviewed changes

dfawley assigned Clement-Jean and unassigned dfawley Apr 3, 2024

dfawley added the Status: Requires Reporter Clarification label Apr 3, 2024

rm proto file and create ProtoReflect function

190126d

dfawley approved these changes Apr 5, 2024

View reviewed changes

dfawley requested a review from arvindbr8 April 5, 2024 17:35

dfawley assigned arvindbr8 and unassigned Clement-Jean Apr 5, 2024

dfawley removed the Status: Requires Reporter Clarification label Apr 5, 2024

arvindbr8 linked an issue Apr 5, 2024 that may be closed by this pull request

Update proto package imports to google.golang.org/protobuf/proto #5316

Closed

arvindbr8 approved these changes Apr 5, 2024

View reviewed changes

arvindbr8 merged commit 879414f into grpc:master Apr 5, 2024
14 checks passed

Clement-Jean deleted the remove_old_proto_pkg branch April 6, 2024 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: move from github.com/golang/protobuf to google.golang.org/protobuf/proto #7059

deps: move from github.com/golang/protobuf to google.golang.org/protobuf/proto #7059

Clement-Jean commented Mar 21, 2024 •

edited by dfawley

Loading

codecov bot commented Mar 21, 2024 •

edited

Loading

dfawley commented Mar 22, 2024

Clement-Jean commented Mar 25, 2024 •

edited

Loading

dfawley commented Mar 25, 2024

Clement-Jean commented Mar 26, 2024

Clement-Jean commented Mar 26, 2024

dfawley commented Mar 26, 2024

dfawley Mar 26, 2024

Clement-Jean Mar 27, 2024

Clement-Jean commented Apr 2, 2024 •

edited

Loading

Clement-Jean commented Apr 3, 2024

dfawley left a comment

dfawley Apr 3, 2024

Clement-Jean Apr 4, 2024 •

edited

Loading

dfawley Apr 3, 2024

dfawley left a comment

arvindbr8 left a comment


		package grpc.credentials;

		option go_package = "google.golang.org/grpc/channelz/service";

deps: move from github.com/golang/protobuf to google.golang.org/protobuf/proto #7059

deps: move from github.com/golang/protobuf to google.golang.org/protobuf/proto #7059

Conversation

Clement-Jean commented Mar 21, 2024 • edited by dfawley Loading

codecov bot commented Mar 21, 2024 • edited Loading

Codecov Report

dfawley commented Mar 22, 2024

Clement-Jean commented Mar 25, 2024 • edited Loading

dfawley commented Mar 25, 2024

Clement-Jean commented Mar 26, 2024

Clement-Jean commented Mar 26, 2024

dfawley commented Mar 26, 2024

dfawley Mar 26, 2024

Choose a reason for hiding this comment

Clement-Jean Mar 27, 2024

Choose a reason for hiding this comment

Clement-Jean commented Apr 2, 2024 • edited Loading

edit

Clement-Jean commented Apr 3, 2024

dfawley left a comment

Choose a reason for hiding this comment

dfawley Apr 3, 2024

Choose a reason for hiding this comment

Clement-Jean Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

dfawley Apr 3, 2024

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

arvindbr8 left a comment

Choose a reason for hiding this comment

Clement-Jean commented Mar 21, 2024 •

edited by dfawley

Loading

codecov bot commented Mar 21, 2024 •

edited

Loading

Clement-Jean commented Mar 25, 2024 •

edited

Loading

Clement-Jean commented Apr 2, 2024 •

edited

Loading

Clement-Jean Apr 4, 2024 •

edited

Loading