What is this OPT thing @rakoo did?

[taoeffect]

This:

        case WRITE_END:
          // Remove temporary additional OPT
          // TODO: @rakoo, wtf is this? Explanation please! Why? What's going on?
          packet.additional = packet.additional.filter(function(val) {
            return val.type != consts.NAME_TO_QTYPE.OPT;
          })

Is that correct? Will that produce the right output for testing EDNS support with the fixtures? What is even going on?

[rakoo]

This part of the code is kind of fishy indeed. The "issue" is that EDNS information (ie information on the DNS exchange itself, not on the queried services) is encoded as an OPT record in the additional section. So there must be a way to encode and decode this.

The flow is that a packet enters the Packet.write method for serializing. If it has EDNS information, this information will be converted to an OPT record and added in the additional array so that it can be serialized just like any other additional record. We "piggyback" this record on the user-provided packet, serialize it, and then when serializing is done (in WRITE_END state) remove that structure, because it is added by the library and I don't like modifying user-provided structure.

So the obvious problem here is that we modify the packet, which the user should assume will not be modified by the library, and hope we will clean after us. This can fail, so a solution would be to create a temporary additional structure that is not stored in the packet but still serialized afterwards.

[taoeffect]

I don't get it. There's already this:

function writeOpt(buff, packet) {
  var pos;

  while (packet.edns_options.length) {
    val = packet.edns_options.pop();
    buff.writeUInt16BE(val.code);
    buff.writeUInt16BE(val.data.length);
    for (pos = 0; pos < val.data.length; pos++) {
      buff.writeUInt8(val.data.readUInt8(pos));
    }
  }

  return WRITE_RESOURCE_DONE;
}

And that should be based on the whether there's anything in packet.edns_options.

The code that's quoted in the issue seems quite hackish. Can't you/we more properly check edns_options for it? Also I just noticed that writeOpt serializes val.data very inefficiently, so I'll open another issue for that: #11

[taoeffect]

I agree that we shouldn't be modifying the packet (at least without good reason). Another place where it's modified is in writeEdns:

  packet.header.rcode = pos - (val.ttl << 4);

[taoeffect]

Thanks for the explanation @rakoo. I guess I meant to just ask: do you think it's possible to just use the edns_options to do this, without having to modify packet.additional?

[tjfontaine]

right the idea is to follow something akin to edns_options to be generic about adding options to the opt field -- part of the validation of the packet (unclear if this should be in the parser/serializer) should be that there is only one OPT packet in the additional field.

I'm open to fixing how edns_options works, but I don't think it makes sense for people consuming it to know how to twiddle OPT fields themselves

[taoeffect]

Fixed in PR #7, I think, have a look.