Skip to content

Gen yaml annotations for flow proto #28944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Gen yaml annotations for flow proto #28944

wants to merge 2 commits into from

Conversation

marqc
Copy link
Contributor

@marqc marqc commented Nov 2, 2023

Please ensure your pull request adheres to the following guidelines:

  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.

Add yaml annotations to generated flow proto as discussed in #28873 (comment)
Requires: #28943

cc: @chancez @glibsm

Add yaml annotations to generated flow proto.

@marqc marqc requested review from a team as code owners November 2, 2023 12:18
@marqc marqc requested review from gandro and brb November 2, 2023 12:18
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 2, 2023
@marqc marqc had a problem deploying to release-base-images November 2, 2023 12:18 — with GitHub Actions Error
@marqc marqc mentioned this pull request Nov 2, 2023
Merged
8 tasks
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 2, 2023
@marqc marqc force-pushed the gen_yaml_annotations_for_flow_proto branch from 3195d80 to 3cc217e Compare November 2, 2023 12:59
@gandro gandro added release-note/misc This PR makes changes that have no direct user impact. sig/hubble dont-merge/blocked Another PR must be merged before this one. labels Nov 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 2, 2023
gandro
gandro requested changes Nov 2, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks. The overall idea looks good to me, but I'm not a big fan of deleting whole folders during the build.

marqc
marqc commented Nov 2, 2023
View reviewed changes
@marqc marqc force-pushed the gen_yaml_annotations_for_flow_proto branch from 3cc217e to bd894ff Compare November 2, 2023 13:39
@marqc marqc requested a review from gandro November 2, 2023 13:42
gandro
gandro reviewed Nov 2, 2023
View reviewed changes
@marqc marqc force-pushed the gen_yaml_annotations_for_flow_proto branch from bd894ff to f122df4 Compare November 2, 2023 14:41
@marqc marqc had a problem deploying to release-base-images November 2, 2023 14:41 — with GitHub Actions Error
@marqc marqc requested a review from gandro November 2, 2023 14:43
glibsm
glibsm reviewed Nov 2, 2023
View reviewed changes
@chancez
Copy link
Contributor

chancez commented Nov 2, 2023

LGTM. I'll approve after #28943 is merged and the base image + CI passes.

@marqc marqc force-pushed the gen_yaml_annotations_for_flow_proto branch from f122df4 to dec100a Compare November 2, 2023 17:52
@marqc marqc requested review from a team as code owners November 2, 2023 17:52
@marqc marqc requested review from christarazi and bimmlerd November 2, 2023 17:52
@marqc marqc temporarily deployed to release-base-images November 2, 2023 21:25 — with GitHub Actions Inactive
@marqc
Copy link
Contributor Author

marqc commented Nov 2, 2023

Can you add justification in the commit msgs for why the changes are needed?

@christarazi Added.

bimmlerd
bimmlerd approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

Fine by me, but will unfortunately conflict with #28945. Given both are currently blocked by the Cloudflare outage, let's just see which one gets in first and rebase the other one.

@marqc marqc force-pushed the gen_yaml_annotations_for_flow_proto branch from 066a40f to 10c7e03 Compare November 6, 2023 21:38
@marqc marqc had a problem deploying to release-base-images November 6, 2023 21:38 — with GitHub Actions Failure
@marqc marqc force-pushed the gen_yaml_annotations_for_flow_proto branch from 10c7e03 to c0c12a0 Compare November 7, 2023 08:23
@marqc marqc had a problem deploying to release-base-images November 7, 2023 08:23 — with GitHub Actions Error
@marqc
Copy link
Contributor Author

marqc commented Nov 7, 2023

Fine by me, but will unfortunately conflict with #28945. Given both are currently blocked by the Cloudflare outage, let's just see which one gets in first and rebase the other one.

Rebased.

@gandro gandro removed the request for review from brb November 7, 2023 10:01
@gandro
Copy link
Member

gandro commented Nov 7, 2023 •
edited
Loading

