fully use buf to generate go from proto

[mmorel-35]

Please provide a description of this PR:

fully use buf to generate go from proto:

• move buf.yaml at the root
• declare buf.build/googleapis/googleapis and buf.build/k8s/api dependencies
• remove common-proto/k8s.io

[istio-testing]

Hi @mmorel-35. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[howardjohn]

@ericvn WDYT of this?

This puts a dependency on buf.build servers at build time (like we depend on the golang module proxy)

[costinm]

@ericvn WDYT of this?

This puts a dependency on buf.build servers at build time (like we depend on the golang module proxy)

Do we still support offline builds (disconnected from internet) / without buf.build ? ( I assume we can build without golang module proxy ).

At the very least the PR description should make this more clear and explain how to use without.

[mmorel-35]

I'm not really familiar with offline usage of buf.build, but the FAQ has some answers to that

[costinm]

Offline usage: "I am on an airplane" or "isolated environment is required for security reasons". I assume the buf.build servers are only used in 'make generate' - so not a problem. In context of XZ, it is a good idea for PR descriptions to be more clear - like "this PR will use an external service to generate some code - reviewers must pay extra attention to PRs including generated code, in case buf servers are compromised and start generating backdoors". We probably should reflect this into some Istio docs - I never paid attention to generated code, but now even binary test blobs need to be carefully considered...

…

On Tue, Apr 23, 2024 at 7:17 AM Matthieu MOREL ***@***.***> wrote: I'm not really familiar with offline usage of buf.build, but the FAQ has some answers to that <https://buf.build/docs/bsr/faqs#use-modules-offline> — Reply to this email directly, view it on GitHub <#50587 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2UQKVJTJ6KJCMCYEF3Y6ZUOVAVCNFSM6AAAAABGRSBWGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZSGQZTMNJYGU> . You are receiving this because your review was requested.Message ID: ***@***.***>

[howardjohn]

fwiw - The codegen still runs locally, just the proto sources are pulled from buf instead of checked into git. They do also have remote codegen but this PR isn't introducing it

[costinm]

