Skip to content
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.

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

Add gNOI Packet Capture service #67

Merged
merged 238 commits into from
Jul 11, 2023
Merged

Add gNOI Packet Capture service #67

merged 238 commits into from
Jul 11, 2023

Conversation

Raj998
Copy link
Contributor

@Raj998 Raj998 commented Apr 20, 2022

  • Added proto and generated go files for packet capture service.

References

  • References to several implementations are in the gNOI PCAP proposal
  • As this change is a new proto, the change is backwards compatible to gNOI

@google-cla
Copy link

google-cla bot commented Apr 20, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@earies
Copy link

earies commented Apr 20, 2022

I'm curious - Do you have a specification or architectural document describing the full intention of these APIs (or planning to publish)?

Is this intended for pcap on network-elements and thus limited to punted/non-transit traffic?
Is this meant to correspond with user-space utilities such as tcpdump? (assume so since there is reference to an opaque string filter expression)
How come the limitation of TCP/UDP L4 protocols?
The loose relationship between filter_name and supported schemas on a target assumes some level of support on shipping implementations? Do you have any such references?
What are the characteristics for response stream (much like definitions in file.proto) as well as how the data field is to be encoded?
Generally, implementations that support some level of pcap, have the ability to write to stdout/filesystem which should likely be accounted for. Streaming captures remotely could very well exacerbate issues under certain conditions and thus believe that there should be surrounding specifications and guidelines outlined as to the full usage of this API

@Raj998
Copy link
Contributor Author

Raj998 commented May 4, 2022

Addressing your questions @earies -

I'm curious - Do you have a specification or architectural document describing the full intention of these APIs (or planning to publish)?
-> Yes, you can find the link to the public doc below -
https://docs.google.com/document/d/1bwvTzbmJAsc7YSfhCpY_mDzHwWfaC7DISD-Xw_0n7KI/edit

Is this intended for pcap on network-elements and thus limited to punted/non-transit traffic?
-> No, it would not be limited to punted traffic

How come the limitation of TCP/UDP L4 protocols?
-> Yes, we can add other protocols like DHCP, DNS, ARP, ICMP, RADIUS etc.

Generally, implementations that support some level of pcap, have the ability to write to stdout/filesystem which should likely be accounted for -
-> We had initially thought of it but later decided to remove it, to keep the functionality focused on the streaming response. Anyway we can handle this in a future PR.

@xavier-contreras
Copy link

@earies , does the document and previous answer help with some of your questions?

For the characteristics of the data field, I'm not sure what the expectation should be. Looking at a generic stream bytes API (with streaming response), I do not see a lot of details: https://github.com/googleapis/googleapis/blob/master/google/bytestream/bytestream.proto#L53

Unless otherwise documented, would it be reasonable for defaults to apply?
https://developers.google.com/protocol-buffers/docs/proto3#scalar

@xavier-contreras
Copy link

This looks good to me; It's a good baseline to start from and can be expanded/enhanced in a future PRs

@dplore
Copy link
Member

dplore commented Jun 2, 2022

@Raj998 you will need to sign the Google CLA before we can accept this contribution

mhines01 and others added 24 commits June 6, 2022 16:46
Update build files
Update gnoi.Path to compliant with gnmi.Path
Update simple test to use new path
…iles Update gnoi.Path to compliant with gnmi.Path Update simple test to use new path
…iles Update gnoi.Path to compliant with gnmi.Path Update simple test to use new path
 * Remove references to the openconfig/reference repo for gNOI
   types import in both protobufs and the Bazel files.
 * Regenerate protobufs using protoc v3.5.1.
Raj998 added 20 commits June 6, 2022 17:00
This reverts commit 6dc955b.
This reverts commit dff27d2.
…g api Update Put to fix duplicate and typo"

This reverts commit adcb256.
…g api Update Put to fix duplicate and typo"

This reverts commit 5541563.
…g api Update Put to fix duplicate and typo"

This reverts commit bd2c158.
This reverts commit 61567e5.
… build files Update gnoi.Path to compliant with gnmi.Path Update simple test to use new path"

This reverts commit 95a7614.
… build files Update gnoi.Path to compliant with gnmi.Path Update simple test to use new path"

This reverts commit 79c5504.
@Raj998
Copy link
Contributor Author

Raj998 commented Jun 6, 2022

Some of the initial commits were done by different email address because of which CLA check was failing and accidentally I did a " git rebase -i --root " instead of "git rebase -i <earlier_commit_hash>" to change author for the earlier commits which lead to this mess. Extremely sorry for the inconvenience caused.

Copy link

@xavier-contreras xavier-contreras left a comment

Choose a reason for hiding this comment

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

This looks good to me, we can build off of this.

@WFDU-HPE
Copy link

Aruba is ok with this

Copy link

@xavier-contreras xavier-contreras left a comment

Choose a reason for hiding this comment

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

This service has been reviewed and approved by two vendors (Arista, Aruba) - and was a collaboration with a network operator (Google). This is ready for review and merge.

Copy link

@mike-albano mike-albano left a comment

Choose a reason for hiding this comment

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

LGTM, nice work on the collaborative approach.

@dplore
Copy link
Member

dplore commented Jul 11, 2023

/gcbrun

@dplore dplore merged commit 6884aef into openconfig:main Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.