Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate edns methods, add extensions #1675

Merged
merged 3 commits into from
Apr 8, 2022

Conversation

leshow
Copy link
Contributor

@leshow leshow commented Apr 4, 2022

Assuming we're still good to go ahead with this modification, for reference the workaround for not being able to remove EDNS, as best I can work out is:

fn remove_edns(msg: Message) -> Message {
    let rcode = msg.response_code();
    let MessageParts {
        header,
        queries,
        answers,
        name_servers,
        additionals,
        sig0,
        edns,
    } = msg.into_parts();

    let mut ret = Message::new();
    ret.set_id(header.id())
        .set_message_type(header.message_type())
        .set_op_code(header.op_code())
        .set_authoritative(header.authoritative())
        .set_truncated(header.truncated())
        .set_recursion_desired(header.recursion_desired())
        .set_recursion_available(header.recursion_available())
        .set_authentic_data(header.authentic_data())
        .set_checking_disabled(header.checking_disabled())
        .set_response_code(rcode)
        .add_queries(queries)
        .add_answers(answers)
        .add_name_servers(name_servers);
    ret.insert_additionals(additionals);

    for sig in sig0 {
        ret.add_sig0(sig);
    }
    ret
}

Looking at all this, a MessageBuilder could probably help make constructing new Messages lot nicer, happy to sketch that out on an issue or something if you like. Let me know if you want to remove the MessageParts -> Message bit from this also... I included it here but if we pursue a builder maybe it's not as necessary.

@leshow
Copy link
Contributor Author

leshow commented Apr 4, 2022

Woop, forgot to re-run the tests lol. doh

edit:
After messing around with all the call sites I'm less confident in making this change, going to play with a few things and I'll post again later.

@leshow leshow changed the title breaking: make Message edns_mut return Optional WIP: breaking: make Message edns_mut return Optional Apr 4, 2022
@leshow leshow changed the title WIP: breaking: make Message edns_mut return Optional deprecate edns methods, add extensions Apr 7, 2022
@leshow leshow force-pushed the msg_modifiers branch 3 times, most recently from b9de4f1 to 2c39e21 Compare April 7, 2022 17:43
@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #1675 (ad5b0c4) into main (f91b45f) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1675      +/-   ##
==========================================
- Coverage   79.92%   79.87%   -0.05%     
==========================================
  Files         177      183       +6     
  Lines       18309    18597     +288     
==========================================
+ Hits        14632    14853     +221     
- Misses       3677     3744      +67     

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

This looks good. See this comment: #1675 (comment)

I think once that is changed in terms of usage, this is good.

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Looks great. There's a minor unused error right now, this is ready once that's clean.

@leshow
Copy link
Contributor Author

leshow commented Apr 8, 2022

Let me know if you want an additional unit test or something to bump up the codecov

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for fixing up this interface. And thanks for the PR.

@bluejekyll bluejekyll merged commit eb58c66 into hickory-dns:main Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants