[netconf] Re: Zaheduzzaman Sarker's No Objection on draft-ietf-netconf-http-client-server-23: (with COMMENT)

[Zaheduzzaman Sarker <[e-mail]>]

On Wed, Aug 21, 2024 at 10:06 PM Kent Watsen <[e-mail]> wrote:

> Hi Sarkar,
>
> Thank you for your review!
> More comments below.
>
> Kent // as author
>
>
> On Aug 21, 2024, at 9:57 AM, Zaheduzzaman Sarker via Datatracker <
> [e-mail]> wrote:
>
> Zaheduzzaman Sarker has entered the following ballot position for
> draft-ietf-netconf-http-client-server-23: No Objection
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to
> https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/
> for more information about how to handle DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-netconf-http-client-server/
>
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> Thanks for working on this complex group of specifications.
>
> I have following comments/questions and I believe addressing those will
> improve
> the specification
>
>   - May be I am failing to see the whole picture but I understand that
>   draft-ietf-netconf-tls-client-server defines tls-common module along
> with
>   tls-client and tls-server modules, but I don't see the Section 3.1.2.2
>   mentions tls-common module which is common to both client and server. is
>   this an overlook? if not then, I would expect some description of how
>   tls-common module is used here or not used here. ( I was initially
> thinking
>   of this as discuss worthy but as I am not sure I understand how this
> whole
>   thing is used together I am putting it as comment, and here is your
> chance
>   to educate me :-) )
>
>
> I appreciate the opportunity!  :)
>
> Let’s do this two ways:
>
> 1) Looking at “uses” statements
>
> With the HTML format, looking at Section 3.1.2.2,
> <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-http-client-server-23#section-3.1.2.2> one
> notices that the tree-diagram contains some lines prefixed by “+---u”.   RFC
> 8340, Section 2.2
> <https://www.rfc-editor.org/rfc/rfc8340.html#section-2.2> explains that
> these are unexpanded “grouping” statements.  That is, the YANG module
> itself contains a statement like “uses foo” that appears in the diagram as
> “-u foo”.
>
> This diagram in particular *uses* four distinct grouping statements.
> Immediately under the diagram there is a link to where each of grouping
> statement is discussed.  You asked specifically about the “tls-common”
> module, so let’s look at the "tls-server-grouping” (being the only grouping
> that resolves in the tis-client-server draft).  Under the diagram, there is
> a line that reads "For the referenced grouping statement(s)” and, in this
> section, the third bullet point links Section 4.1.2.1
> <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-tls-client-server-41#section-4.1.2.1> of
> [I-D.ietf-netconf-tls-client-server]
>

> Following that link, we’re now looking that the tree diagram for the
> "tls-server-grouping” grouping.  In this diagram, near the bottom, you will
> find a line that reads "-u tlscmn:hello-params-grouping”.   Please note
> that the “tlscmn” prefix is for the “tls-common” module you ask about.
> The “hello-params-grouping” is the name of a “grouping” statement in the “tls-common”
> module. We’re getting close…
>
> Just below this diagram, in the "For the referenced grouping statement(s)”
> section, the last bullet point links Section 2.1.3.1
> <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-tls-client-server-41#hello-params-grouping> in
> the same document.  Following that link, you will note that this section is
> under Section 2, which is entitled 'The "ietf-tls-common” Module’.  Bingo!
>

>
> 2. Looking at the “import" statements
>
> Whilst we started off by looking at Section 3.1.2.2, which provides an
> overview of the “ietf-http-server” module, Section 3.3
> <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-http-client-server-23#section-3.3> presents
> the YANG module itself.
>
> Note that the first paragraph in this Section 3.3 starts with "This YANG
> module has references…”, with the second-to-last document being
> I-D.ietf-netconf-tls-client-server
> <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-tls-client-server-41>.
> We’ll come back to this in a moment…
>
> Looking at the module, notice that the 5th “import” statement is "import
> ietf-tls-server”.  You may also at this time notice that there is no import
> statement for the “ietf-tls-common” module; that is, this module doesn’t
> import the tls-common module *directly*.
>
> Now, if we were the follow the I-D.ietf-netconf-tls-client-server
> <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-tls-client-server-41> link
> from above, and jump to Section 4.3
> <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-tls-client-server-41#section-4.3>,
> which presents the “ietf-tis-server” YANG module, we see that the 5th
> “import” statement is "import ietf-tls-common”.   That is, this module
> imports “tls-common” *directly*, whilst the “ielf-http-server” module
> imports “tls-common” *indirectly*.
>
>
> In sum, the "ielf-http-server” module indirectly imports the
> "ietf-tls-common” module but, because it is an indirect import, it doesn’t
> appear in the body of the text in the http-client-server draft.   Makes
> sense?
>

Now I know what was my source of confution. I started from the import and
in my read I looked upto the feature section (4.1.1) and got mixed up. Now,
I know I should looked into all the nested imports.

Thanks for this excellent walk through and congratulations on becoming my
mentor ( all my future YANG questions will be directed to you :-)) .


