should `proto::rr::resource::Record.rdata` really be an `Option`? about trust-dns HOT 6 CLOSED

There are some cases where the RDATA is unspecified yet the record-type is. This happens in the dns update sections of the code. That’s mostly why it ended up being Option.

by "unspecified" do you mean that the RDATA contents do not match what's specified in the record-type header field or may even be completely absent? can that unspecified state be serialized / deserialized into wire format (bytes)? or is it a state that's limited to e.g. the construction / build stage of a resource record?

if the state is limited to a section of the code, we could have two Record types, a specified one and an unspecified one, that share a lot of code save for the RDATA getter / setter.

struct SpecifiedRecord<RDATA> {
    header: RecordHeader,
    rdata: RDATA,
}

struct UnspecifiedRecord {
    header: RecordHeader,
    // `.record_type` is allowed to diverge from `.rdata`
    record_type: RecordType,
    rdata: Option<Unknown>, // or maybe Option<RData>
}

struct RecordHeader {
    name: Name,
    ttl: u32,
    // ..
}

if you could point out where in code the unspecified state happens, that would help me better understand how it's being used

from trust-dns.

This sounds reasonable to me. This might be an oversight from when we migrated to the generically-typed RData setup, or it might be necessary in some contexts to take out the rdata value without having ownership of the Record? I think staging an attempt at simplifying here would be helpful. (@bluejekyll might have more context, but might also take longer to respond...)

from trust-dns.

There are some cases where the RDATA is unspecified yet the record-type is. This happens in the dns update sections of the code. That’s mostly why it ended up being Option.

I couldn’t figure out an elegant way to deal with that case.

from trust-dns.

if you could point out where in code the unspecified state happens

I found the relevant code in crates/proto/op/update_message.rs which implements RFC2136 (DNS update). After some initial sketching I think I'm leaning towards creating a separate UpdateRecord, which accepts an Option-al .rdata field, and a separate UpdateMessage, which better aligns with the DNS message structure described in RFC2136 (section 2); and then dedicating the existing Record, tweaked to have a non-Option-al .rdata field, and Message to QUERY-like logic (RFC1035).

Furthermore, RFC4033 states that "The DNS security extensions provide data and origin authentication for DNS data. The mechanisms outlined above are not designed to protect operations such as zone transfers and dynamic update (RFC2136, RFC3007)" so I think it makes sense to not overload the Message struct, which can include DNSSEC signatures, with DNS update functionality, which is unrelated to DNSSEC.

from trust-dns.

Strong 👍 from me!

from trust-dns.

I’ve tried to go down this route in the past. I’m open to see where your investigation leads. There are a lot of areas of the code that rely on Message and Record, so it might end up being a large change.

from trust-dns.