Move istio/proxy protos into istio/api #2107

howardjohn · 2021-09-30T18:39:51Z

This does two things:

Move some protos we have in istio/istio that are manually generating
the go protos, using an outdated protobuf library and outdated proto.
Move some protos we have in istio/proxy that are not generating any go
protos at all.

A next step will be to have istio/istio import these go libraries, and
istio/proxy to import these as proto_libraries.

istio-policy-bot · 2021-09-30T18:39:53Z

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

istio-testing · 2021-09-30T18:39:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

howardjohn · 2021-09-30T18:40:04Z

@kyessenov @yangminzhu any concerns with this?

kyessenov · 2021-09-30T18:41:35Z

Yeah, I think this is just adding overhead. These are not user visible APIs. This does not help proxy build at all (which is all bazel).

howardjohn · 2021-09-30T18:43:05Z

Yeah, I think this is just adding overhead. These are not user visible APIs. This does not help proxy build at all (which is all bazel).

I wasn't sure if it was use for proxy build but I think it is for istio/istio. This was mostly inspired by your comment, which I agree with: istio/istio#33583 (comment).

We don't have to change istio/proxy if its just annoying. WDYT?

kyessenov · 2021-09-30T19:32:13Z

You can manually generate and commit protos to istio/istio with standard toolchain. It's not like they change every month. It's just confusing to require TOC approval for machine protos so this repo is not appropriate.

howardjohn · 2021-09-30T19:42:43Z

We do already have half of them here. I would prefer to be consistent personally. It's also useful to be able to import just the Protos without the dependency hell of Istio/istio - Protos living in Istio repo has been an issue for at least 3 projects I know of

…

On Thu, Sep 30, 2021, 12:32 PM Kuat ***@***.***> wrote: You can manually generate and commit protos to istio/istio with standard toolchain. It's not like they change every month. It's just confusing to require TOC approval for machine protos so this repo is not appropriate. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2107 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXLDWVDQL5PCXM3TAR3UES3MRANCNFSM5FC674AQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

howardjohn · 2021-09-30T19:43:07Z

we can make Network team the owners of envoy/?

…

On Thu, Sep 30, 2021, 12:42 PM John Howard ***@***.***> wrote: We do already have half of them here. I would prefer to be consistent personally. It's also useful to be able to import just the Protos without the dependency hell of Istio/istio - Protos living in Istio repo has been an issue for at least 3 projects I know of On Thu, Sep 30, 2021, 12:32 PM Kuat ***@***.***> wrote: > You can manually generate and commit protos to istio/istio with standard > toolchain. It's not like they change every month. It's just confusing to > require TOC approval for machine protos so this repo is not appropriate. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#2107 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAEYGXLDWVDQL5PCXM3TAR3UES3MRANCNFSM5FC674AQ> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > >

kyessenov · 2021-09-30T20:01:01Z

As I mentioned earlier, this is just pure overhead IMHO. Yes, it can be done, but the value of tooling in istio/api for Envoy build is close to zero if not negative. Some protos in istio/proxy are not even protos, as they are just documentation for Wasm configs (which is not using protobuf).

howardjohn · 2021-09-30T21:19:43Z

I mentioned above "We don't have to change istio/proxy if its just annoying", so it shouldn't impact istio/proxy developers.

We have to put them somewhere else (can be a copy), since istio/istio importing istio/proxy is probably not a good idea. That leaves us istio/api or istio/istio.

IMO istio/api is a better choice, as we already have half the protos here, already have all the tooling setup to generate protos, and it allows folks to import just the protos without all of istio/istio.

What would you prefer as a way to solve istio/istio#35210?

yangminzhu · 2021-10-01T00:19:32Z

I like this PR for making it auto generated instead of relying on manually generating the proto code. It's always better to be automatic even if these files does not change often.

The tooling changes seems very small in the PR so I guess there should not be too much overhead of it?

howardjohn · 2021-10-04T16:38:41Z

Blocked by istio/tools#1718

howardjohn · 2021-10-05T00:42:57Z

@kyessenov do you have concerns if we do not touch istio/proxy?

Fixes istio/istio#35210. This does two things: * Move some protos we have in istio/istio that are manually generating the go protos, using an outdated protobuf library and outdated proto. * Move some protos we have in istio/proxy that are not generating any go protos at all. A next step will be to have istio/istio import these go libraries, and istio/proxy to import these as proto_libraries.

kyessenov · 2021-10-05T02:33:41Z

My vote is not binding, feel free to merge. It can always be reverted if it becomes a hassle.

howardjohn · 2021-10-05T17:24:00Z

/retest

douglas-reid · 2021-10-05T20:43:20Z

These configs are definitely not meant to be user-facing, but we've stored similar schema in istio/api before (mixer client config).

Is there a way to annotate them, or highlight the implementation-detail-ish nature of them prominently? Would shoving them into internal/... or similar work?

We definitely need to keep them in sync (either through remote deps in istio/proxy, or some sort of sync job if pushing).

