go proto: include (opt-in) vtprotobuf generation #31172

howardjohn · 2023-12-04T23:27:17Z

Commit Message: go proto: include vtprotobuf generation
Additional Description:

See envoyproxy/go-control-plane#824 for more information

This PR adds the vtprotobuf protoc plugin for Go. This works on top of the existing protoc-gen-go, to add optimized marshal functions that callers can opt in to using. This is not like gogo, which was a very invasive change -- everything is layered and opt-in. See issue for benchmarks, etc.

Additionally, to avoid possible binary size increase, the entire new code is protected under a go build tag. Users will need to opt-in at build time (-tags=vtprotobuf). By default, there is no impact for users at all.

Risk Level: Low - only additional opt-in code
Testing: Manually tested in Istio codebase
Docs Changes: NONE
Release Notes: NONE
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2023-12-04T23:27:25Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @moderation

🐱

Caused by: #31172 was opened by howardjohn.

see: more, trace.

moderation

For the binary size risk you highlight, is that risk for the go-control-plane binary, the Envoy binary or both?

Binary size: importing every proto results in a 4MB increase in binary size. Not its quite uncommon to import every proto in the repo, so typically this cost is much smaller
Mitigation, if needed: expose this only under a build tag, so a use can opt in (or opt out)

moderation · 2023-12-05T02:44:49Z

bazel/dependency_imports.bzl

@@ -146,6 +146,13 @@ def envoy_dependency_imports(go_version = GO_VERSION, jq_version = JQ_VERSION, y
        # use_category = ["api"],
        # source = "https://github.com/bufbuild/protoc-gen-validate/blob/v0.6.1/dependencies.bzl#L23-L28"
    )
