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

Fixed a bug w/ emqx bridge #2312

Closed
Ryanauger95 opened this issue Mar 14, 2019 · 3 comments
Assignees
Labels
Milestone

Comments

@Ryanauger95
Copy link

@Ryanauger95 Ryanauger95 commented Mar 14, 2019

Hi, I fixed a bug in the emqx.schema related to bridging.

Using a near-default emqx.conf:

## The Clean start flag of a remote bridge.
##
## Value: boolean
## Default: true
##
## NOTE: Some IoT platforms require clean_start
##       must be set to 'true'
bridge.aws.clean_start = true

## The username for a remote bridge.
##
## Value: String
bridge.aws.username = user

## The password for a remote bridge.
##
## Value: String
bridge.aws.password = passwd

## Mountpoint of the bridge.
##
## Value: String
bridge.aws.mountpoint = bridge/aws/${node}/

## Forward message topics
##
## Value: String
## Example: topic1/#,topic2/#
bridge.aws.forwards = topic1/#,topic2/#

## Bribge to remote server via SSL.
##
## Value: on | off
bridge.aws.ssl = on

## PEM-encoded CA certificates of the bridge.
##
## Value: File
bridge.aws.cacertfile = {{ platform_etc_dir }}/certs/cacert.pem

## Client SSL Certfile of the bridge.
##
## Value: File
bridge.aws.certfile = {{ platform_etc_dir }}/certs/client-cert.pem

## Client SSL Keyfile of the bridge.
##
## Value: File
bridge.aws.keyfile = {{ platform_etc_dir }}/certs/client-key.pem

I was getting errors bridging because the sslopt was a deeply nested list due to a bug in emqx.schema.

{ok,[{aws,[{address,"127.0.0.1:1883"},
           {clean_start,true},
           {client_id,"bridge_aws"},
           {connect_module,emqx_bridge_mqtt},
           {forwards,["topic1/#","topic2/#"]},
           {keepalive,10000},
           {max_inflight_batches,0},
           {mountpoint,"bridge/aws/${node}/"},
           {password,"passwd"},
           {proto_ver,v4},
           {queue,#{}},
           {reconnect_delay_ms,30000},
           {retry_interval,20000},
           {ssl,true},
           {ssl_opts,[{versions,[tlsv1,'tlsv1.1','tlsv1.2']},
                      [{keyfile,"etc/certs/client-key.pem"},
                       [{ciphers,["ECDHE-ECDSA-AES256-GCM-SHA384",
                                  "ECDHE-RSA-AES256-GCM-SHA384"]},
                        [{certfile,"etc/certs/client-cert.pem"},
                         [{cacertfile,"etc/certs/cacert.pem"},[]]]]]]},

It's just one line, change:

 SslOpts = Parse(Opt, Val) ++ [proplists:get_value(ssl_opts, Opts, [])],
to 
 SslOpts = Parse(Opt, Val) ++ proplists:get_value(ssl_opts, Opts, [])

I would happily submit a patch file in the future for stuff like this, or push directly to my own branch and make a pull request, but I don't have access and don't know how you all handle external contribution. Please let me know so that maybe next time I don't make an issue!

@Ryanauger95

This comment has been minimized.

Copy link
Author

@Ryanauger95 Ryanauger95 commented Mar 14, 2019

Hate to keep bothering you all, but I believe I found 2 more bugs relating to the bridge..:

#1 was a Logging bug in emqx_bridge:351
?LOG(info, "Bridge p diconnectednreason=~p", [name(), Conn, Reason]), the ~n needs to be a ~p as there are 3 args.

#2 in emqx_message:set_headers, it doesn't properly handle the case where the headers are undefined due to the is_map() guard. So either we can remove the is_map guard or add a 3rd function overload:
set_headers(_Headers, Msg) ->
Msg;

I'm surprised no one has pointed these out because bridging seems impossible w/o these fixes

@spring2maz

This comment has been minimized.

Copy link
Contributor

@spring2maz spring2maz commented Mar 15, 2019

Thanks for your help!
We have done a major refactoring in bridge recently.
The schema issue was introduce 2 days ago which I do not know much in detail.
the other two embarrassing crashes are due to lack of test coverage.
Sending a PR to fix NOW.

@turtleDeng turtleDeng added this to the 3.1-beta.2 milestone Mar 15, 2019
@turtleDeng turtleDeng added the Support label Mar 15, 2019
terry-xiaoyu added a commit that referenced this issue Mar 16, 2019
* Fix SSL option parser

* Fix bridge disconnect log formatting

* Set no headers if new headers is undefined
@gilbertwong96

This comment has been minimized.

Copy link
Contributor

@gilbertwong96 gilbertwong96 commented Mar 18, 2019

It has been fixed in emqx3.1 beta2. Thanks for your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.