Opened 2 years ago

Closed 14 months ago

Last modified 11 months ago

#5668 enhancement closed fixed (fixed)

Support the OPT pseudo-record

Reported by: exarkun Owned by: rwall
Priority: normal Milestone: EDNS0
Component: names Keywords: edns
Cc: Branch: branches/opt-record-5668-4
(diff, github, buildbot, log)
Author: exarkun, rwall Launchpad Bug:

Description

RFC 2671 defines a new record type, OPT (value 41). This record doesn't carry information about a domain name. Instead, it carries extended information about the format of the request or response message.

We should be able to decode these into a structured object and serialize them back into a message.

See EDNS0 for some notes on the big picture.

Attachments (1)

opt-record-5668-examples.patch (30.7 KB) - added by rwall 14 months ago.
Some examples of _OPTHeader usage.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/opt-record-5668

(In [34367]) Branching to 'opt-record-5668'

comment:2 Changed 2 years ago by exarkun

(In [34368]) Apply just the parts of the #5454 patch which add the OPT record type

refs #5668

comment:3 Changed 2 years ago by exarkun

Next thing to do, I think, is figure out why OPTHeader exists, and why RRHeader is changed to subclass it.

comment:4 Changed 2 years ago by BobNovas

Jean-Paul - In the 5454 patch, OPTHeader exists because Record_OPT uses or overloads some of the fields of what would be RRHeader. So OPTHeader is just the header fields of a Record_OPT and so OPTHeader is a subset of RRHeader in terms of fields (and OPTHeader is also a subclass of RRHeader).

OPTHeader._headerFactory decodes just enough of a stream to decide if it should create an OPTHeader or an RRHeader and then either creates an OPTHeader or rewinds the stream and creates an RRHeader and returns one or the other. So, Message.parseRecords calls OPTHeader._headerFactory and gets the right Header (OPTHeader or RRHeader, which is a subclass of OPT), and then decodes the rest of the record from the type of the record, which is in OPTHeader, hence also in RRHeader.

Bad idea? Sorry.

comment:5 Changed 2 years ago by exarkun

Thanks for the explanation, it helps a lot. On a closer reading of the EDNS RFC, now I wonder if it wouldn't be better to just have OPTHeader and forget about Record_OPT. Not sure yet, just the direction my thoughts are going right now.

comment:6 Changed 17 months ago by rwall

  • Author changed from exarkun to exarkun, rwall
  • Branch changed from branches/opt-record-5668 to branches/opt-record-5668-2

(In [38420]) Branching to 'opt-record-5668-2'

comment:7 Changed 17 months ago by rwall

  • Keywords review added

Ready for review in log:branches/opt-record-5668-2

  • This isn't finished but I'd like some feedback on what I've done and how much further to go in this ticket.
  • I've been concentrating on OPTHeader
  • So far I've implemented all the fixed fields of the OPT RR described here: http://tools.ietf.org/html/rfc6891#section-6.1.2
  • I need to decide whether to bother supporting the EDNS options tuples
  • There don't seem to be (m)any real world uses for it yet - only proposals (http://www.iana.org/assignments/dns-parameters/dns-parameters.xml#dns-parameters-11)
  • I've left BobNovas original Record_OPT class in place for now, but it could become the container and parser for the Option tuples if we choose to support them.
  • I've made some initial modifications to dns.Message to allow decoding OPT RRs but this part could be moved to a separate ticket where we introduce a client API for issuing EDNS queries.

Build Results:

-RichardW.

comment:8 Changed 16 months ago by tom.prince

  • Owner set to tom.prince

comment:9 Changed 16 months ago by tom.prince

  • Keywords review removed
  • Owner changed from tom.prince to rwall

So, a question I have: are the data structures meant to encode messages syntactically or semantically? I think that the answer to this whether we have something like OPTHeader or Record_OPT. If we view things syntactically, then OPT is a record that can appear in messages (theoretically anywhere, but only in the additional data in a conforming implementation). If we view things semantically, then OPT is just additional data of the message, that just happens to look like a record. If I understand EDNS? correctly, it seems this ticket is about the former representation.

  1. If this is about implementing a syntactic view, then does it make sense to disallow OPT in other sections?
  2. After further perusal of the RFCs, I realize that OPT interprets the RR header differently. I wonder if it would make sense to
    1. Probe for the type before decoding.
    2. or extract the data for OPTHeader from the existing instance of RRHeader.