+    go_repository(
+        name = "com_github_planetscale_vtprotobuf",
+        importpath = "github.com/planetscale/vtprotobuf",


OSSF Scorecard results:

scorecard --repo=https://github.com/planetscale/vtprotobuf RESULTS ------- Aggregate score: 4.2 / 10 Check scores: |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | SCORE | NAME | REASON | DOCUMENTATION/REMEDIATION | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Binary-Artifacts | no binaries found in the repo | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#binary-artifacts | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Branch-Protection | branch protection not enabled | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#branch-protection | | | | on development/release | | | | | branches | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | CI-Tests | 6 out of 6 merged PRs | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#ci-tests | | | | checked by a CI test -- score | | | | | normalized to 10 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | CII-Best-Practices | no effort to earn an OpenSSF | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#cii-best-practices | | | | best practices badge detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 7 / 10 | Code-Review | found 2 unreviewed changesets | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#code-review | | | | out of 7 -- score normalized | | | | | to 7 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Contributors | project has 13 contributing | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#contributors | | | | companies or organizations | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Dangerous-Workflow | no dangerous workflow patterns | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#dangerous-workflow | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Dependency-Update-Tool | no update tool detected | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#dependency-update-tool | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Fuzzing | project is not fuzzed | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#fuzzing | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | License | license file detected | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#license | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Maintained | 30 commit(s) and 7 issue | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#maintained | | | | activity found in the last 90 | | | | | days -- score normalized to 10 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | ? | Packaging | packaging workflow not | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#packaging | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 2 / 10 | Pinned-Dependencies | dependency not pinned by hash | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#pinned-dependencies | | | | detected -- score normalized | | | | | to 2 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | SAST | SAST tool is not run on all | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#sast | | | | commits -- score normalized to | | | | | 0 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Security-Policy | security policy file not | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#security-policy | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | ? | Signed-Releases | no releases found | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#signed-releases | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Token-Permissions | detected GitHub workflow | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#token-permissions | | | | tokens with excessive | | | | | permissions | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Vulnerabilities | 14 existing vulnerabilities | https://github.com/ossf/scorecard/blob/483cc31b60c63cc7e13a9b4211ff6acf8b4d7d20/docs/checks.md#vulnerabilities | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|

I updated the repo, now its better in a few areas:

Aggregate score: 5.5 / 10 |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | SCORE | NAME | REASON | DOCUMENTATION/REMEDIATION | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Binary-Artifacts | no binaries found in the repo | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#binary-artifacts | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Branch-Protection | branch protection is fully | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#branch-protection | | | | enabled on development and all | | | | | release branches | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | ? | CI-Tests | internal error: cannot list | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#ci-tests | | | | check runs by ref | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | CII-Best-Practices | no badge detected | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#cii-best-practices | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 8 / 10 | Code-Review | GitHub code reviews found for | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#code-review | | | | 26 commits out of the last 30 | | | | | -- score normalized to 8 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Contributors | 13 different companies found | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#contributors | | | | -- score normalized to 10 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Dependency-Update-Tool | no update tool detected | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#dependency-update-tool | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Fuzzing | project is not fuzzed | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#fuzzing | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Maintained | 30 commit(s) out of 30 and 22 | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#maintained | | | | issue activity out of 30 found | | | | | in the last 90 days -- score | | | | | normalized to 10 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | ? | Packaging | no published package detected | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#packaging | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 8 / 10 | Pinned-Dependencies | dependency not pinned by hash | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#pinned-dependencies | | | | detected -- score normalized | | | | | to 8 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | SAST | no SAST tool detected | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#sast | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Security-Policy | security policy file not | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#security-policy | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | ? | Signed-Releases | no releases found | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#signed-releases | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Token-Permissions | non read-only tokens detected | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#token-permissions | | | | in GitHub workflows | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Vulnerabilities | no vulnerabilities detected | https://github.com/ossf/scorecard/blob/23b0ddb8aa96356321cf31a2709723e29b15a951/docs/checks.md#vulnerabilities | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|

I'm not seeing those changes. I'm not seeing branch protection and your run doesn't see CI. Adding a security policy would be good too.

scorecard version __ ____ ____ ___ ____ _____ ____ _ ____ ____ / / / ___| / ___| / _ \ | _ \ | ____| / ___| / \ | _ \ | _ \ / / \___ \ | | | | | | | |_) | | _| | | / _ \ | |_) | | | | | _ / / ___) | | |___ | |_| | | _ < | |___ | |___ / ___ \ | _ < | |_| | (_) /_/ |____/ \____| \___/ |_| \_\ |_____| \____| /_/ \_\ |_| \_\ |____/ ./scorecard: OpenSSF Scorecard GitVersion: v4.13.1-84-gcb721a85-dirty GitCommit: cb721a8526fccd47bc234d9d2005eecd01e4b0ae GitTreeState: dirty BuildDate: 1701764656 GoVersion: go1.21.4 Compiler: gc Platform: linux/amd64

scorecard --repo=https://github.com/planetscale/vtprotobuf RESULTS ------- Aggregate score: 4.9 / 10 Check scores: |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | SCORE | NAME | REASON | DOCUMENTATION/REMEDIATION | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Binary-Artifacts | no binaries found in the repo | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#binary-artifacts | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Branch-Protection | branch protection not enabled | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#branch-protection | | | | on development/release | | | | | branches | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | CI-Tests | 6 out of 6 merged PRs | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#ci-tests | | | | checked by a CI test -- score | | | | | normalized to 10 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | CII-Best-Practices | no effort to earn an OpenSSF | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#cii-best-practices | | | | best practices badge detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 8 / 10 | Code-Review | found 1 unreviewed changesets | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#code-review | | | | out of 7 -- score normalized | | | | | to 8 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Contributors | project has 13 contributing | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#contributors | | | | companies or organizations | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Dangerous-Workflow | no dangerous workflow patterns | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#dangerous-workflow | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Dependency-Update-Tool | no update tool detected | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#dependency-update-tool | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Fuzzing | project is not fuzzed | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#fuzzing | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | License | license file detected | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#license | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 10 / 10 | Maintained | 30 commit(s) and 6 issue | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#maintained | | | | activity found in the last 90 | | | | | days -- score normalized to 10 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | ? | Packaging | packaging workflow not | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#packaging | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 2 / 10 | Pinned-Dependencies | dependency not pinned by hash | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#pinned-dependencies | | | | detected -- score normalized | | | | | to 2 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | SAST | SAST tool is not run on all | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#sast | | | | commits -- score normalized to | | | | | 0 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Security-Policy | security policy file not | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#security-policy | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | ? | Signed-Releases | no releases found | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#signed-releases | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 0 / 10 | Token-Permissions | detected GitHub workflow | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#token-permissions | | | | tokens with excessive | | | | | permissions | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------| | 8 / 10 | Vulnerabilities | 2 existing vulnerabilities | https://github.com/ossf/scorecard/blob/cb721a8526fccd47bc234d9d2005eecd01e4b0ae/docs/checks.md#vulnerabilities | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------------------------------------------|

