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

CoAP discovery handler #346

Closed
wants to merge 17 commits into from
Closed

CoAP discovery handler #346

wants to merge 17 commits into from

Conversation

jiayihu
Copy link
Contributor

@jiayihu jiayihu commented Jul 1, 2021

What this PR does / why we need it: it adds support for the CoAP protocol, which is an important protocol for RESTful IoT devices.

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • version has been updated appropriately (./version.sh)

@jiayihu jiayihu changed the title Coap CoAP discovery handler Jul 1, 2021
@@ -6,7 +6,19 @@ metadata:
spec:
discoveryHandler:
name: {{ required "A custom.configuration.discoveryHandlerName is required." .Values.custom.configuration.discoveryHandlerName }}
discoveryDetails: {{ .Values.custom.configuration.discoveryDetails }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

for custom-configuration, i think the intent is that .Values.custom.configuration.discoveryDetails be yaml that can implicitly be what you are describing explicitly below. if we make the explicit changes you have here, custom-configuration becomes coap-configuration (and can no longer be other configs)

Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems more like the start of a deployment/helm/templates/coap-configuration.yaml :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the custom.configuration values should be added to a coap section of the values file: https://github.com/deislabs/akri/blob/main/deployment/helm/values.yaml. A coap-discovery-handler.yaml file should also be added instead of using the custom one.

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thanks for this great work @jiayihu! See my comments for my main questions around multicast discovery and async coap client. Also, can you add a coap-configuration.md document under the docs folder to explain installation and the discoveryDetails parameters provided for filtering? For example, here is the one for ONVIF. Finally, are you interested in CoAP being able to be compiled into the agent? If so, I can point out where those changes need to happen in a subsequent PR.


The Broker acts as an HTTP-to-CoAP Proxy. It translates RESTful HTTP requests into RESTful CoAP requests and vice versa for the response. "Cross-Protocol Proxying between CoAP and HTTP" is defined in section 10 of RFC 7252. Currently, the Broker forwards only GET requests.

The Broker is also in an excellent position to cache CoAP responses. "Unlike HTTP, the cacheability of CoAP responses does not depend on the request method, but it depends on the Response Code." Currently, the Broker only caches CoAP responses with status code equal to `2.05 Content`, which is returned if the device is not reachable during the connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove extra � character here: 2.05 Content

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2021-07-07 at 23 50 02
Sorry what character are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird. It shows up on mine:
image

docs/proposals/coap.md Outdated Show resolved Hide resolved

- Is there a better way to store the discovered resources as Configuration in the cluster?

The current implementation would need a controller to accept queries about available resources and return the name of the Broker's service which can communicate to the device. The device is listed as a generic `akri.sh/coap-021dd7` resource on the node, which is too generic to be useful by any application. A better label would be `akri.sh/oic.r.temperature-021dd7`, the discovered resource. This would allow using the K8s controller for scheduling pods that need the resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

For this, someone could deploy a configuration named oir.r.tempurature and filter for specifically temperature devices right?

Copy link
Contributor Author

@jiayihu jiayihu Jul 7, 2021

Choose a reason for hiding this comment

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

That is definitely an idea but the number of resource types is quite huge. Besides, as cluster administrator, who deploys the configurations, you may not know them in advance. Imagine you tasked to set up the clusters of a company with several geographical locations. It also means that you need two separate configurations, thus duplicate discovery handlers, for a same device that exposes both temperature and brightness as resources.

With that being said, I like the idea. It should also already be supported by using the queryFilter discoveryDetail like this:

queryFilter:
  name: "rt" // resource type
  value: "oic.r.temperature"

It's part of the CoAP standard that the devices can supported filtering by using a query filter. Moreover, my understanding of the RFC is that it supports only one filter. Even though I've implemented it, I never actually thought about using it for my use case 😄 It remains a workaround local deployments though, IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you mean by ", thus duplicate discovery handlers"? There would still only be one coap DH running on the node but it would be called twice, once for each config


### With Akri

The Akri Configuration defines a list of IP addresses to use for resource discovery, implementing thus unicast discovery. Likewise, a multicast IP address can be used for multicast discovery. Both methods can be used at the same time.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a multicast example? Does an ip address always have to be provided or can a network scan be performed? Also static ip address are more of an aliveness check and resources supported by that device check rather than discovery, correct?

Copy link
Contributor Author

@jiayihu jiayihu Jul 7, 2021

Choose a reason for hiding this comment

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

A multicast discovery is just a normal CoAP request sent to the special IP address 224.0.1.187 (in the case of IPv4) and with the broadcast flag on the IP packet. CoAP devices should be instructed to reply if they receive that kind of broadcast packet. The discovery handler only supports IPv4 multicast for now.

Static IP addresses are more for debugging or local clusters where you know which devices are available. I don't expect them to be used for real-world cases. That's why the default values.yml use multicast. For instance I use the static IP of my laptop during development, the IP of my local embedded device when actually testing the whole system.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great!

samples/brokers/coap-broker/src/http_coap.rs Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
@kate-goldenring
Copy link
Contributor

@jiayihu can you add some documentation on how to test your discovery handler? Do you happen to have a way you are mocking the coap devices?

@jiayihu
Copy link
Contributor Author

jiayihu commented Jul 7, 2021

Sure, I'll add more documentation. To mock the CoAP devices you can use coap-rs (I personally use node-coap because it's quicker to use an interpreted language for testing). Should I just provide the instructions in a .md file or setup an actual project? I think the latter would be better, but I'm not sure if you want something like that within the repo.

@kate-goldenring
Copy link
Contributor

Sure, I'll add more documentation. To mock the CoAP devices you can use coap-rs (I personally use node-coap because it's quicker to use an interpreted language for testing). Should I just provide the instructions in a .md file or setup an actual project? I think the latter would be better, but I'm not sure if you want something like that within the repo.

Are you referring to only documentation on mocking the devices? What would go in the "project" you're suggesting? A coap-configuration.md file should come with this pr that explains how to configure a Configuration for coap. If you are willing, an end-to-end demo would be fabulous that shows how the configuration, discovery handler, and broker come together, like the ones for opc ua and udev.

@kate-goldenring
Copy link
Contributor

@jiayihu give us a bump when you have addressed the comments and think this is ready for another round of reviews.

@jiayihu
Copy link
Contributor Author

jiayihu commented Aug 13, 2021

@kate-goldenring I think this is finally ready for a second review 🎉
I've seen that you've moved the doc to a separate repo. I can open a separate PR there after you have approved the files here.

@kate-goldenring
Copy link
Contributor

@kate-goldenring I think this is finally ready for a second review 🎉
I've seen that you've moved the doc to a separate repo. I can open a separate PR there after you have approved the files here.

Hi @jiayihu, this is great to see! I've been on vacation the past few days, but I'll start taking a look this week! Another PR with docs on the docs site is perfect.

let query_filter = query_filter.clone();

tokio::task::spawn_blocking(move || {
let mut shared_devices = shared_devices.lock().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe wait to lock shared_devices after making the call to discovery_endpoint

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thanks for adding in the configuration file, dockerfiles, and deployment yaml! Added some questions and comments about settings and docs. Haven't looked into the implementation yet. I'll look through that next.

memoryRequest: 10Mi
# cpuRequest defines the minimum amount of CPU that must be available to this Pod
# for it to be scheduled by the Kubernetes Scheduler
cpuRequest: 10m
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably do an analysis using the vertical pod autoscaler as we did with other Akri components to determine good estimates here. Or we could leave out requests and limits for now and add an issue to do that.

capacity: 1
discoveryDetails:
multicast: true
multicastIpAddress: 224.0.1.187
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the default for coap as explained in the configuration doc, should it ever be configurable?

{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this some auto-linting? Can you undo to this and other irrelevant changes to other files.

spec:
containers:
- name: akri-coap-discovery
image: {{ printf "%s:%s" (required "A coap.discovery.image.repository is required." .Values.coap.discovery.image.repository) .Values.coap.discovery.image.tag | quote }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Akri will host the image. set the value as done in reference udev-discovery-handler.yaml for an example

{{- else }}
queryFilter: null
{{- end }}
discoveryTimeoutSeconds: {{ .Values.coap.configuration.discoveryDetails.discoveryTimeoutSeconds }}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this used?

# Copy over container legal notice
COPY ./build/container-images-legal-notice.md .

RUN apt-get update && apt-get install -y --no-install-recommends libssl-dev openssl && apt-get clean
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with broker: are libssl-dev and openssl needed? I dont remember why they were necessary in all of the other containers


RUN apt-get update && apt-get install -y --no-install-recommends libssl-dev openssl && apt-get clean
COPY ./target/${CROSS_BUILD_TARGET}/${BUILD_TYPE}/coap-broker /coap-broker
ENV RUST_LOG=coap_broker,api
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the api package?


## Deploying the CoAP Discovery Handler

In order for the Agent to know how to discover CoAP servers an CoAP Discovery Handler must exist. Akri supports an Agent image that includes all supported Discovery Handlers. This Agent will be used if `agent.full=true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation doesn't support embedding coap in the Agent. A subsequent PR can add that support. For now, this section can be simplified to "The CoAP Discovery Handlers are deployed by specifying coap.discovery.enabled=true when installing Akri."


Discovery Handlers are passed discovery details that are set in a Configuration to determine what to discover, filter out of discovery, and so on.

By default, the CoAP Discovery Handler doesn't require any additional information as it implements the multicast CoAP discovery (described in [Section 8 of RFC 7252](https://datatracker.ietf.org/doc/html/rfc7252#section-8)). The IPv4 address reserved for "All CoAP Nodes" is `224.0.1.187`. Devices must support the default port `5683` as specified by the standard. At the time of writing, IPv6 multicast is not supported in the CoAP Discovery Handler.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this IPv4 address ever be configurable? When would it change?

|---|---|---|---|
| coap.configuration.discoveryDetails.multicast | boolean | true | Enable IPv4 multicast discovery |
| coap.configuration.discoveryDetails.multicastIpAddress | string | 224.0.1.187 | The IPv4 to which the Discovery Handler sends the packets |
| coap.configuration.discoveryDetails.staticIpAddresses | Array of strings | [] | Additional static IP addresses to look for during the discovery. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a description of when to use coap.configuration.discoveryDetails.staticIpAddresses

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

I just finished looking through everything and wow! This is some awesome work. The coap-demo.md was smooth. I was able to get through it without any problems! I've made a lot of comments about nits and conventions, but overall this is looking great. One more push and we should be there. Once you have addressed the docs comments, go ahead and put a PR into akri-docs. Another note, maybe run cargo clippy -p coap-discovery-handler and cargo clippy -p coap-broker to grab any syntax fixes.


### Broker Pod Settings

If you would like workloads ("broker" Pods) to be deployed automatically to discovered devices, a broker image should be specified in the Configuration. Alternatively, if it meets your scenario, you could use the Akri's default CoAP broker ("ghcr.io/deislabs/akri/coap-broker").
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove "the" in "you could use the Akri's default CoAP broker"


### Other settings

The CoAP Discovery Handlers supports the same "Capacity" and "Automatic Service Creation" as OPC UA. Refer to the latter [documentation](https://github.com/deislabs/akri/blob/main/docs/opcua-configuration.md#disabling-automatic-service-creation) for additional information.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is supported by all discovery handlers, but we are repeating the setting in each configuration file so they can be standalone. Can you copy and past that section into here, chaning opcua to coap where applicable?


fn discover_endpoint(
coap_client: &impl CoAPClient,
ip_address: &String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you aren't using &str?

akri-discovery-utils = { path = "../../discovery-utils" }
anyhow = "1.0.41"
async-trait = "0.1.51"
coap = { git = "https://github.com/Covertness/coap-rs" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we point to their crate instead of GitHub repo. coap = "0.11"?

@@ -62,7 +62,7 @@ jobs:
BUILD_ARM64: 0
BUILD_SLIM_AGENT: 0
AGENT_FEATURES: "agent-full"
PACKAGES_TO_EXCLUDE: "akri-udev akri-onvif akri-opcua udev-video-broker debug-echo-discovery-handler onvif-discovery-handler opcua-discovery-handler udev-discovery-handler"
PACKAGES_TO_EXCLUDE: "akri-udev akri-onvif akri-opcua udev-video-broker coap-discovery-handler debug-echo-discovery-handler onvif-discovery-handler opcua-discovery-handler udev-discovery-handler"
Copy link
Contributor

Choose a reason for hiding this comment

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

This isnt needed, since it isnt a package in the Agent

## Deploy a Kubernetes application which requests the temperature via HTTP

The Akri CoAP built-in Broker Pod is automatically deployed when an instance is created. The Broker exposes a RESTful endpoint which translates HTTP request to CoAP requests. To inspect the associated Kubernetes Service, run `kubectl get crd`.

Copy link
Contributor

Choose a reason for hiding this comment

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

should this be kubectl get services?

```
kubectl run curl --image=radial/busyboxplus:curl -i --tty

[ root@curl:/ ]$ curl http://akri-coap-920f97-svc/sensors/temp
Copy link
Contributor

Choose a reason for hiding this comment

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

specify that the id varies and should be changed when curling

coap-lite = "0.5"
env_logger = "0.8.4"
log = "0.4.14"
tokio = { version = "1", features = ["full"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: specify specific features to minimize unused code


[dependencies]
akri-shared = { path = "../../../shared" }
coap = { git = "https://github.com/Covertness/coap-rs" }
Copy link
Contributor

Choose a reason for hiding this comment

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

specify crate instead of github

| coap.configuration.discoveryDetails.multicast | boolean | true | Enable IPv4 multicast discovery |
| coap.configuration.discoveryDetails.multicastIpAddress | string | 224.0.1.187 | The IPv4 to which the Discovery Handler sends the packets |
| coap.configuration.discoveryDetails.staticIpAddresses | Array of strings | [] | Additional static IP addresses to look for during the discovery. |
| coap.configuration.discoveryDetails.queryFilter | { name: string, value: string } | {} | Single name-value pair to filter the discovered resource, as described in [Section 4.1 of RFC 6690](https://datatracker.ietf.org/doc/html/rfc6690#section-4.1) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe break this out into two different fields for clarity, since it seems like the following is the appropriate was to configure the filter:

   --set coap.configuration.discoveryDetails.queryFilter.name="rt" \
   --set coap.configuration.discoveryDetails.queryFilter.value="oic.r.temperature"

@@ -8,13 +8,16 @@ members = [
"shared",
"agent",
"controller",
"samples/apps/coap-device",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if this should live in akri as it is a mock device. I feel like the coap-demo.md is sufficient for getting people going. Or we could put it on a branch of akri, or you could host it if interested. @bfjelds for thoughts

serde_json = "1.0.45"
serde_derive = "1.0.104"
serde_yaml = "0.8.17"
tokio = { version = "0.2", features = ["rt-threaded", "sync", "time", "stream", "fs", "macros", "uds"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We recently went to tokio 1.0. Could you bump to that version? It isnt required since it is not embedded in the agent.

@@ -28,8 +28,6 @@ spec:
cpu: {{ .Values.debugEcho.configuration.brokerPod.resources.cpuRequest }}
limits:
{{`"{{PLACEHOLDER}}"`}} : "1"
memory: {{ .Values.debugEcho.configuration.brokerPod.resources.memoryLimit }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was this removed?

name = "coap-discovery-handler"
version = "0.6.8"
authors = ["Jiayi Hu <jiayi.ghu@gmail.com>"]
edition = "2018"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: 2018? should this be 2021?

@@ -0,0 +1,19 @@
[package]
name = "coap-broker"
version = "0.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the versions match our overall versioning ("0.6.14" ... or better yet "0.7.0")?

Copy link
Collaborator

Choose a reason for hiding this comment

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

and update version.sh for each new Cargo.toml verion location?

@bfjelds
Copy link
Collaborator

bfjelds commented Aug 31, 2021

there are a lot of //TODO comments. i wonder if these shouldn't be converted to issues?

@jiayihu
Copy link
Contributor Author

jiayihu commented Aug 31, 2021

Thank you to both of you for the comments. Unfortunately, vacation has ended and I'm not able to go back to the PR right now 🙃 It might take a while again

@kate-goldenring kate-goldenring linked an issue Sep 7, 2021 that may be closed by this pull request
@github-actions
Copy link
Contributor

PR has been automatically marked as stale due to inactivity for 90 days. Update the PR to remove label, otherwise it will be automatically closed."

@github-actions github-actions bot added the stale label Nov 30, 2021
@romoh
Copy link
Contributor

romoh commented Dec 8, 2021

@jiayihu, there have been a couple of changes since your PR. Let us know if you need help with rebase once you are ready to work on this again., we'll be glad to help.

@github-actions github-actions bot removed the stale label Dec 9, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

PR has been automatically marked as stale due to inactivity for 90 days. Update the PR to remove label, otherwise it will be automatically closed."

@github-actions github-actions bot added the stale label Mar 9, 2022
@romoh romoh added keep-alive and removed stale labels Apr 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

PR has been automatically marked as stale due to inactivity for 90 days. Update the PR to remove label, otherwise it will be automatically closed."

@github-actions github-actions bot added the stale label Jul 5, 2022
@github-actions github-actions bot closed this Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support CoRE resource discovery protocol
4 participants