Will we need to update whatever is generating the reference docs for them as well (https://istio.io/latest/docs/reference/config/proxy_extensions/) ?

howardjohn · 2021-10-05T22:15:51Z

Is there a way to annotate them, or highlight the implementation-detail-ish nature of them prominently? Would shoving them into internal/... or similar work?

Do you think a README in envoy/ is sufficient? There are already protos in envoy/ that are the ~same so I am a bit hesitant to change the folder structure or be inconsistent with those

douglas-reid · 2021-10-05T23:15:20Z

Do you think a README in envoy/ is sufficient? There are already protos in envoy/ that are the ~same so I am a bit hesitant to change the folder structure or be inconsistent with those

SGTM.

louiscryan · 2021-10-05T23:39:42Z

buf.gen-golang.yaml

+plugins:
+- name: go
+  out: .
+  opt: paths=source_relative


missing newline?

louiscryan · 2021-10-05T23:43:05Z

I think this makes sense. Can you add the README to this CL to help folks contextualize a bit?

louiscryan

Approved. Please add the suggested README prior to commit

kyessenov · 2021-10-06T04:07:08Z

@douglas-reid Thanks for reminding me of the horrors of mixerclient protos from here used in proxy bazel (with all the gogo crap).

howardjohn · 2021-10-06T16:42:13Z

README added

kyessenov · 2023-01-05T19:17:27Z

These have actually diverged now like I was afraid of, so hopefully there's no user facing repercussions.

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Sep 30, 2021

google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Sep 30, 2021

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 30, 2021

howardjohn marked this pull request as ready for review October 1, 2021 23:21

howardjohn requested review from brian-avery, linsun, louiscryan, nrjpoddar and smawson as code owners October 1, 2021 23:21

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 1, 2021

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Oct 4, 2021

howardjohn force-pushed the envoy-protos branch from ebb1190 to a5fd17a Compare October 4, 2021 16:33

howardjohn force-pushed the envoy-protos branch from a5fd17a to 5818bf2 Compare October 5, 2021 00:36

howardjohn force-pushed the envoy-protos branch from 5818bf2 to 178e4ac Compare October 5, 2021 00:43

louiscryan reviewed Oct 5, 2021

View reviewed changes

buf.gen-golang.yaml

plugins:

- name: go

out: .

opt: paths=source_relative

Copy link

Contributor

louiscryan Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline?

louiscryan approved these changes Oct 6, 2021

View reviewed changes

add README

efc184d

douglas-reid approved these changes Oct 6, 2021

View reviewed changes

istio-testing merged commit 8e91abc into istio:master Oct 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move istio/proxy protos into istio/api #2107

Move istio/proxy protos into istio/api #2107

howardjohn commented Sep 30, 2021

istio-policy-bot commented Sep 30, 2021

istio-testing commented Sep 30, 2021

howardjohn commented Sep 30, 2021

kyessenov commented Sep 30, 2021

howardjohn commented Sep 30, 2021

kyessenov commented Sep 30, 2021

howardjohn commented Sep 30, 2021 via email

howardjohn commented Sep 30, 2021 via email

kyessenov commented Sep 30, 2021

howardjohn commented Sep 30, 2021 •

edited

Loading

yangminzhu commented Oct 1, 2021

howardjohn commented Oct 4, 2021

howardjohn commented Oct 5, 2021

kyessenov commented Oct 5, 2021

howardjohn commented Oct 5, 2021

douglas-reid commented Oct 5, 2021

howardjohn commented Oct 5, 2021

douglas-reid commented Oct 5, 2021

louiscryan Oct 5, 2021

louiscryan commented Oct 5, 2021

louiscryan left a comment

kyessenov commented Oct 6, 2021

howardjohn commented Oct 6, 2021

kyessenov commented Jan 5, 2023

Move istio/proxy protos into istio/api #2107

Move istio/proxy protos into istio/api #2107

Conversation

howardjohn commented Sep 30, 2021

istio-policy-bot commented Sep 30, 2021

istio-testing commented Sep 30, 2021

howardjohn commented Sep 30, 2021

kyessenov commented Sep 30, 2021

howardjohn commented Sep 30, 2021

kyessenov commented Sep 30, 2021

howardjohn commented Sep 30, 2021 via email

howardjohn commented Sep 30, 2021 via email

kyessenov commented Sep 30, 2021

howardjohn commented Sep 30, 2021 • edited Loading

yangminzhu commented Oct 1, 2021

howardjohn commented Oct 4, 2021

howardjohn commented Oct 5, 2021

kyessenov commented Oct 5, 2021

howardjohn commented Oct 5, 2021

douglas-reid commented Oct 5, 2021

howardjohn commented Oct 5, 2021

douglas-reid commented Oct 5, 2021

louiscryan Oct 5, 2021

Choose a reason for hiding this comment

louiscryan commented Oct 5, 2021

louiscryan left a comment

Choose a reason for hiding this comment

kyessenov commented Oct 6, 2021

howardjohn commented Oct 6, 2021

kyessenov commented Jan 5, 2023

howardjohn commented Sep 30, 2021 •

edited

Loading