public icon indicating copy to clipboard operation
public copied to clipboard

Add initial OpenConfig model for gRPC Tunnel feature

Open earies opened this issue 2 years ago • 8 comments

  • (A) release/models/system/openconfig-system-grpc-tunnel.yang
    • Add OpenConfig model for gRPC Tunnel feature

References:

  • https://github.com/openconfig/grpctunnel/
  • https://github.com/openconfig/grpctunnel/blob/master/doc/grpctunnel_design.md
  • https://github.com/openconfig/grpctunnel/blob/master/proto/tunnel/tunnel.proto
module: openconfig-system
  +--rw system
     +--rw oc-sys-grpc-tun:grpc-tunnel
        +--rw oc-sys-grpc-tun:config
        |  +--rw oc-sys-grpc-tun:retry-interval?   uint64
        +--ro oc-sys-grpc-tun:state
        |  +--ro oc-sys-grpc-tun:retry-interval?   uint64
        +--rw oc-sys-grpc-tun:servers
        |  +--rw oc-sys-grpc-tun:server* [name]
        |     +--rw oc-sys-grpc-tun:name      -> ../config/name
        |     +--rw oc-sys-grpc-tun:config
        |     |  +--rw oc-sys-grpc-tun:name?                 string
        |     |  +--rw oc-sys-grpc-tun:enable?               boolean
        |     |  +--rw oc-sys-grpc-tun:host?                 oc-inet:host
        |     |  +--rw oc-sys-grpc-tun:port?                 oc-inet:port-number
        |     |  +--rw oc-sys-grpc-tun:transport-security?   boolean
        |     |  +--rw oc-sys-grpc-tun:certificate-id?       string
        |     |  +--rw oc-sys-grpc-tun:source-address?       oc-inet:ip-address
        |     |  +--rw oc-sys-grpc-tun:network-instance?     oc-netinst:network-instance-ref
        |     |  +--rw oc-sys-grpc-tun:dscp?                 oc-inet:dscp
        |     |  +--rw oc-sys-grpc-tun:target*               target-type
        |     +--ro oc-sys-grpc-tun:state
        |        +--ro oc-sys-grpc-tun:name?                 string
        |        +--ro oc-sys-grpc-tun:enable?               boolean
        |        +--ro oc-sys-grpc-tun:host?                 oc-inet:host
        |        +--ro oc-sys-grpc-tun:port?                 oc-inet:port-number
        |        +--ro oc-sys-grpc-tun:transport-security?   boolean
        |        +--ro oc-sys-grpc-tun:certificate-id?       string
        |        +--ro oc-sys-grpc-tun:source-address?       oc-inet:ip-address
        |        +--ro oc-sys-grpc-tun:network-instance?     oc-netinst:network-instance-ref
        |        +--ro oc-sys-grpc-tun:dscp?                 oc-inet:dscp
        |        +--ro oc-sys-grpc-tun:target*               target-type
        +--rw oc-sys-grpc-tun:target-options
           +--rw oc-sys-grpc-tun:config
           |  +--rw oc-sys-grpc-tun:pattern*         enumeration
           |  +--rw oc-sys-grpc-tun:custom-string?   string
           |  +--rw oc-sys-grpc-tun:delimeter?       string
           +--ro oc-sys-grpc-tun:state
              +--ro oc-sys-grpc-tun:pattern*         enumeration
              +--ro oc-sys-grpc-tun:custom-string?   string
              +--ro oc-sys-grpc-tun:delimeter?       string

earies avatar Jul 08 '22 17:07 earies

Compatibility Report for commit fae9ac3b3a62b798acba53272400175c2e927690: ⛔ yanglint@SO 1.10.17

OpenConfigBot avatar Jul 08 '22 17:07 OpenConfigBot

@dplore @robshakir PTAL

earies avatar Jul 11 '22 19:07 earies

@gcsl to review

dplore avatar Jul 12 '22 18:07 dplore

Would a better choice be to integrate into openconfig-telemetry.yang?

gwizdms avatar Jul 12 '22 18:07 gwizdms

@gwizdms - openconfig-telemetry imo is in need of an overhaul to the point where it would likely be deprecated - a PR will be cut soon that will expose more granularity to gRPC services outside of this model that will cover much of the dynamic subscriptions (as well as other RPCs and services such as gNOI, gRIBI, P4RT, etc..). The persistent subscriptions portion of the telemetry model is not precise enough and leads to proprietary implementation today (With varying nodes having never been implemented and supported).

At the same time, grpctunnel is not limited to "telemetry" so I think it would be incorrect to attach it to that domain as it is more so a generic call-home method to transport any inbound TCP services

earies avatar Jul 12 '22 18:07 earies

Ebben, would it be possible to extend target-type with the configurable port? Often, the implemetations allow to specify custom port for servers, like NETCONF, gRPC and etc. In addition, I believe it would be nice to have an option for target-type being custom string. This allows to use grpc-tunnel also for custom services. for instance, telnet is not specified in this proposal....

thanks! miro

mirovr avatar Jul 13 '22 12:07 mirovr

Ebben, would it be possible to extend target-type with the configurable port? Often, the implemetations allow to specify custom port for servers, like NETCONF, gRPC and etc. In addition, I believe it would be nice to have an option for target-type being custom string. This allows to use grpc-tunnel also for custom services. for instance, telnet is not specified in this proposal....

thanks! miro

@mirovr - For your first point, this was a consideration but the issue turns into a bit of unnecessary duplication of specifying the target port (both at the listening TCP service and at this tunnel mapping layer) and maintaining synchronization between the two. In reality the listening service port specification is good enough and the implementation can maintain the dynamic mapping. If a user updates say SSH to listen on tcp/2222 vs. tcp/22 within the SSH service configuration, this would be good enough that the tunnel target parameters need not be updated to match. Essentially this is abstracted away from user-complexity...

For the second - this is a good point since the .proto is a very loose API from this standpoint. The TargetType enum is more or less a placeholder for what should be populated into a string field (https://github.com/openconfig/grpctunnel/blob/master/proto/tunnel/tunnel.proto#L79-L82). For this, I think a middle ground approach would be to keep the "well defined" enum variants as they are but make this a union with a string specifying that the string value be loose and negotiated out of band - what do you think?

earies avatar Jul 13 '22 14:07 earies

@earies - thanks for your response. I understand the reasoning and it seems OK to me. thanks!

miro

mirovr avatar Jul 22 '22 12:07 mirovr