-
Notifications
You must be signed in to change notification settings - Fork 673
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
base: main
Are you sure you want to change the base?
Conversation
internal/server/coap/coap.go
Outdated
@@ -21,6 +22,8 @@ type Server struct { | |||
handler mux.HandlerFunc | |||
} | |||
|
|||
var enableDTLS = true |
There was a problem hiding this comment.
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
internal/server/coap/coap.go
Outdated
@@ -21,6 +22,8 @@ type Server struct { | |||
handler mux.HandlerFunc | |||
} | |||
|
|||
var enableDTLS = true |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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
fdf4654
to
908e596
Compare
@@ -24,6 +27,8 @@ type Server struct { | |||
handler mux.HandlerFunc | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
go func() { | ||
errCh <- gocoap.ListenAndServeTCPTLS("udp", s.Address, tlsConfig, s.handler) | ||
errCh <- gocoap.ListenAndServeDTLS("udp", s.Address, dtlsConfig, s.handler) |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
docker/nginx/nginx-key.conf
Outdated
@@ -161,4 +161,22 @@ stream { | |||
} | |||
} | |||
|
|||
# COAP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CoAP
docker/nginx/nginx-x509.conf
Outdated
@@ -151,4 +151,31 @@ stream { | |||
} | |||
} | |||
|
|||
#COAP |
There was a problem hiding this comment.
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 #
908e596
to
d68c3b0
Compare
docker/nginx/nginx-x509.conf
Outdated
@@ -151,4 +151,31 @@ stream { | |||
} | |||
} | |||
|
|||
#CoAP |
There was a problem hiding this comment.
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.
There was a problem hiding this 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}; | ||
} |
There was a problem hiding this comment.
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}; |
There was a problem hiding this comment.
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).
4c6a6c4
to
0e4b393
Compare
LGTM In Future it will nice if coap server have CID support Reference code for ID |
0e4b393
to
ca3976c
Compare
Signed-off-by: 1998-felix <[email protected]>
Signed-off-by: 1998-felix <[email protected]>
Signed-off-by: felix.gateru <[email protected]>
Signed-off-by: felix.gateru <[email protected]>
Signed-off-by: felix.gateru <[email protected]>
Signed-off-by: felix.gateru <[email protected]>
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