fix(pubsub): prevent infinite retry with publishing invalid utf-8 chars

[hongalex]

INTERNAL is a retryable code for Publish but we should not indefinitely retry this error code when message attributes contains invalid utf-8 characters, which the grpc-go library returns as an INTERNAL error.

I made the decision to have the custom publish retry settings (currently just the invalid wrapping) take precedence over both the retry settings defined in the GAPIC layer, as well as user-specified custom retry. This is because we have no good way to detect whether or not the underlying GAPIC publisher client is using the default retry settings or a user specified version. Currently, there is no reason for users to need to override the custom publisher retry (since invalid utf-8 characters should never be retried), but this may change in the future. An alternative is to hard code the value of the default GAPIC publish retry settings to compare against, but this solution is inelegant.

Fixes #5268

[codyoss]

Just one minor nit, or else LGTM

[AndreasBergmeier6176]

Why not rather validate whether attribute values are valid UTF-8 to begin with? Wouldn't that be more elegant?

[hongalex]

It's cheaper to punt the behavior down to the gRPC level and have it bubble up the marshaling error, rather than add another check for every publish call to detect if there are UTF-8 characters.

Did this cause an issue for you recently?

[AndreasBergmeier6176]

It's cheaper to punt the behavior down to the gRPC level and have it bubble up the marshaling error, rather than add another check for every publish call to detect if there are UTF-8 characters.

Did this cause an issue for you recently?

Well, it might be cheaper but figuring out which of the attributes is the actual problem is a PITA. Especially if you have multiple layers of software that modifies the attributes. If the gRPC level would at least report, what the failing attribute key was...

[hongalex]

If the gRPC level would at least report, what the failing attribute key was...

If you get back the string field contains invalid UTF-8 error, consider iterating over the attributes and call utf8.ValidString to see which attribute failed.

[AndreasBergmeier6176]

If the gRPC level would at least report, what the failing attribute key was...

If you get back the string field contains invalid UTF-8 error, consider iterating over the attributes and call utf8.ValidString to see which attribute failed.

I think I would do that if the gRPC module would export said error. Currently nothing is preventing the gRPC implementation from just doing a rug-pull and use yet another error string.

[hongalex]

Yeah 100% agree that catching this based on the error string is brittle (and is what we're currently doing as well in the library). Are you willing to open an issue against grpc-go for this issue (needing to export the error)?

[AndreasBergmeier6176]

Yeah 100% agree that catching this based on the error string is brittle (and is what we're currently doing as well in the library). Are you willing to open an issue against grpc-go for this issue (needing to export the error)?

See here: golang/protobuf#1642