That's a bit better - but still something that should be in the PR description. I assume someone modifying the proto sources is less critical - at worse they may remove security fields but the tests should catch that. The difference between this and golang module proxy is that the module proxy is untrusted - go still verifies the SHAs from go.sum. Given Istio use as a security product - and our insane build and code complexity - we need to start paying very close attention to this, and make sure "add a test" or "improve build time" PRs don't 'accidentally' do other things like change the security model of the build. ( I do like 'buf' a lot - happy to see this PR, wouldn't even mind codegen - but I see very little security considerations in PR description and reviews )

…

On Tue, Apr 23, 2024 at 8:00 AM John Howard ***@***.***> wrote: fwiw - The codegen still runs locally, just the proto sources are pulled from buf instead of checked into git. They do also have remote codegen but this PR isn't introducing it — Reply to this email directly, view it on GitHub <#50587 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2ROK6G3K2FTLFB4HRDY6ZZPDAVCNFSM6AAAAABGRSBWGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZSGU4TINBTGE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[costinm]

And for my understanding - by 'pull from buf instead of checked into git' - do you mean we make http requests on each "make gen" to some server, or is it a git repo that has all the protos in an organized structure ? Protos-in-git allow using standard/well-known tools to see the difference between versions, has some caching and mirroring for hermetic builds, etc - it's not only security that needs some good explanation.

…

On Tue, Apr 23, 2024 at 9:14 AM Costin Manolache ***@***.***> wrote: That's a bit better - but still something that should be in the PR description. I assume someone modifying the proto sources is less critical - at worse they may remove security fields but the tests should catch that. The difference between this and golang module proxy is that the module proxy is untrusted - go still verifies the SHAs from go.sum. Given Istio use as a security product - and our insane build and code complexity - we need to start paying very close attention to this, and make sure "add a test" or "improve build time" PRs don't 'accidentally' do other things like change the security model of the build. ( I do like 'buf' a lot - happy to see this PR, wouldn't even mind codegen - but I see very little security considerations in PR description and reviews ) On Tue, Apr 23, 2024 at 8:00 AM John Howard ***@***.***> wrote: > fwiw - The codegen still runs locally, just the proto sources are pulled > from buf instead of checked into git. They do also have remote codegen but > this PR isn't introducing it > > — > Reply to this email directly, view it on GitHub > <#50587 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAUR2ROK6G3K2FTLFB4HRDY6ZZPDAVCNFSM6AAAAABGRSBWGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZSGU4TINBTGE> > . > You are receiving this because your review was requested.Message ID: > ***@***.***> >

[bleggett]

That's a bit better - but still something that should be in the PR description. I assume someone modifying the proto sources is less critical - at worse they may remove security fields but the tests should catch that. The difference between this and golang module proxy is that the module proxy is untrusted - go still verifies the SHAs from go.sum.

This effectively does the same thing as go.sum but with buf.lock - either way, any mutation of the protos changes the SHA and would require a buf.lock update - which would generate a Git commit on our end.

So the risks here are the same risks we have today around the potential for someone to start compromising Go code we're pulling - no better and no worse, and in either case we have traceability/attestability via git.

[bleggett]

/ok-to-test

[bleggett]

I assume the buf.build servers are only used in 'make generate' - so not a problem.

I'm not really familiar with offline usage of buf.build, but the FAQ has some answers to that

I'm assuming this means we would be hitting buf servers at least once-per-CI-run? And if their servers were down our CI would start to fail? Unless we retain local state between runs which I don't think we do.

It also looks like there might be some caveats with the local cache, depending on whether you use plugins: bufbuild/buf#2543

I'll defer to @ericvn but while I like the idea here, and I don't think the security model changes, I am concerned that we wouldn't have a good mitigation if the buf servers went down for a day and that does concern me a bit.

[costinm]

No, John mentioned earlier that it's not using the buf servers at all. If that's correct, and we are getting some stuff from their git repos - I have no other concerns. Also I don't even mind even hitting buf servers - just want to make sure it is well understood from (build) security and review perspective, in context of recent events.

…

On Tue, Apr 30, 2024 at 3:11 PM Ben Leggett ***@***.***> wrote: I assume the buf.build servers are only used in 'make generate' - so not a problem. https://buf.build/docs/bsr/faqs#use-modules-offline I'm assuming this means we would be hitting buf servers at least once-per-CI-run? And if their servers were down our CI would start to fail? Unless we retain local state between runs which I don't think we do. It also looks like there might be some caveats with the local cache, depending on whether you use plugins: bufbuild/buf#2543 <bufbuild/buf#2543> — Reply to this email directly, view it on GitHub <#50587 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2RAVK5S6FKPOSQ7P6LZAAJHTAVCNFSM6AAAAABGRSBWGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBXGU2DAOJXHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[ericvn]

My first thought reading this was that we will use a local cache (after being download once from the server), then their servers, and if not available, we can get from another source (although I'm still not sure of the last item). I think that should be easy to test. If that all works, then I think this is an acceptable idea. I wouldn't want to eliminate off-line builds. Last question is if we would want to create a local cache to use across CI jobs.

[costinm]

Well - we do need better PR descriptions... I don't know what it's using - it's opaque code. My first thought was that it's going to Jia Tan's server - but according to John it's all local based on some SHA that someone knows what it does. IMO - after binary test blobs, auto generated boilerplate is probably the second least carefully reviewed thing, followed by build infra code.

…

On Thu, May 2, 2024 at 12:43 PM Eric Van Norman ***@***.***> wrote: My first thought reading this was that we will use a local cache, then their servers, and if not available, we can get from another source (although I'm still not sure of the last item). I think that should be easy to test. If that all works, then I think this is an acceptable idea. I wouldn't want to eliminate off-line builds. Last question is if we would want to create a local cache to use across CI jobs. — Reply to this email directly, view it on GitHub <#50587 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2RBWMFMHUEMJDRC643ZAKJN7AVCNFSM6AAAAABGRSBWGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJRGQZDAMBQHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[howardjohn]

While the security risk is probably low/acceptable, TBH I don't really see the benefits here. We are talking about adding a cache, security, availability... what benefits do we get? Seems like little to none

[costinm]

I agree - while we need to start paying more attention to those things, I don't think this PR or using buf is a real problem - or we need even more complexity. If a vendor needs air-gapped builds - and they find it's not so air-gapped - we can fix it. +1 on merging it, but I would still appreciate a more detailed description and confirmation on where do we actually get the generated protos, in case anyone else is curious.

…

On Thu, May 2, 2024 at 7:14 PM John Howard ***@***.***> wrote: While the security risk is probably low/acceptable, TBH I don't really see the benefits here. We are talking about adding a cache, security, availability... what benefits do we get? Seems like little to none — Reply to this email directly, view it on GitHub <#50587 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2USOD6SB4TW4GXXPVDZALXH5AVCNFSM6AAAAABGRSBWGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJSGAZDKNZRGI> . You are receiving this because your review was requested.Message ID: ***@***.***>

[bleggett]

While the security risk is probably low/acceptable, TBH I don't really see the benefits here. We are talking about adding a cache, security, availability... what benefits do we get? Seems like little to none

Only real benefit is it removes ~20k lines worth of vendored protos from our tree (which is good). If we have concerns about caching/availability for CI I am OK holding off on this tho, until we have good answers.

I would like at least testing of/proposed mitigations for any potential CI breakage we could get from this if the buf servers are down, and whether we could safely retain cache across builds or not.

[istio-testing]

@mmorel-35: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
release-notes_istio	7626370	link	true	/test release-notes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[jacob-delgado]

Are we able to reproduce any build if we go down this route? I don't like having checked in protos, but as long as we can have something in the manifest as to which shas from protos we checked in that'd be great.

I use buf often in smaller projects and I enjoy using it compared to using protoc. So overall it's a +1 from me.

[istio-testing]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.