Vulnerabilities went from 0 (your original) -> 10 (my run) -> 8 (your run).
Looks like I was on an older version of scorecard, hence the differences

howardjohn · 2023-12-05T02:52:58Z

For the binary size risk you highlight, is that risk for the go-control-plane binary, the Envoy binary or both?

Binary size: importing every proto results in a 4MB increase in binary size. Not its quite uncommon to import every proto in the repo, so typically this cost is much smaller
Mitigation, if needed: expose this only under a build tag, so a use can opt in (or opt out)

this exclusively impacts go-control-plane (binary and importers of the library)

kyessenov · 2023-12-05T03:47:57Z

cc @dfawley since grpc-go consumes go-control-plane

dfawley · 2023-12-05T17:29:53Z

cc @dfawley since grpc-go consumes go-control-plane

Thank you. IIUC this should not affect us since we don't use bazel?

howardjohn · 2023-12-05T17:30:58Z

cc @dfawley since grpc-go consumes go-control-plane

Thank you. IIUC this should not affect us since we don't use bazel?

This will impact the generated code in go-control-plane (everything under https://github.com/envoyproxy/go-control-plane/tree/main/envoy)

dfawley · 2023-12-05T19:24:14Z

I see...

Binary size: importing every proto results in a 4MB increase in binary size

How can I see what the impact will be to grpc-go binary sizes with xds enabled?

Our set of protos imported is currently:

"github.com/envoyproxy/go-control-plane/envoy/admin/v3"
"github.com/envoyproxy/go-control-plane/envoy/api/v2"
"github.com/envoyproxy/go-control-plane/envoy/api/v2/core"
"github.com/envoyproxy/go-control-plane/envoy/api/v2/endpoint"
"github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/endpoint/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/listener/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/rbac/v2"
"github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/clusters/aggregate/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/common/fault/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/fault/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/rbac/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/router/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/network/http_connection_manager/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/client_side_weighted_round_robin/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/least_request/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/pick_first/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/ring_hash/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/round_robin/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/wrr_locality/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/rbac/audit_loggers/stream/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/transport_sockets/tls/v3"
"github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
"github.com/envoyproxy/go-control-plane/envoy/service/load_stats/v3"
"github.com/envoyproxy/go-control-plane/envoy/service/status/v3"
"github.com/envoyproxy/go-control-plane/envoy/type"
"github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3"
"github.com/envoyproxy/go-control-plane/envoy/type/v3"
"github.com/envoyproxy/go-control-plane/pkg/cache/types"
"github.com/envoyproxy/go-control-plane/pkg/cache/v3"
"github.com/envoyproxy/go-control-plane/pkg/resource/v3"
"github.com/envoyproxy/go-control-plane/pkg/server/v3"
"github.com/envoyproxy/go-control-plane/pkg/wellknown"

howardjohn · 2023-12-05T19:33:06Z

I see...

Binary size: importing every proto results in a 4MB increase in binary size

How can I see what the impact will be to grpc-go binary sizes with xds enabled?

Our set of protos imported is currently:

"github.com/envoyproxy/go-control-plane/envoy/admin/v3"
"github.com/envoyproxy/go-control-plane/envoy/api/v2"
"github.com/envoyproxy/go-control-plane/envoy/api/v2/core"
"github.com/envoyproxy/go-control-plane/envoy/api/v2/endpoint"
"github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/endpoint/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/listener/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/rbac/v2"
"github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3"
"github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/clusters/aggregate/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/common/fault/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/fault/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/rbac/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/router/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/filters/network/http_connection_manager/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/client_side_weighted_round_robin/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/least_request/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/pick_first/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/ring_hash/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/round_robin/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/load_balancing_policies/wrr_locality/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/rbac/audit_loggers/stream/v3"
"github.com/envoyproxy/go-control-plane/envoy/extensions/transport_sockets/tls/v3"
"github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
"github.com/envoyproxy/go-control-plane/envoy/service/load_stats/v3"
"github.com/envoyproxy/go-control-plane/envoy/service/status/v3"
"github.com/envoyproxy/go-control-plane/envoy/type"
"github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3"
"github.com/envoyproxy/go-control-plane/envoy/type/v3"
"github.com/envoyproxy/go-control-plane/pkg/cache/types"
"github.com/envoyproxy/go-control-plane/pkg/cache/v3"
"github.com/envoyproxy/go-control-plane/pkg/resource/v3"
"github.com/envoyproxy/go-control-plane/pkg/server/v3"
"github.com/envoyproxy/go-control-plane/pkg/wellknown"

Using examples/features/xds/client and go build -ldflags '-extldflags -static -s -w' .:

Before: 21M (21368832 bytes)
After: 23M (24104960 bytes)

go mod edit -replace github.com/envoyproxy/go-control-plane=github.com/howardjohn/go-control-plane@v0.0.0-20231204221533-4e477fbf1a5b can temporarily test the change before this merges

soulxu · 2023-12-07T02:18:34Z

@adisuissa Would you like to take a look at this? but let me know if I'm assigned to the wrong person. Thanks!

soulxu · 2023-12-07T02:18:43Z

/assign @adisuissa

adisuissa · 2023-12-07T15:43:05Z

Thanks for working on this!
Generally speaking I'm ok with this assuming the new dependency is ok, but want to highlight the risks that you mentioned:

Binary size: importing every proto results in a 4MB increase in binary size. Not its quite uncommon to import every proto in the repo, so typically this cost is much smaller
Mitigation, if needed: expose this only under a build tag, so a use can opt in (or opt out)
Increase dependency that could, in theory, start to decay.
Mitigation: dropping vtprotobuf would be compatible, as noted above (just have a large performance impact)

Specifically the binary size increase may be impacting Envoy-Mobile. @alyssawilk to chime in on this.

howardjohn · 2023-12-07T15:47:32Z

Specifically the binary size increase may be impacting Envoy-Mobile. @alyssawilk to chime in on this.
The impact is only for golang code which I assume doesn't impact mobile (?)

howardjohn · 2023-12-12T14:56:40Z

It seems like a lot of hangup is on binary size. What if we make it opt-in behind a go build tag?

This enables conditional compiling. This is particularly useful for shared protobufs for common APIs, where not all consumers have the same preferences around build size vs benefits of vtprotobuf. This allows the consumer of the library to chose whether vtprotobuf is used, rather than only the producer of the library. Additionally, it provides guidance to use `vtprotobuf` as the tag. The hope is that if multiple libraries do this, a user can just set that tag instead of `-tags=envoy_vt,enable_vtprotobuf_prometheus,vtproto_k8s` or something like that. See envoyproxy/envoy#31172, where this will be utilized (I think).

abeyad · 2023-12-12T17:56:43Z

The binary size will matter a lot for Envoy Mobile, 4MB is quite a bit to add to the binary size of a mobile app library. It would be interesting to see what the Envoy Mobile binary size impact is with and without this PR.

@howardjohn could you kindly run the following commands with and without your branch, and report back on the results:

bazel build --config=remote //test/performance:test_binary_size --config=sizeopt --copt=-ggdb3 --linkopt=-fuse-ld=lld --config=release-android
strip -s -o bazel-bin/test/performance/test_binary_size_stripped bazel-bin/test/performance/test_binary_size
ls -alh bazel-bin/test/performance/test_binary_size_stripped

howardjohn · 2023-12-12T17:57:39Z

The binary size will matter a lot for Envoy Mobile, 4MB is quite a bit to add to the binary size of a mobile app library. It would be interesting to see what the Envoy Mobile binary size impact is with and without this PR.

@howardjohn could you kindly run the following commands with and without your branch, and report back on the results:

bazel build --config=remote //test/performance:test_binary_size --config=sizeopt --copt=-ggdb3 --linkopt=-fuse-ld=lld --config=release-android

strip -s -o bazel-bin/test/performance/test_binary_size_stripped bazel-bin/test/performance/test_binary_size

ls -alh bazel-bin/test/performance/test_binary_size_stripped

This only impacts Go (via go-control-plane) so its not relevant to Envoy mobile

abeyad · 2023-12-12T17:59:54Z

The binary size will matter a lot for Envoy Mobile, 4MB is quite a bit to add to the binary size of a mobile app library. It would be interesting to see what the Envoy Mobile binary size impact is with and without this PR.
@howardjohn could you kindly run the following commands with and without your branch, and report back on the results:

bazel build --config=remote //test/performance:test_binary_size --config=sizeopt --copt=-ggdb3 --linkopt=-fuse-ld=lld --config=release-android

strip -s -o bazel-bin/test/performance/test_binary_size_stripped bazel-bin/test/performance/test_binary_size

ls -alh bazel-bin/test/performance/test_binary_size_stripped

This only impacts Go (via go-control-plane) so its not relevant to Envoy mobile

So it wouldn't get compiled into the Envoy Mobile .so? Based on your PR, it looks like it effects the api_proto_package which EM would indirectly use.

howardjohn · 2023-12-12T18:03:13Z

So it wouldn't get compiled into the Envoy Mobile .so? Based on your PR, it looks like it effects the api_proto_package which EM would indirectly use.

I don't see how it would, it only impacts go-control-plane. I cannot run the commands you sent to verify though, they do not seem to work for me.

abeyad · 2023-12-12T18:36:56Z

I don't see how it would, it only impacts go-control-plane. I cannot run the commands you sent to verify though, they do not seem to work for me.

I tried it and the binary size didn't change for Envoy Mobile. So I don't have concerns with this PR from the Envoy Mobile point of view. Thanks!

adisuissa

Given @abeyad ok, this PR LGTM from change perspective.
@moderation to weigh in on dependency perspective.

dfawley · 2023-12-12T19:01:07Z

I'm not sure this is a good idea.

This can never be removed once it is included.
It has downsides (binary size) to go along with its benefits (performance).
It might prevent us from upgrading protoc-gen-go if there is ever an incompatible change made in protoc-gen-go and this project stops being maintained.

Can vtproto not work as a separate add-on? Why does it need to generate code into the same package as the protoc-gen-go code?

howardjohn · 2023-12-12T19:07:09Z

This can never be removed once it is included.

I disagree. vtprotobuf is designed in a way such that the same program can compile with or without it included (https://github.com/vitessio/vitess/blob/40f314c1d052db3fa5a52e5fb2ddd2bddaefde44/go/vt/servenv/grpc_codec.go#L40-L49 for example). This gives envoy flexibility to rip it out at anytime without breaking compatibility with importers (of course, they don't get the benefits anymore). This is very different from gogo proto.

The build tag (discussed below) also further suggests users will not use the library in a broken way, as it is unlikely that the project is always compiled with the build tag (there will probably always be some dev that runs a raw go test, etc).

It has downsides (binary size) to go along with its benefits (performance).

See #31172 (comment) and planetscale/vtprotobuf#122, I intend to change this to make it opt-in.

It might prevent us from upgrading protoc-gen-go if there is ever an incompatible change made in protoc-gen-go and this project stops being maintained.

If so, we can remove it per above. I would not expect protoc-gen-go to make such changes given how things are layered; any change that could break vtprotobuf would laso likely completely break compatibility and represent a protobuf-go v3. vtproto doesn't depend on any internal details of protoc-gen-go really, aside from the fact a Go struct exists.

Can vtproto not work as a separate add-on? Why does it need to generate code into the same package as the protoc-gen-go code?

It is attaching methods to the types which can only be done in the same package

dfawley · 2023-12-12T19:22:17Z

It is attaching methods to the types which can only be done in the same package

Is that really the only way they could find to improve the performance?

Or are there maybe other options that would allow this project to benefit everyone without requiring someone to go modify the official codegen for every project that has protos?

adisuissa · 2024-01-02T14:40:27Z

/wait-any

phlax · 2024-01-09T09:41:33Z

@howardjohn seems DCO is broken

howardjohn · 2024-01-09T16:42:10Z

@howardjohn seems DCO is broken

Thanks, fixed.

adisuissa · 2024-01-09T21:51:25Z

cc @dfawley and @htuch for any comments, as they've probably had more experience in this domain.

Depends on envoyproxy/envoy#31675 and envoyproxy/envoy#31172, although we can merge before if we want.

dfawley · 2024-01-09T22:49:53Z

If this is opt-in, I have no concerns from the gRPC side, as our users will be unaffected (unless they choose to enable it in their builds).

* Optimize binary size and performance tradeoffs with build flags Depends on envoyproxy/envoy#31675 and envoyproxy/envoy#31172, although we can merge before if we want. * Update prow/benchtest.sh Co-authored-by: Eric Van Norman <ericvn@us.ibm.com> --------- Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>

PGV is extremely expensive in terms of binary size. This directly feeds into memory usage, storage and transmission costs, etc. For our binary, disable PGV (which we don't use) trims our binaries by a whopping 15MB! This PR has no negative impact on any users unless they explicitly opt out of PGV (with `-tags=disable_pgv`). All existing users are un-impacted. This change only impacts Go via the go-control-plane repo. This change is do as a post-processing step rather than in PGV upstream as PGV has moved to v1.0 and totally rewritten the repo, so the old PGV is frozen AFAIK. Since moving to v1.0 is a major effort (possibly not planned), the simple approach here was taken. This mirrors #31172, where similar proto additions are currently on hold due to the binary size increase. There we are also planning to take a similar opt-in to the binary size increase. In this case, we use an opt-out for backwards compatibility, as PGV has been around longer. Signed-off-by: John Howard <howardjohn@google.com>

…o#48712) * Optimize binary size and performance tradeoffs with build flags Depends on envoyproxy/envoy#31675 and envoyproxy/envoy#31172, although we can merge before if we want. * Update prow/benchtest.sh Co-authored-by: Eric Van Norman <ericvn@us.ibm.com> --------- Co-authored-by: Eric Van Norman <ericvn@us.ibm.com> (cherry picked from commit d533e52)

adisuissa · 2024-01-16T13:44:47Z

Please merge main.
Change LGTM. Assigning @htuch for senior-maintainer review.
/assign @htuch

adisuissa

LGTM, thanks!

howardjohn · 2024-01-16T16:56:26Z

@adisuissa thanks! I merged. Test is failing due to the v1.29.0 publishing I think

See envoyproxy/go-control-plane#824 for more information Signed-off-by: John Howard <howardjohn@google.com>

Signed-off-by: John Howard <howardjohn@google.com>

htuch

Makes sense as opt-in improvement, thanks.

Without this fix, vtprotobuf is accidentally enabled for everyone instead of just opt-in. Fixing envoyproxy#31172 Signed-off-by: John Howard <howardjohn@google.com>

Without this fix, vtprotobuf is accidentally enabled for everyone instead of just opt-in. Fixing #31172 Signed-off-by: John Howard <howardjohn@google.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Dec 4, 2023

repokitteh-read-only bot assigned moderation Dec 4, 2023

howardjohn force-pushed the protoc-gen-vt branch from 7e5d965 to 064cb85 Compare December 4, 2023 23:35

moderation reviewed Dec 5, 2023

View reviewed changes

repokitteh-read-only bot assigned adisuissa Dec 7, 2023

howardjohn mentioned this pull request Dec 12, 2023

Add ability to include a build tag planetscale/vtprotobuf#122

Merged

adisuissa reviewed Dec 12, 2023

View reviewed changes

howardjohn mentioned this pull request Dec 12, 2023

Expose vtprotobuf generation envoyproxy/go-control-plane#824

Closed

repokitteh-read-only bot removed the waiting:any label Jan 9, 2024

howardjohn force-pushed the protoc-gen-vt branch from 3a4fc81 to 2f09735 Compare January 9, 2024 15:52

howardjohn changed the title ~~go proto: include vtprotobuf generation~~ go proto: include (opt-in) vtprotobuf generation Jan 9, 2024

howardjohn requested a review from adisuissa January 9, 2024 21:33

howardjohn added a commit to howardjohn/istio that referenced this pull request Jan 9, 2024

Optimize binary size and performance tradeoffs with build flags

24af9da

Depends on envoyproxy/envoy#31675 and envoyproxy/envoy#31172, although we can merge before if we want.

howardjohn mentioned this pull request Jan 9, 2024

Optimize binary size and performance tradeoffs with build flags istio/istio#48712

Merged

repokitteh-read-only bot assigned htuch Jan 16, 2024

adisuissa previously approved these changes Jan 16, 2024

View reviewed changes

howardjohn dismissed adisuissa’s stale review via b173879 January 16, 2024 16:42

howardjohn force-pushed the protoc-gen-vt branch from 2f09735 to b173879 Compare January 16, 2024 16:42

howardjohn added 2 commits January 16, 2024 14:06

go proto: include vtprotobuf generation

238d30f

See envoyproxy/go-control-plane#824 for more information Signed-off-by: John Howard <howardjohn@google.com>

Make it opt-in

20481c5

Signed-off-by: John Howard <howardjohn@google.com>

howardjohn force-pushed the protoc-gen-vt branch from b173879 to 20481c5 Compare January 16, 2024 22:06

htuch approved these changes Jan 17, 2024

View reviewed changes

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jan 17, 2024

htuch merged commit 21b52ba into envoyproxy:main Jan 17, 2024
54 checks passed

howardjohn mentioned this pull request Jan 17, 2024

ci: fix typo in vtprotobuf disable logic #31865

Merged

ravenblackx pushed a commit that referenced this pull request Jan 18, 2024

ci: fix typo in vtprotobuf disable logic (#31865)

948d614

Without this fix, vtprotobuf is accidentally enabled for everyone instead of just opt-in. Fixing #31172 Signed-off-by: John Howard <howardjohn@google.com>

go proto: include (opt-in) vtprotobuf generation #31172

go proto: include (opt-in) vtprotobuf generation #31172

Conversation

howardjohn commented Dec 4, 2023 • edited Loading

repokitteh-read-only bot commented Dec 4, 2023

moderation left a comment

Choose a reason for hiding this comment

moderation Dec 5, 2023

Choose a reason for hiding this comment

howardjohn Dec 5, 2023

Choose a reason for hiding this comment

moderation Dec 5, 2023

Choose a reason for hiding this comment

howardjohn Dec 5, 2023

Choose a reason for hiding this comment

howardjohn commented Dec 5, 2023

kyessenov commented Dec 5, 2023

dfawley commented Dec 5, 2023

howardjohn commented Dec 5, 2023

dfawley commented Dec 5, 2023

howardjohn commented Dec 5, 2023

soulxu commented Dec 7, 2023

soulxu commented Dec 7, 2023

adisuissa commented Dec 7, 2023

howardjohn commented Dec 7, 2023

howardjohn commented Dec 12, 2023

abeyad commented Dec 12, 2023

howardjohn commented Dec 12, 2023

abeyad commented Dec 12, 2023

howardjohn commented Dec 12, 2023

abeyad commented Dec 12, 2023

adisuissa left a comment

Choose a reason for hiding this comment

dfawley commented Dec 12, 2023

howardjohn commented Dec 12, 2023 • edited Loading

dfawley commented Dec 12, 2023

adisuissa commented Jan 2, 2024

phlax commented Jan 9, 2024

howardjohn commented Jan 9, 2024

adisuissa commented Jan 9, 2024

dfawley commented Jan 9, 2024

adisuissa commented Jan 16, 2024

adisuissa left a comment

Choose a reason for hiding this comment

howardjohn commented Jan 16, 2024

htuch left a comment

Choose a reason for hiding this comment

howardjohn commented Dec 4, 2023 •

edited

Loading

howardjohn commented Dec 12, 2023 •

edited

Loading