>
>
>
>
>   - Looking at Section 3.1.2.2 the quic-supported tree, I am wondering if
> this
>   how it should be done or not. As TLS 1.3 is ovened into QUIC, QUIC take
>   cares of the TLS record layer, the way tls-server-parameters and
>   tls-server-grouping is used would it be sufficient? may be it does, but I
>   dont understand it as TLS is not used with QUIC as it is used with TCP.
>
>
> This is a fair observation.  I too struggle if it’s best.  In my recent
> interaction with Mark Nottingham, I wrote the three “case” statements out
> differently, something like this:
>
> tcp   -->  http-over-tcp         // HTTP/1.1 or HTTP/2
> tls   -->  http-over-tls-over-tcp  // HTTP/1.1 or HTTP/2
> quic  -->  http-over-quic-over-udp // currently just HTTP/3
>
> These labels also don’t sit well with me, because they don’t speak to
> HTTP-versions (e.g., HTTP/1.1, HTTP/2, HTTP/3).   I feel like there is a
> level of indirection missing…
>
> For instance, now that we’re familiar with tree-diagrams (right?), it
> seems more accurate for the three “case” statements to actually be
> “grouping” statements that could then be rolled into new “case” statements
> as follows:
>
>   grouping http-server-stack-grouping:
>     +-- (http-version)
>        +--:(http11) {http11-supported}?
>        |  +-- http11
>        |     +-- (transport)
>        |        +-:(tcp) {tcp-supported}?
>        |        |  +-- tcp
>        |        |     +--u http-over-tcp-grouping
>        |        +-:(tls) {tls-supported}?
>        |           +-- tls
>        |              +--u http-over-tls-over-tcp-grouping
>        +--:(http2) {http2-supported}?
>        |     +-- (transport)
>        |        +-:(tcp) {tcp-supported}?
>        |        |  +-- tcp
>        |        |     +--u http-over-tcp-grouping
>        |        +-:(tls) {tls-supported}?
>        |           +-- tls
>        |              +--u http-over-tls-over-tcp-grouping
>        +--:(http3) {http3-supported}?
>               +-- (transport)
>                 +-:(quic) {quic-supported}?
>                    +-- quic
>                       +--u http-over-quic-over-udp-grouping
>
>
> This seems more accurate, but still doesn’t sit well with me.  For
> instance, when configuring an NGINX server (i.e., nginx.conf), there are
> “server” stanzas that each have “listen” statements, which suggests
> something more like this:
>
>   grouping http-server-app-grouping:  <— note: this is an “app” grouping
> (not a “stack” grouping)
>     +-- server* [server-name]
>       +-- server-name  string
>       +-- listen* [transport port]
>       |   +-- transport  enum w/ values “tcp” and “udp”
>       |   +-- port       uint16
>       |   +-- http-version  enum w/ values http11, http2, and http3
>       |   +-- tcp-server-parameters  { if ../transport = ’tcp’ }
>       |   |    +—u tcps:tcp-server-grouping
>       |   +-- udp-server-parameters  { if ../transport = ’udp’ }
>       |        +—u udps:udp-server-grouping
>       +-- tls-server-parameters
>       +   +--u tlss:tls-server-grouping
>       +-- client-authentication {client-auth-supported}
>           +-- ...
>
>
This is becoming interesting. It would all depend on the purpose of this
grouping, right? if this is about cofiguring YANG clients and servers then
you would like to provide as much details at you like even mentioning the
HTTP versions and port numbers to run the servers on ... but it this is
about dictating client behaviours then there could be issues with HTTP
versions as there are different ways clients picks http versions AFAIK via
negotiation or hardcoded.

When you described the last variant of potential groupping, I could see
that one can fold the QUIC support into UDP server grouping with specific
port number (its 443 for the web). But I am not an YANG expert and I will
let you experts decide on what would be the best way to resolve it.


>
> Anyway, I think you get the idea.  I feel bad this is coming out in an
> IESG review...
>
> PS: no big deal if this takes awhile to resolve, since this draft has a
> normative reference to the “udp-client-server” draft, which is only just
> now about to enter WGLC (in the NETCONF WG).   Point being is that this
> draft wouldn’t necessarily get published any faster...
>

Yeah, it would have been better to catch this earlier in the document
process, but looking into positive side , this is also a indication that
the review process is somewhat working :-). Its great that it got flagged
and we are addressing it.


>
>
>   - proxy-connect feature only refers to CONNECT method of RFC9110, but
> what
>   about the CONNECT-UDP (RFC9298) that http clients can use to connect via
>   proxy? Is that considered here but explicitly put out of scope? I can see
>   HTTP/2 and HTTP/3 clients can use CONNECT-UDP method which is derived
> from
>   CONNECT method.
>
>
> Grrr, this is an oversight.  My lame excuse is that the support for HTTP/3
> was added long after the proxy-support was added, and no one thought to add
> it...
>

OK, then I expect that this will be addressed in new version.

>
>
> Your review was excellent in sussing out some long-forgotten items!
>

Thanks, happy to be useful.

//Zahed


>
> Thanks,
> Kent
>
>
>
>
>
>
>
>
>
>