Skip to content

Make NAT translation reporting generic#1467

Open
bewing wants to merge 3 commits intoopenconfig:masterfrom
bewing:feat/nat-remote-tuple
Open

Make NAT translation reporting generic#1467
bewing wants to merge 3 commits intoopenconfig:masterfrom
bewing:feat/nat-remote-tuple

Conversation

@bewing
Copy link
Copy Markdown

@bewing bewing commented Apr 2, 2026

Change Scope

  • Rewrite nat-translation-entry-state to cover all use cases and convey more information
  • This is incompatible with the initial release of the NAT model

Platform Implementations

Tree View

module: openconfig-nat
  +--rw nat
     +--rw instances
        +--rw instance* [name]
            +--ro translations
               +--ro translation* [translation-id]
                  +--ro translation-id    -> ../state/translation-id
                  +--ro state
                     +--ro translation-id?     uint64
-                    +--ro internal-address?   inet:ip-address
-                    +--ro internal-port?      inet:port-number
-                    +--ro external-address?   inet:ip-address
-                    +--ro external-port?      inet:port-number
                     +--ro protocol?           protocol-type
+                    +--ro nat-type?         nat-translation-type
+                    +--ro interface?        oc-if:base-interface-ref
+                    +--ro original
+                    |  +--ro src-address?   inet:ip-address
+                    |  +--ro src-port?      inet:port-number
+                    |  +--ro dst-address?   inet:ip-address
+                    |  +--ro dst-port?      inet:port-number
+                    +--ro translated
+                    |  +--ro src-address?   inet:ip-address
+                    |  +--ro src-port?      inet:port-number
+                    |  +--ro dst-address?   inet:ip-address
+                    |  +--ro dst-port?      inet:port-number
                     +--ro creation-time?      yang:date-and-time
                     +--ro last-activity?      yang:date-and-time
                     +--ro timeout?            uint32
-                    +--ro source-pool?        string
-                    +--ro source-mapping?     string
+                    +--ro nat-pool?         string
+                    +--ro nat-mapping?      string
                     +--ro counters
-                       +--ro packet-count-inbound?    yang:counter64
-                       +--ro packet-count-outbound?   yang:counter64
-                       +--ro byte-count-inbound?      yang:counter64
-                       +--ro byte-count-outbound?     yang:counter64
+                       +--ro original-direction
+                       |  +--ro packets?   yang:counter64
+                       |  +--ro bytes?     yang:counter64
+                       +--ro reply-direction
+                          +--ro packets?   yang:counter64
+                          +--ro bytes?     yang:counter64

@bewing bewing requested a review from a team as a code owner April 2, 2026 18:59
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the NAT operational state model in openconfig-nat.yang by replacing the flat internal/external address structure with a more flexible 5-tuple representation (original and translated) to support SNAT, DNAT, and double NAT. It also renames pool and mapping leaves and restructures counters to be direction-based. Feedback indicates that these breaking changes require a version bump and revision history update. Additionally, suggestions were made to add a NONE or PASSTHROUGH enum to nat-translation-type and to use a less restrictive interface reference type to support subinterfaces.

Comment on lines +680 to +681
leaf interface {
type oc-if:base-interface-ref;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The interface leaf uses oc-if:base-interface-ref, which only allows referencing a top-level interface. Since NAT is frequently applied to subinterfaces (where L3 configuration resides in OpenConfig), this type may be too restrictive. Consider using a type that allows referencing subinterfaces (like a simple string or a more complex interface-ref structure), or clarify if only base interfaces are intended to be reported here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

oc-if:base-interface-ref is used in several other places already in the model - should I update all instances of it to use oc-if:interface-ref-common ?

@bewing
Copy link
Copy Markdown
Author

bewing commented Apr 2, 2026

Does the OC team prefer force pushes in response to feedback, or incremental commits for each discussion item?

@dplore
Copy link
Copy Markdown
Member

dplore commented Apr 7, 2026

@nleiva would you like to review?

@dplore
Copy link
Copy Markdown
Member

dplore commented Apr 7, 2026

/gcbrun

@OpenConfigBot
Copy link
Copy Markdown

No major YANG version changes in commit 31c7a62

@nleiva
Copy link
Copy Markdown
Contributor

nleiva commented Apr 7, 2026

@nleiva would you like to review?

Of course, give me a week or two. Got this and SNMP on my TODO list now.

@rolandphung
Copy link
Copy Markdown
Contributor

@dplore Just looking at this in a process perspective, I noticed that this is marked automatically as non-breaking, while the model change is clearly breaking. Digging a bit further I noticed that the entire nat model is not referenced anywhere in the documentation (specifically https://openconfig.net/projects/models/paths/).

My question is that are we still considering this as unpublished so we can make backwards incompatible changes? Is that why non-breaking is tagged here?

If unpublished, when are we going to add /nat root? If not, should this be considered breaking?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants