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

feat(binding-coap): implement DNS-SD via mDNS #963

Merged
merged 6 commits into from
May 11, 2023

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Apr 27, 2023

This PR adds DNS-SD functionality (via mDNS) to the binding-coap package in order to cover the respective assertion from the Discovery specification.

I still need to do some interoperability testing, therefore I opened this PR only as a draft for now. However, hopefully the PR will be ready for review soon :)

@relu91
Copy link
Member

relu91 commented May 3, 2023

I feel like the same code can be used to expose the same endpoints using HTTP. For this reason, it makes sense to do more software engineering... I like the idea of having dedicated packages also for "introduction methods", bindings that will then advertise an URL to the core and the core will forward it to the installed discovery services. This would works also for Directory and it might solve some of the issues we experienced.

@JKRhb
Copy link
Member Author

JKRhb commented May 3, 2023

I feel like the same code can be used to expose the same endpoints using HTTP. For this reason, it makes sense to do more software engineering... I like the idea of having dedicated packages also for "introduction methods", bindings that will then advertise an URL to the core and the core will forward it to the installed discovery services. This would works also for Directory and it might solve some of the issues we experienced.

Oh, that's a very good idea! I will try to encapsulate the code as much as possible (currently, it's still very WIP) so that we can also reuse it in other places (moving it to its own package).

@JKRhb
Copy link
Member Author

JKRhb commented May 3, 2023

The introduction logic is now moved inside a new MdnsIntroducerclass, which should already be usable with CoAP. Before the PR is ready for review, though, I will try to make the class more generic, also allowing for other protocols.

@JKRhb JKRhb marked this pull request as ready for review May 6, 2023 07:44
@JKRhb
Copy link
Member Author

JKRhb commented May 6, 2023

I think this PR would now be ready for review :)

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

As always thank you really much for your efforts @JKRhb ! I like the MdnsIntroducer interface, do you think we can in the future move it to its own package? Or are there some blockers that we need to take into account? Have you considered renaming MdnsIntroducer to MdnsIntroductionMethod?

Finally, I would add some basic tests (as we did with Zion https://github.com/vaimee/zion/blob/main/test/unit/introduction/mdns.spec.ts) and fix the issue below.

packages/binding-coap/src/mdns-introducer.ts Outdated Show resolved Hide resolved
@JKRhb
Copy link
Member Author

JKRhb commented May 9, 2023

I like the MdnsIntroducer interface, do you think we can in the future move it to its own package? Or are there some blockers that we need to take into account?

Moving it to its own package sounds good to me :) I was wondering, though: Should it become a package on its own? Or should it be a collection of introduction mechanisms? Maybe we can also discuss these questions in a dedicated issue.

Have you considered renaming MdnsIntroducer to MdnsIntroductionMethod?

We can surely rename it, MdnsIntroducer was kind of the first idea that came to my mind when creating the class 😅 However, I personally liked the parallel to the "Discoverer" terminology, while the name might be a bit more accurate than MdnsIntroductionMethod since it is rather an implementation of the introduction method than the method itself. But MdnsIntroductionMethod would also be fine for me as a name :)

Finally, I would add some basic tests (as we did with Zion https://github.com/vaimee/zion/blob/main/test/unit/introduction/mdns.spec.ts)

Oh, sure! Unfortunately, I am a bit occupied for the next few days but I will try to add some tests over the course of the weekend :)

@danielpeintner
Copy link
Member

Moving it to its own package sounds good to me :) I was wondering, though: Should it become a package on its own? Or should it be a collection of introduction mechanisms? Maybe we can also discuss these questions in a dedicated issue.

I agree about having some more discussions about where to place it.
I wonder whether it "deserves" its own package or whether it should move to an existing package (both generic packages core and td-tools do not seem to be a good fit).
Hence discoverers package or so might be an option? Another option would be something like "discoverer-bla" etc like we do for bindings...

@relu91
Copy link
Member

relu91 commented May 10, 2023

Moving it to its own package sounds good to me :) I was wondering, though: Should it become a package on its own? Or should it be a collection of introduction mechanisms? Maybe we can also discuss these questions in a dedicated issue.

I agree about having some more discussions about where to place it. I wonder whether it "deserves" its own package or whether it should move to an existing package (both generic packages core and td-tools do not seem to be a good fit). Hence discoverers package or so might be an option? Another option would be something like "discoverer-bla" etc like we do for bindings...

I agree that we currently don't have a nice package to fit this class. Having a single package just for this seems overkill. It depends on which other introduction methods we want to support and how much they are different from each other.

relu91

This comment was marked as duplicate.

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

A small comment below + there is a conflict with the master.


public async close(): Promise<void> {
return new Promise((resolve) => {
this.mdns.destroy(resolve);
Copy link
Member

Choose a reason for hiding this comment

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

I checked https://nodejs.org/api/net.html#serverclosecallback and I saw that the callback is called with an error if the server was not open before. I know that it might be a kind of impossible state, but maybe it is worth handling this corner case as well. @JKRhb what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, @relu91 :) The conflict should now be resolved via a rebase and I tried to incorporate the error case in 7c2a867, did I get that right?

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

Perfect! merging.

@relu91 relu91 merged commit a9011b7 into eclipse-thingweb:master May 11, 2023
7 checks passed
@JKRhb JKRhb deleted the coap-dns-sd branch May 11, 2023 17:54
@egekorkan
Copy link
Member

@JKRhb can you update the testing results in wot-testing accordingly? I think this is really cool and it should help with it ;)

@JKRhb
Copy link
Member Author

JKRhb commented May 16, 2023

@JKRhb can you update the testing results in wot-testing accordingly? I think this is really cool and it should help with it ;)

Sure! I will open a PR in the testing repo shortly :) With this new implementation, the assertion should actually be covered already :)

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

4 participants