@marqc Can you rebase again? Unfortunately main was broken due to a merge race (c.f. #28745) and you seem to have pulled in the broken commit. Thanks!

@marqc marqc force-pushed the gen_yaml_annotations_for_flow_proto branch from c0c12a0 to 64a4a3b Compare November 7, 2023 14:15
@marqc marqc temporarily deployed to release-base-images November 7, 2023 14:15 — with GitHub Actions Inactive
@gandro
Copy link
Member

gandro commented Nov 7, 2023

/test

@gandro gandro removed the dont-merge/blocked Another PR must be merged before this one. label Nov 7, 2023
@marqc marqc force-pushed the gen_yaml_annotations_for_flow_proto branch from 64a4a3b to 98ad6ff Compare November 7, 2023 22:28
@marqc marqc temporarily deployed to release-base-images November 7, 2023 22:28 — with GitHub Actions Inactive
@gandro
Copy link
Member

gandro commented Nov 8, 2023

/test

@squeed
Copy link
Contributor

squeed commented Nov 8, 2023

Travis took a nap :-(. If this PR doesn't need any more pushes, then a close-and-reopen will do the trick.

@marqc marqc requested a review from glibsm November 9, 2023 12:19
@gandro gandro added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Nov 13, 2023
marqc added 2 commits November 15, 2023 11:26
@marqc
Add protoc-gen-gotag plugin to builder image.
7215fa2 
Plugin will be used to generate yaml annotation in structs generated
from proto to be able to unmarshall FlowFilter from YAML for cilium#25508.

Signed-off-by: Marek Chodor <mchodor@google.com>
@marqc
Add yaml annotations to flow proto gen files.
b518051 
Yaml annotations are needed to unmarshall FlowFilter structs for flow
logging feature cilium#25508.

Signed-off-by: Marek Chodor <mchodor@google.com>
@marqc marqc force-pushed the gen_yaml_annotations_for_flow_proto branch from 98ad6ff to b518051 Compare November 15, 2023 11:34
@marqc marqc temporarily deployed to release-base-images November 15, 2023 11:34 — with GitHub Actions Inactive
@gandro
Copy link
Member

gandro commented Nov 15, 2023 •
edited
Loading

/test

@marqc
Copy link
Contributor Author

marqc commented Nov 16, 2023

I just realized that names generated with this plugin are inconsitent with json tags. This would make a mess in the API. I will close this and instead I will use different YAML parser that works with json tags sigs.k8s.io/yaml

@marqc marqc closed this Nov 16, 2023
skmatti pushed a commit to skmatti/cilium that referenced this pull request Jul 24, 2024
@marqc @yasz24
Introduce dynamic hubble flow logs exporters.
89626df 
Add protoc-gen-gotag plugin to builder image.
Add yaml annotations to flow proto gen files.
Introduce dynamic hubble flow logs exporters based on config file.

Signed-off-by: Marek Chodor <mchodor@google.com>
Carry-Patch-Approval: b/309936968
Upstream-PR: cilium#28873
Upstream-PR: cilium#28943
Upstream-PR: cilium#28944
Change-Id: I7871d8b119822c3228a6e959b86dd1b6b23d275a
Reviewed-on: https://gke-internal-review.googlesource.com/c/third_party/cilium/+/860928
Unit-Verified: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Reviewed-by: Akhil Velagapudi <avelagap@google.com>
Reviewed-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Tested-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
skmatti pushed a commit to skmatti/cilium that referenced this pull request Jul 24, 2024
@marqc @yasz24
Replace yaml parser used by dynamic hubble exporter.
cd0aabf 
The new parser utilizes json tags to keep API consistent.
This also reverts cherry-pick of
cilium#28944 as this is no longer needed.

Carry-Patch-Approval: b/309936968
Upstream-PR: cilium#28873
Change-Id: I8d44e7f937fb56f3c5c6c4ed943586d39a4ea17b
Reviewed-on: https://gke-internal-review.googlesource.com/c/third_party/cilium/+/867331
Tested-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Unit-Verified: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Reviewed-by: Akhil Velagapudi <avelagap@google.com>
Reviewed-by: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com>
Prow-Auto-Submit: Marek Chodor <mchodor@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@christarazi christarazi christarazi approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@glibsm glibsm Awaiting requested review from glibsm

Assignees
No one assigned
Labels
dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. kind/community-contribution This was a contribution made by a community member. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@marqc @chancez @gandro @squeed @glibsm @christarazi @bimmlerd