-
Notifications
You must be signed in to change notification settings - Fork 16
Ipv6 routing header merge #442
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
base: master
Are you sure you want to change the base?
Conversation
flow/packet-headers/header.yaml
Outdated
| macsec: | ||
| $ref: './macsec.yaml#/components/schemas/Flow.Macsec' | ||
| x-field-uid: 23 | ||
| ipv6_routing_extension: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks incorrect way to approach this. We should be probably having an array of ipv6 extension headers as discussed with choice of routing as one option ( only one as of now )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, among of the following Ipv6 header extensions. model is to support the 4th.
Hop-by-Hop Options
Fragment
Destination Options
Routing
Authentication
Encapsulating Security Payload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated based on discussion.
| x-field-pattern: | ||
| description: 8-bit selector that identifies the type of header immediately | ||
| following the SRH. It uses the same values as the IPv4 Protocol field. | ||
| The default value 59 indicates that there is no next header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably at least provide a reference to iana/ietf specification for list of values and mention udp and tcp values since that would probably be a likely use-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description updated with tcp / udp values and iana reference.
| format: integer | ||
| length: 8 | ||
| default: 59 | ||
| x-constants: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention at least tcp and udp from below in documentation. This is not otherwise visible to the user at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should provide link to IANA provided in previous comment and also mention that 59 means no next header, indicating that after this extension header, the payload is directly appended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
| sctp: 132 | ||
| features: [auto, count] | ||
| x-field-uid: 1 | ||
| hdr_ext_len: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be at a higher level extension header object not repeated in each extension header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in some cases absence of auto makes this very hard to use . Also , increment/decrement etc. might not have much meaning for this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added auto and restructured model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still not part of extension header as it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it one level up.
| 8-octet units, not including the first 8 octets of the SRH itself. | ||
| format: integer | ||
| length: 8 | ||
| default: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope 2 is correct with default settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default is now 0.
| description: Variable-length padding bytes. Must be set to 0 on transmission. | ||
| type: string | ||
| format: hex | ||
| default: '00' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not align with default length above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Length is common across all TLV types with auto support. Length will be updated based on TLV data.
| 0 to 5. | ||
| format: integer | ||
| length: 8 | ||
| default: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideal default should be byte padded length of pattern below automatically . Only specify and set for negative testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLV type and length can be auto now.
| description: The HMAC digest, truncated to 32 octets. Its length must | ||
| be a multiple of 8 octets. | ||
| format: mac | ||
| type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very unclear to me why format is mac if length is variable. How will user set this correctly. Even if not now, how would we generate correct hmac value automatically via model if some impl. supports that. It looks very difficult to set this correctly from a script . Also, need to think when packets vary and hmac might vary,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed hmac tlv as its not supported in IxN also.
| description: The variable-length opaque data carried by the TLV. | ||
| format: hex | ||
| type: string | ||
| default: '00' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default length and this dont match I think .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Length is common across all TLV types with auto support. Length will be updated based on TLV data.
| $ref: '#/components/schemas/Flow.Ipv6SegmentRouting.Segment' | ||
| x-field-uid: 8 | ||
| tlvs: | ||
| $ref: '#/components/schemas/Flow.Ipv6SegmentRouting.Tlvs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be an array ? How to have just 1 TLV and not the rest . Can some of these TLVs come multiple times esp. path tracing tlv . This somehow seems to be on the wrong track to me,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be arrays Tlv objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made TLVs as array.
| default: segment_routing | ||
| x-field-uid: 1 | ||
| x-enum: | ||
| segment_routing: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we can give name as ipv6_routing (as per rfc: its routing) base choice value . it will be one of the values.
Hop-by-Hop Options
Fragment
Destination Options
Routing
Authentication
Encapsulating Security Payload.
As per rfc, if one or more headers the order is like this. that probably we can document this in future addition.
IPv6 header
Hop-by-Hop Options header
Destination Options header (note 1)
Routing header
Fragment header
Authentication header (note 2)
Encapsulating Security Payload header (note 2)
Destination Options header (note 3)
Upper-Layer header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
| features: | ||
| - count | ||
| x-field-uid: 1 | ||
| p_flag: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, go snappi code will good if change the properly.
Flags().SetProtected(true)
Flags().SetOAM(true)
Flags().SetAlert(true) etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have updated the Flags section, removed unused flags and renamed remaining ones.
| $ref: '#/components/schemas/Flow.Ipv6SegmentRouting.Segment' | ||
| x-field-uid: 8 | ||
| tlvs: | ||
| $ref: '#/components/schemas/Flow.Ipv6SegmentRouting.Tlvs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be arrays Tlv objects
| format: hex | ||
| default: '00' | ||
| x-field-uid: 3 | ||
| Flow.Ipv6SegmentRouting.TlvHmac: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If DUT tries to verify this feild should be generated by properly by means of what RFC says. So, how we are giving the input of pre-shared key, whether traffic will generate the HMAC accordingly?
key: The pre-shared key identified by HMAC Key ID
HMAC algorithm: Identified by the HMAC Key ID
Text: A concatenation of the following fields from the IPv6 header and the SRH, as it would be received at the node verifying the HMAC:
IPv6 header: Source address (16 octets)
SRH: Last Entry (1 octet)
SRH: Flags (1 octet)
SRH: HMAC 16 bits following Length
SRH: HMAC Key ID (4 octets)
SRH: All addresses in the Segment List (variable octets)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HMAC TLV is removed from model.
apratimmukherjee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still have some structural changes TBD and some comments to improve documentation/align attribute names.
flow/packet-headers/header.yaml
Outdated
| x-field-uid: 23 | ||
| ipv6_extension_headers: | ||
| description: Ipv6 extension packet headers. For routing extension headers | ||
| the IPv6 next header is 43. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should mention that ipv6_extension_header should be preceded by ipv6_header or other ipv6_extension_header(s) and the next_header field in the preceding header should be set to the correct value depending on the choice of extension header configured. This should be done automatically by the implementation if the auto choice is selected for the next_header field in the preceding header.
For Routing Extension header , this value is 43. The available defined values are listed in https://www.iana.org/assignments/ipv6-parameters/ipv6-parameters.xhtml#extension-header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated description.
| components: | ||
| schemas: | ||
| Flow.Ipv6ExtHeaders: | ||
| description: Ipv6 extension packet headers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ipv6 -> IPv6 throughout inside description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| x-enum: | ||
| segment_routing: | ||
| x-field-uid: 1 | ||
| next_header: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be attribute of common extension header, not segment_routing sub-choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move it up in hierarchy.
| format: integer | ||
| length: 8 | ||
| default: 59 | ||
| x-constants: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should provide link to IANA provided in previous comment and also mention that 59 means no next header, indicating that after this extension header, the payload is directly appended.
| sctp: 132 | ||
| features: [auto, count] | ||
| x-field-uid: 1 | ||
| hdr_ext_len: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still not part of extension header as it should be.
| x-field-uid: 4 | ||
| path_trace: | ||
| x-field-uid: 5 | ||
| type: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might remove this for now , basically any user defined arbitrary value makes it custom tlv and other attributes become meaningless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed type field.
| x-field-uid: 2 | ||
| length: | ||
| x-field-pattern: | ||
| description: The length of the TLV value in bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If auto is selected , the implementation should automatically set the length field to the correct length as per the selected TLV and attributes,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| features: | ||
| - count | ||
| x-field-uid: 5 | ||
| u2_flag: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better maybe just u_flag or unused_flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed all flag fields.
| default: '00' | ||
| x-field-uid: 1 | ||
| Flow.Ipv6SegmentRouting.TlvIngressNode: | ||
| description: A TLV that may be used to carry the IPv6 address of the ingress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference to RFC esp. to Type value and calling it out specifically would be useful for all the TLVs in the model here,
| features: | ||
| - count | ||
| x-field-uid: 2 | ||
| value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
egress_ipv6_ip ?
flow/packet-headers/header.yaml
Outdated
| the IPv6 next header is 43. | ||
| type: array | ||
| items: | ||
| $ref: './ipv6_routing.yaml#/components/schemas/Flow.Ipv6ExtHeaders' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object name could be Flow.Ipv6ExtHeader as this is an array of object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
| tlvs: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/Flow.Ipv6SegmentRouting.Tlvs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel class name could be Flow.Ipv6SegmentRouting.Tlv no s.
Note: tlvs is absolutely fine. So, no need to change the property name.
| lacp: | ||
| $ref: './lacp.yaml#/components/schemas/Flow.Lacp' | ||
| x-field-uid: 24 | ||
| ipv6_extension_header: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this would be ipv6_extension_headers as it contain array of Flow.Ipv6ExtHeader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed array and retained header as discussed.
| default: 0 | ||
| features: [auto, count] | ||
| x-field-uid: 2 | ||
| ingress: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will better to defined TLV Type within the description. This comment for all TLV types.
| x-field-uid: 2 | ||
| timestamp: | ||
| description: Timestamp indicating when the measurement was taken. | ||
| format: hex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not support other pattern. Better to use Hex which support pattern (values,, increment, ..)
IPv6 Segment Routing Header Support
Redocly view of proposed model
Proposal is for IPv6 segment routing (Type 4) header, This is the first of the IPv6 routing extension header support in OTG model.
The model is made generic to accommodate other IPv6 extension header types, hop-by-hop or fragment. Currently we will support routing extensions only. In routing also we will currently support Type 4 which is segment routing.
Note: The IPv6 next_header if set to AUTO, will be updated to reflect the correct next header type, which is 43 for IPv6 route.
RFC for reference