Some (incomplete) notes about the code itself:

  1. OPTHeader:
    1. Is there any reason for OPTHeader.fmt to be public? I realize that this is public on a bunch of thins in t.n.dns, but it seems like an implementation detail.
    2. rdlen? Then name is unclear, and I'm not sure it is useful, in any case.
    3. Is there a reason this doesn't use FancyStrMixin?
    4. self.name.decode is modifying a class attribute. :(
      • test_optHeaderName and test_optHeaderType say that the attributes are readonly, which isn't true. This is misleading.
      • Malformed packets might contain different values for name.
  2. Record_OPT
    1. Don't use assert (see #6560).
    2. encode doesn't use self.fmt for some reason.

This isn't a complete review, but the branch seems in progress, and I think there are some questions about direction and intent that need to be answered before a more thorough review.

As an aside, it seems like it would have made more sense if .decode was a classmethod.

comment:10 Changed 16 months ago by rwall

  • Branch changed from branches/opt-record-5668-2 to branches/opt-record-5668-3

(In [38749]) Branching to 'opt-record-5668-3'

comment:11 Changed 16 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Ready for another review in log:branches/opt-record-5668-3

  1. The individual changes since the last review can be seen in log:branches/opt-record-5668-2
  2. Build Results:
    1. http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/opt-record-5668-3
    2. I don't understand why there are so many new pydoctor errors, but they look unrelated to my branch.
  3. I removed the Record_OPT and associated tests
  4. Reverted all changes to dns.Message - a new EDNSMessage class will be introduced in #5675.
  5. Added code to parse OPT variable options and a class to represent variable options.
  6. More comments and responses below...

Replying to tom.prince:

So, a question I have: are the data structures meant to encode messages syntactically or semantically? I think that the answer to this whether we have something like OPTHeader or Record_OPT. If we view things syntactically, then OPT is a record that can appear in messages (theoretically anywhere, but only in the additional data in a conforming implementation). If we view things semantically, then OPT is just additional data of the message, that just happens to look like a record. If I understand EDNS? correctly, it seems this ticket is about the former representation.

  1. If this is about implementing a syntactic view, then does it make sense to disallow OPT in other sections?
  2. After further perusal of the RFCs, I realize that OPT interprets the RR header differently. I wonder if it would make sense to
    1. Probe for the type before decoding.
    2. or extract the data for OPTHeader from the existing instance of RRHeader.

OPTHeader is meant as a low level 'syntactic' structure (if I
understand you correctly).

My plan is to add new EDNSMessage class (in #5675) whose decode method
will first parse bytes with RRHeader.
If type 41 (OPT) is found in the additional section, it will rewind
the strio and parse the bytes again with OPTHeader (as in r38438).

Then the OPTHeader attributes will be used to calculate certain
EDNSMessage attributes eg EDNSMessage.rcode = EDNSMessage._rcode &
OPTHeader.rcode.

Setting EDNSMessage.rcode > 16 will cause EDNSMessage.encode to append
a suitable OPTHeader with the extended part of the rcode.

If type 41 is found in the answer, auth, or query section, a DNS error
will be returned (somehow)

https://tools.ietf.org/html/rfc6891#section-6.1.3

Some (incomplete) notes about the code itself:

  1. OPTHeader:
    1. Is there any reason for OPTHeader.fmt to be public? I realize that this is public on a bunch of thins in t.n.dns, but it seems like an implementation detail.

Only for consistency with the RRHeader.fmt but I guess that's not
necessary. Now private.

  1. rdlen? Then name is unclear, and I'm not sure it is useful, in any case.

I just copied the RFC terminology.

In RRHeader it's called rdlength (because thats what its called in
rcf1035.)

In RRHeader it's public so that dns.Message can read the correct
number of bytes for constructing the RRHeader.payload. (rdata)

In OPTHeader I guess it won't be necessary because I've made it
responsible for decoding its own rdata payload (the variable options).

  1. Is there a reason this doesn't use FancyStrMixin?

Done.

  1. self.name.decode is modifying a class attribute. :(

Fixed.

  • test_optHeaderName and test_optHeaderType say that the

attributes are readonly, which isn't true. This is
misleading.

Fixed.

  • Malformed packets might contain different values for name.

There was a short discussion on IRC and I decided to discard
the name for OPT records (ignoring possible non-root names).

20:24 < rwall> tomprince: Hi. In https://twistedmatrix.com/trac/ticket/5668 you said "Malformed packets might contain different values for name"
20:24 < rwall> I don't think I should raise an exception in that case, should I log a warning? What do you think?
20:26 < exarkun> no one wants arbitrary network peers to be able to fill their log file with junk
20:28 < rwall> So just ignore it you think?
20:29 < rwall> Other names objects log decoding errors
20:29 -!- Irssi: Pasting 5 lines to #twisted-dev. Press Ctrl-K if you wish to do this or Ctrl-C to cancel.
20:29 < exarkun> is the comment about `Name().decode(strio)`?
20:29 < rwall> eg             log.msg(
20:29 < rwall>                 "Decoded %d bytes in %s record, but rdlength is %d" % (
20:29 < rwall>                     soFar, self.fancybasename, length
20:29 < rwall>                 )
20:29 < rwall>             )
20:29 < rwall> yes
20:29 < exarkun> yes
20:29 < exarkun> Twisted Names is 10 years old though
20:29 < exarkun> So it may contain code that no longer represents what we think is a good idea
20:29 < rwall> understood.
20:30 < exarkun> It seems like ignoring it is probably fine.  If a peer sends something other than "" there, they're broken and who knows what
                 they're expecting to happen.
20:31 < rwall> we could return SRVERROR
20:31 < exarkun> We could
20:31 < rwall> So that they know they're broken.
20:31 < exarkun> But in this case it's not very hard to ignore the fact that they're broken
20:31 < rwall> ok
20:31 < tomprince> If the edns version isn't 0, we should probably error, though.
20:31 < exarkun> Returning an error seems nice to me, but inevitably someone will complain that it breaks interop with their favorite dns tool
20:32 < rwall> tomprince: Yes I can do that.
20:33 < rwall> Return SRVERROR I mean....but let me reread what the rfc says about version.
20:34 < exarkun> The RFC does have something to say about getting an EDNS version you don't understand
20:34 < exarkun> (but I forget what)
20:37 < rwall> "If a responder does not implement the VERSION level of the request, then it MUST respond with RCODE=BADVERS.I think I can take care
               of the
20:37 < rwall> oops
20:38 < rwall> Anyway I think I can defer that to #5675 or a later ticket.
  1. Record_OPT
    1. Don't use assert (see #6560).
    2. encode doesn't use self.fmt for some reason.

I've removed all that code now, it was a legacy of BobNovas original
patch. (#5454)

This isn't a complete review, but the branch seems in progress, and

I think there are some questions about direction and intent that
need to be answered before a more thorough review.

Understood.

As an aside, it seems like it would have made more sense if

.decode was a classmethod.

I've converted decode to a classmethod.

Thanks for your review and I look forward to more feedback now that
I've tidied up the branch.

-RichardW.

comment:12 Changed 16 months ago by tom.prince

As an aside, it seems like it would have made more sense if .decode was a classmethod.

I've converted decode to a classmethod

Sorry I was unclear. I haven't looked at the code, but I'm not sure making .decode a class method is compatible with implementing IEncodable. While I think making .decode a class method across the board would be an improvement, I don't see how to do that compatibly, and certainly don't expect you to do it in this ticket.

comment:13 follow-up: Changed 16 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall
  1. So, looking close, I don't see any need for OPTHeader and OPTVariableOptions to implement IEncodable, other than consistency. But I do think consistency is serious consideration. Perhaps get glyph or exarun or someone to weigh in here.
  2. OPTVariableOption: I'm not entirely happy with this name, but have no suggetions.
  3. I notice you only test options of lengh '2', and that all the data is very similiar. It would be better if the code and length and data could easily be distinguished in the encoded form.

Otherwise this looks good. Please merge after addressing 3 and either properly implementing IEncodable or after getting other input, stop implementing IEncodable.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 16 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Ready for review in log:branches/opt-record-5668-3

Thanks for the review.

  1. Build Results
    1. http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/opt-record-5668-3
  2. I fixed some Python3 build failures (bytes problems)
  3. Removed some unnecessary class attributes.
  4. More comments and responses below...

Replying to tom.prince:

  1. So, looking close, I don't see any need for OPTHeader and OPTVariableOptions to implement IEncodable, other than consistency. But I do think consistency is serious consideration. Perhaps get glyph or exarun or someone to weigh in here.

Having studied the t.n.dns in more detail it seems to me that
IEncodable is mainly there to support DNS name compression, and it's
possible that there may be a need to have names in OPTHeader options
in the future, so it's probably a good idea to support the interface.

The main problem is IEncodable.decode which would be much better if it
*returned* an IEncodable and could then be provided by a classmethod.

I haven't heard from exarkun yet, but in #5675 he did suggest using
classmethods for a new EDNSMessage class.

It's easier there, because dns.Message doesn't implement
IEncodable. (I guess because it initialises the compDict which is
passed down to various IEncoder.encode implementors.)

So anyway, I've changed _OPTHeader and _OPTVariableOption to properly
implement IEncodable for now, but I've also made them private so that
I can change them freely in a later branch.

_OPTHeader will be an implementation detail of EDNSMessage so it may
remain private.

  1. OPTVariableOption: I'm not entirely happy with this name, but have no suggetions.

I'm not happy with it either.

How about _OPTVariable?

  1. I notice you only test options of lengh '2', and that all the data is very similiar. It would be better if the code and length and data could easily be distinguished in the encoded form.

I added some more varied / longer bytes to those tests.

Otherwise this looks good. Please merge after addressing 3 and
either properly implementing IEncodable or after getting other
input, stop implementing IEncodable.

Please take another look.

I've started writing some code for #5675 EDNSMessage and to be honest
I'm having doubts about the _OPTHeader class.

In particular I wonder if it might be better for it to be a wrapper /
adaptor around an RRHeader.

Instead of _OPTHeader.decode, it could read the attributes from an
RRHeader (eg RRHeader.ttl) and convert them to corresponding
attributes in EDNSMessage.

Similarly, during encode it could generate an RRHeader suitable for
adding to the additional section.

That way there would only be one encode and decode implementation.

I'll try and attach some prototype code as a patch.

-RichardW.

comment:15 in reply to: ↑ 14 Changed 16 months ago by rwall

Replying to rwall:
<snip>

I've started writing some code for #5675 EDNSMessage and to be honest
I'm having doubts about the _OPTHeader class.
In particular I wonder if it might be better for it to be a wrapper /
adaptor around an RRHeader.
Instead of _OPTHeader.decode, it could read the attributes from an
RRHeader (eg RRHeader.ttl) and convert them to corresponding
attributes in EDNSMessage.

So that's what I've done, and I think it's more elegant.

I've added a fromRRHeader method which extracts all the standard
and EDNS values and options from the RRHeader and it's UnknownRecord.

I've also been working on #5675 to illustrate how I intend _OPTHeader
to be used.

I've done as much as I can in a branch

And added the parts which rely on _OPTHeader to
attachment:ticket/5675/edns-message-5675.OPTHeader-integration.patch

That patch illustrates how the ednsVersion attribute is
decoded via _OPTHeader via RRHeader.

And how an _OPTHeader can be added directly to the additional list of
Message during encoding.

I think I'll stop now as its becoming difficult to work on EDNSMessage
until this branch is merged.

Looking forward to some feedback.

-RichardW.

comment:16 Changed 14 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:17 follow-up: Changed 14 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to rwall
  • Status changed from assigned to new

Thanks for your work on this Richard!

  1. There's a simple conflict in the test module, please merge forward and resolve it. Unfortunately this prevents me from doing a build on buildbot now, so this isn't going to be a complete review.
  2. twisted/names/dns.py
    1. _OPTHeader
      1. showAttributes is defined with str - should this be nativeString instead?
      2. You don't need to duplicate the documentation for instance attributes in the class docstring and parameters in the __init__ docstring. See http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto9
      3. The docstring for the root is a little terse - combined with the test docs, I see what's going on here, but the implementation docstring alone didn't do it for me. A couple other things to consider here:
        1. the tests say it's a class attribute, but it's documented and used as an instance attribute (@ivar and self.name rather than @cvar and _OPTHeader.name or type(self).name (both of which are weird - I think of class variables as things that get used in a class method))
        2. does it need to be a class variable? it could be a global in the module - either public or private.
      4. Please use L{} for builtin types rather than C{} - eg L{int}
      5. docs for version includes the text '0' where I think it means something more like C{0} (I don't think it's actually allowed for the value to be a string, so '0' is a bit confusing)
      6. self.options = options or [] does a surprising thing if options is given as [] (and application code does a surprising thing of mutating the list passed in afterwards). Can you expand this to use the usual if options is None: options = []; self.options = options idiom?
      7. encode documents compDict as being ignored, but it passes the value in to RRHeader.encode which does indeed use it for name compression
      8. decode has an XXX. I think we would be better off here either:
        1. self.__dict__.update(newOptHeader.__dict__) (still tricky, probably not to be preferred)
        2. Use compareAttributes to construct a loop to copy exactly the right attributes
        3. using explicit assignment of each attribute
  3. twisted/names/test/test_dns.py
    1. OPTHeaderTests
      1. A number of test methods make multiple assertions. I think it would be great if each test method only made one assertion.
      2. It might be cool if the encoded form and the values of the attributes of the OPT record used in both test_encode and test_decode were shared between those two tests, avoiding the possibility the assertions being made could diverge (allowing encoding and decoding behavior to diverge). Hopefully the tests get shorter too. :)
      3. Same comment about the pair test_encodeWithOptions and test_decodeWithOptions, I think

Sorry I don't have any deep, meaningful comments about the nature of EDNS(0) or the architecture of the changes taken as a whole! The code basically just looks right, apart from the review comments above, which are the minor localized issues. I'd love to say "please fix and merge" at this point, but as I mentioned at the top I'd really like to see a buildbot run before saying that.

As I mentioned on IRC, I do think it would be pretty cool to see some examples based on an imagined high-level/abstract interface that will one day be constructed from these low-level primitives. Any kind of specification would be useful, really, but I like examples because they're a very easily approachable form of specification. With this in hand, it'll be easier to evaluate the low-level steps on the way to make sure they're steps in the right direction. Plus it'll be useful documentation someday (and there's always the chance it will attract more development effort, since people will know where to pitch in).

Thanks again!

comment:18 Changed 14 months ago by rwall

  • Branch changed from branches/opt-record-5668-3 to branches/opt-record-5668-4

(In [39321]) Branching to 'opt-record-5668-4'

comment:19 in reply to: ↑ 17 Changed 14 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Replying to exarkun:

Thanks for your work on this Richard!

  1. There's a simple conflict in the test module, please merge forward and resolve it.

Fixed r39322

Unfortunately this prevents me from doing a build on buildbot now, so this isn't going to be a complete review.

  1. twisted/names/dns.py
    1. _OPTHeader
      1. showAttributes is defined with str - should this be

nativeString instead?

Done. The OPTHeader name should always be null but I guess if it's not then
it's useful to see it rendered in the native format.

  1. You don't need to duplicate the documentation for instance attributes in the class docstring and parameters in the __init__ docstring. See http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto9

Done.

  1. The docstring for the root is a little terse - combined with the test docs, I see what's going on here, but the implementation docstring alone didn't do it for me.

Done.

A couple other things to consider here:

  1. the tests say it's a class attribute, but it's documented and used as an instance attribute (@ivar and self.name rather than @cvar and _OPTHeader.name or type(self).name (both of which are weird - I think of class variables as things that get used in a class method))

Done.

  1. does it need to be a class variable? it could be a global in the module - either public or private.

Done. Additionally I made both name and type readonly properties.

  1. Please use L{} for builtin types rather than C{} - eg L{int}

Done.

  1. docs for version includes the text '0' where I think it means something more like C{0} (I don't think it's actually allowed for the value to be a string, so '0' is a bit confusing)

Done.

  1. self.options = options or [] does a surprising thing if options is given as [] (and application code does a surprising thing of mutating the list passed in afterwards). Can you expand this to use the usual `if options is None: options = []; self.options = options` idiom?

Done.

  1. encode documents compDict as being ignored, but it passes the value in to RRHeader.encode which does indeed use it for name compression

Done. I originally thought "not used" because the name is always b,
but as far as I remember that will be compressed too.

I also hadn't considered that concrete implementations of
_OPTVariableOption may contain DNS names so the compression dict may
be used there too.

  1. decode has an XXX. I think we would be better off here either:
    1. self.__dict__.update(newOptHeader.__dict__) (still tricky, probably not to be preferred)
    2. Use compareAttributes to construct a loop to copy exactly the right attributes
    3. using explicit assignment of each attribute

Done. Using compareAttributes.

  1. twisted/names/test/test_dns.py
    1. OPTHeaderTests
      1. A number of test methods make multiple assertions. I think
      it would be great if each test method only made one assertion.

Done. I've made all tests single assertion.

_OPTHeader uses FancyEqMixin so in test_decode and
test_fromRRHeader I replaced the multiple assertions with a single
assertEqual of decoded and expected _OPTHeader instances.

Also added a test that trailing bytes are not consumed during decoding.

  1. It might be cool if the encoded form and the values of the attributes of the OPT record used in both test_encode and test_decode were shared between those two tests, avoiding the possibility the assertions being made could diverge (allowing encoding and decoding behavior to diverge). Hopefully the tests get shorter too. :)
  2. Same comment about the pair test_encodeWithOptions and test_decodeWithOptions, I think

I moved the bytes and matching OPTHeader instances to a factory class
and it has made the tests considerably shorter.

Sorry I don't have any deep, meaningful comments about the nature of
EDNS(0) or the architecture of the changes taken as a whole! The
code basically just looks right, apart from the review comments
above, which are the minor localized issues. I'd love to say
"please fix and merge" at this point, but as I mentioned at the top
I'd really like to see a buildbot run before saying that.

I fixed a few twistedchecker and pyflakes issues. All looks fine now
apart from some twistedchecker warnings which I think are spurious:

As I mentioned on IRC, I do think it would be pretty cool to see
some examples based on an imagined high-level/abstract interface
that will one day be constructed from these low-level primitives.
Any kind of specification would be useful, really, but I like
examples because they're a very easily approachable form of
specification. With this in hand, it'll be easier to evaluate the
low-level steps on the way to make sure they're steps in the right
direction. Plus it'll be useful documentation someday (and there's
always the chance it will attract more development effort, since
people will know where to pitch in).

I've tried.

  • doc/names/examples/test_edns_compliance.py

This example shows that _OPTHeader can be added to the existing
dns.Message to create invalid EDNS messages for testing the behaviour
of DNS servers.

  • doc/names/examples/txdig.py

For this one, I've copied across the EDNSMessage class from #5675 and
hacked together some EDNS protocols.

The proposed API here is EDNSResolver which is instantiated with
various EDNSMessage flags. It highlights some inflexibility in the
existing DNS protocol, factory and Resolver APIs which probably
deserve separate tickets.

See the docstrings for more comments.

  • doc/names/examples/edns_server.tac.py

This one shows what it takes to create a DNS server which at least
recognises EDNS OPT records and doesn't blindly send them back to the
client.

More work will be required in DNSServerFactory and EDNSMessage to make
server respond appropriately to different EDNS and standard query
messages.

See docstrings for more comments.

Ok, that's all. Thanks again for the review. Let me know what you think.

-RichardW.

comment:20 follow-up: Changed 14 months ago by exarkun

  • Keywords review removed
  • Owner set to rwall

Thanks.

  1. Need a news fragment. These APIs are all private, so I guess it's a misc.
  2. twisted/names/test/test_dns.py
    1. OPT_NON_STANDARD_ATTRIBUTES, BYTES, OBJECT seem like unusual names. How about something more conventional, like OPTNonStandardAttributes.{bytes,object}?
    2. Some missing whitespace near the + in test_decodeDiscardsName
  3. twisted/names/edns.py, doc/names/examples/
    1. Cool. It's very nice to get a preview of some of the functionality this is going to enable.
    2. It looks like at least one test in test_edns_compliance.py would be nicer if it used twisted.names.edns.EDNSDatagramProtocol instead of the mangling protocol? I see how it might be hard to exercise the double OPT record case with EDNSDatagramProtocol since that's invalid and generally good APIs should not encourage you to do invalid things...
    3. It looks like twisted/names/edns.py is suggesting some improvements that should be made to twisted/names/dns.py. Are there tickets for these things?
    4. While I'm glad to have seen this code, it doesn't look like it actually belongs in this branch - mainly because of the dependency on the big pile of new, untested code in twisted/names/edns.py. These seem like good examples to introduce when we've actually done the rest of the work to have this EDNS functionality, and as targets to be working towards I think they do a great job. Let's move them to another ticket, though?

If you address 2 and 3.4, I'm happy to see this merged. Thanks again.

Changed 14 months ago by rwall

Some examples of _OPTHeader usage.

comment:21 in reply to: ↑ 20 Changed 14 months ago by rwall

  • Status changed from new to assigned

Replying to exarkun:

Thanks.

  1. Need a news fragment. These APIs are all private, so I guess it's a misc.

Done.

  1. twisted/names/test/test_dns.py
    1. OPT_NON_STANDARD_ATTRIBUTES, BYTES, OBJECT seem like unusual names. How about something more conventional, like OPTNonStandardAttributes.{bytes,object}?

Done.

  1. Some missing whitespace near the + in test_decodeDiscardsName

Done.

  1. twisted/names/edns.py, doc/names/examples/
    1. Cool. It's very nice to get a preview of some of the functionality this is going to enable.
    2. It looks like at least one test in test_edns_compliance.py would be nicer if it used twisted.names.edns.EDNSDatagramProtocol instead of the mangling protocol? I see how it might be hard to exercise the double OPT record case with EDNSDatagramProtocol since that's invalid and generally good APIs should not encourage you to do invalid things...

Yep that's true. And even for the multiple _OPTHeader case, there's
nothing stopping me adding an OPT to the additional list
directly...currently...although maybe that's a problem with the
Message / EDNSMessage API.

  1. It looks like twisted/names/edns.py is suggesting some improvements that should be made to twisted/names/dns.py. Are there tickets for these things?

Not yet. I'll have a better idea about what's required when I've
finished EDNSMessage in #5675.

  1. While I'm glad to have seen this code, it doesn't look like it actually belongs in this branch - mainly because of the dependency on the big pile of new, untested code in twisted/names/edns.py. These seem like good examples to introduce when we've actually done the rest of the work to have this EDNS functionality, and as targets to be working towards I think they do a great job. Let's move them to another ticket, though?

Done.

See attachment:ticket:5668:opt-record-5668-examples.patch

If you address 2 and 3.4, I'm happy to see this merged. Thanks again.

Thanks for the review. I'll merge shortly.

-RichardW.

comment:22 Changed 14 months ago by rwall

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [39533]) Merge opt-record-5668-4: A private API for dealing with EDNS OPT records.

Author: rwall, BobNovas
Reviewers: tom.prince, exarkun
Fixes: #5668
Refs: #5454

A private API for dealing with EDNS OPT records and OPT variable
options.

comment:23 follow-up: Changed 14 months ago by itamar

It sounds like the ticket for examples was never actually opened? There's an attachment but it'll get lost here since this ticket is closed.

comment:24 in reply to: ↑ 23 Changed 14 months ago by rwall

Replying to itamar:

It sounds like the ticket for examples was never actually opened? There's an attachment but it'll get lost here since this ticket is closed.

I haven't created a new ticket, but I have added the examples to #5675 in r39562 and continued to improve and update them.

I intend to add them permanently as part of the documentation for #5670 (or an intermediate EDNSProtocol) ticket).

comment:25 Changed 11 months ago by rwall

  • Milestone set to EDNS0
Note: See TracTickets for help on using tickets.