Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

gitserver: grpc: relax BlameAuthor name field to allow arbitrary byte sequences#62917

Merged
ggilmore merged 1 commit into
mainfrom
05-24-gitserver_grpc_relax_blameauthor_name_field_to_allow_arbitrary_byte_sequences
May 29, 2024
Merged

gitserver: grpc: relax BlameAuthor name field to allow arbitrary byte sequences#62917
ggilmore merged 1 commit into
mainfrom
05-24-gitserver_grpc_relax_blameauthor_name_field_to_allow_arbitrary_byte_sequences

Conversation

@ggilmore

@ggilmore ggilmore commented May 24, 2024

Copy link
Copy Markdown
Contributor

It seems that git author names are allowed to be non utf-8 fields.

As observed on sourcegraph.com (link).

{
  "insertId": "rfy02652om7bc152",
  "jsonPayload": {
    "Function": "github.com/sourcegraph/sourcegraph/internal/grpc/internalerrs.doLog",
    "Resource": {
      "service.name": "gitserver",
      "service.instance.id": "gitserver-0",
      "service.version": "274109_2024-05-14_5.4-0d2f0e7fb786"
    },
    "message": "grpc: error while marshaling: string field contains invalid UTF-8",
    "InstrumentationScope": "gitserver.gRPC.internal.error.reporter.streamingMethod.postMessageSend",
    "timestampNanos": 1715684539936480800,
    "Attributes": {
      "grpcService": "gitserver.v1.GitserverService",
      "nonUTF8StringFields": [
        "hunk.author.name"
      ],
      "initialRequestJSON": "{\"repo_name\":\"github.com/torvalds/linux\",\"commit\":\"a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6\",\"path\":\"drivers/usb/storage/unusual_devs.h\"}",
      "messageJSON": "{\"hunk\":{\"start_line\":2398,\"end_line\":2399,\"commit\":\"63dc3ff3e019287e8cb4647808de1d93acddd006\",\"author\":{\"name\":\"David H\\ufffdrdeman\",\"email\":\"david@2gen.com\",\"date\":{\"seconds\":1132789549}},\"message\":\"[PATCH] USB: fix USB key generates ioctl_internal_command errors issue\",\"filename\":\"drivers/usb/storage/unusual_devs.h\",\"previous_commit\":{\"commit\":\"21b1861fb2ba5b25b32c63bc540bbc7ca1d186f8\",\"filename\":\"drivers/usb/storage/unusual_devs.h\"}}}",
      "grpcCode": "Internal",
      "grpcMethod": "Blame"
    },
    "Caller": "internalerrs/logging.go:239"
  },
  "resource": {
    "type": "k8s_container",
    "labels": {
      "namespace_name": "prod",
      "container_name": "gitserver",
      "location": "us-central1-f",
      "cluster_name": "cloud",
      "project_id": "sourcegraph-dev",
      "pod_name": "gitserver-0"
    }
  },
  "timestamp": "2024-05-14T11:02:19.936736171Z",
  "severity": "ERROR",
  "labels": {
    "k8s-pod/controller-revision-hash": "gitserver-65d76d6ff8",
    "k8s-pod/deploy": "sourcegraph",
    "k8s-pod/type": "gitserver",
    "k8s-pod/group": "backend",
    "k8s-pod/app": "gitserver",
    "k8s-pod/statefulset_kubernetes_io/pod-name": "gitserver-0",
    "compute.googleapis.com/resource_name": "gke-cloud-containerd-pool-3ff685fe-tvqd",
    "k8s-pod/purpose": "vertical-pod-autoscaler-try-recommend"
  },
  "logName": "projects/sourcegraph-dev/logs/stderr",
  "receiveTimestamp": "2024-05-14T11:02:24.996687291Z"
}

As such, we need to relax the field type to allow for arbitrary byte sequences (as opposed to the string type that enforces uftf-8 encoding).

Test plan

Existing CI

@ggilmore ggilmore marked this pull request as ready for review May 24, 2024 21:10
@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_change_blamerequest_path_field_from_string_to_bytes branch from 6b8dd77 to ba79370 Compare May 24, 2024 21:17
@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_relax_blameauthor_name_field_to_allow_arbitrary_byte_sequences branch from 51a7e8b to 61ec65a Compare May 24, 2024 21:17
@graphite-app

graphite-app Bot commented May 27, 2024

Copy link
Copy Markdown

Movie gif. Sacha Baron Cohen as Borat sits in front of a map and raises his two thumbs, waggling his eyebrows and giving an approving nod and toothy grin. (Added via Giphy)

@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_change_blamerequest_path_field_from_string_to_bytes branch from ba79370 to a5c9f0a Compare May 28, 2024 19:59
@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_relax_blameauthor_name_field_to_allow_arbitrary_byte_sequences branch from 61ec65a to 1c8fa59 Compare May 28, 2024 20:00
@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_change_blamerequest_path_field_from_string_to_bytes branch from a5c9f0a to 3e86bb8 Compare May 28, 2024 20:02
@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_relax_blameauthor_name_field_to_allow_arbitrary_byte_sequences branch from 1c8fa59 to 44aa557 Compare May 28, 2024 20:02
@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_change_blamerequest_path_field_from_string_to_bytes branch from 3e86bb8 to 61f1d42 Compare May 28, 2024 20:25
@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_relax_blameauthor_name_field_to_allow_arbitrary_byte_sequences branch from 44aa557 to c15fcdd Compare May 28, 2024 20:25
@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_change_blamerequest_path_field_from_string_to_bytes branch from 61f1d42 to cca5b48 Compare May 29, 2024 08:33
Base automatically changed from 05-24-gitserver_grpc_change_blamerequest_path_field_from_string_to_bytes to main May 29, 2024 08:49
@ggilmore ggilmore force-pushed the 05-24-gitserver_grpc_relax_blameauthor_name_field_to_allow_arbitrary_byte_sequences branch from c15fcdd to 294234a Compare May 29, 2024 08:50

ggilmore commented May 29, 2024

Copy link
Copy Markdown
Contributor Author

Merge activity

  • May 29, 4:51 AM EDT: Graphite rebased this pull request as part of a merge.
  • May 29, 5:18 AM EDT: @ggilmore merged this pull request with Graphite.

@ggilmore ggilmore merged commit d1f7715 into main May 29, 2024
@ggilmore ggilmore deleted the 05-24-gitserver_grpc_relax_blameauthor_name_field_to_allow_arbitrary_byte_sequences branch May 29, 2024 09:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants