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

NOISSUE - Add DTLS support to CoAP messaging #1909

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

felixgateru
Copy link
Contributor

@felixgateru felixgateru commented Sep 15, 2023

What does this do?

Add DTLS support to CoAP messaging

Which issue(s) does this PR fix/relate to?

None

List any changes that modify/break current functionality

Refactor: Updated the nginx reverse proxy with CoAP stream.
Refactor: Add internal server configuration for DTLS support.

Have you included tests for your changes?

No

Did you document any new/modified functionality?

No

Notes

To be merged after https://github.com/mainflux/mainflux/pull/1918

@felixgateru felixgateru changed the title NOISSUE - description NOISSUE - Add DTLS support to CoAP messaging Sep 15, 2023
@@ -21,6 +22,8 @@ type Server struct {
handler mux.HandlerFunc
}

var enableDTLS = true
Copy link
Contributor

Choose a reason for hiding this comment

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

use env var to pass this

@@ -21,6 +22,8 @@ type Server struct {
handler mux.HandlerFunc
}

var enableDTLS = true
Copy link
Contributor

Choose a reason for hiding this comment

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

use env var to pass this

@@ -55,6 +58,22 @@ func (s *Server) Start() error {
go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be the default case in line 78

@felixgateru felixgateru force-pushed the coap_dtls branch 2 times, most recently from fdf4654 to 908e596 Compare October 18, 2023 10:10
@@ -24,6 +27,8 @@ type Server struct {
handler mux.HandlerFunc
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

}

go func() {
errCh <- gocoap.ListenAndServeTCPTLS("udp", s.Address, tlsConfig, s.handler)
errCh <- gocoap.ListenAndServeDTLS("udp", s.Address, dtlsConfig, s.handler)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is ListenTCP removed, we shoulld have all 3 options, no TLS, TLS and DTLS. Modify the config file and this

Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

I had two minor remarks. The rest is good. Please address those and remove draft, then we will merge.

@@ -161,4 +161,22 @@ stream {
}
}

# COAP
Copy link
Contributor

Choose a reason for hiding this comment

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

CoAP

@@ -151,4 +151,31 @@ stream {
}
}

#COAP
Copy link
Contributor

Choose a reason for hiding this comment

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

CoAP, and add space after the #

@felixgateru felixgateru marked this pull request as ready for review October 20, 2023 03:34
@felixgateru felixgateru requested a review from a team as a code owner October 20, 2023 03:34
@@ -151,4 +151,31 @@ stream {
}
}

#CoAP
Copy link
Contributor

Choose a reason for hiding this comment

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

Put a space after the # comment, I have already demanded this.

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

Having two top-level stream directives is not allowed. Move CoAP server under the same stream as MQTT and always manually test PRs since this won't work due to multiple stream directives.


upstream coap_cluster {
server coap-adapter:${MF_COAP_ADAPTER_HTTP_PORT};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add an empty line to the end of the file.

# SPDX-License-Identifier: Apache-2.0

upstream coap_cluster {
server coap-adapter:${MF_COAP_ADAPTER_HTTP_PORT};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just CoAP port, not CoAP HTTP port (HTTP is used only for healthcheck).

@felixgateru felixgateru force-pushed the coap_dtls branch 2 times, most recently from 4c6a6c4 to 0e4b393 Compare October 24, 2023 03:06
@arvindh123
Copy link
Contributor

Signed-off-by: 1998-felix <felix.gateru@gmail.com>
Signed-off-by: 1998-felix <felix.gateru@gmail.com>
Signed-off-by: felix.gateru <felix.gateru@gmail.com>
Signed-off-by: felix.gateru <felix.gateru@gmail.com>
Signed-off-by: felix.gateru <felix.gateru@gmail.com>
Signed-off-by: felix.gateru <felix.gateru@gmail.com>
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

5 participants