fix: improper use of secretAddedSignal channel #1054

FelixTing · 2022-03-10T18:14:40Z

Revert #1047
Implement the retry mechanism inside the External MQTT Trigger.

Signed-off-by: Felix Ting [email protected]

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/app-functions-sdk-go/blob/main/.github/CONTRIBUTING.md

PR Checklist

Please check if your PR fulfills the following requirements:

I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
I am not introducing a new dependency (add notes below if you are)
I have added unit tests for the new feature or bug fix (if not, why?) Requires integration tests
I have fully tested (add details below) this the new feature or bug fix (if not, why?)
I have opened a PR for the related docs change (if not, why?)
fix: Update External MQTT Trigger Docs edgex-docs#710

Testing Instructions

Download this Docker Compose file

Edit the Docker Compose file to add the custom App Service's service key to EdgeX service secretstore-setup's ADD_SECRETSTORE_TOKENS environment variable.

  secretstore-setup:
    container_name: edgex-security-secretstore-setup
    depends_on:
    - security-bootstrapper
    - vault
    environment:
      ADD_KNOWN_SECRETS: redisdb[app-rules-engine],redisdb[device-rest],redisdb[device-virtual]
      ADD_SECRETSTORE_TOKENS: 'app-external-mqtt-trigger'

Run EdgeX Foundry
docker-compose -f docker-compose.yml up -d

Download this App Service configuration
Modify [Trigger] section with the following settings:

[Trigger]
Type="external-mqtt"
  [Trigger.externalmqtt]
  Url = "tls://test.mosquitto.org:8884"
  SubscribeTopics="edgex/#"
  ClientId ="app-external-mqtt-trigger"
  Qos            = 0
  KeepAlive      = 10
  Retained       = false
  AutoReconnect  = true
  ConnectTimeout = "30s"
  SkipCertVerify = false
  AuthMode = "clientcert"
  SecretPath = "mqtt"
  RetryDuration = 30
  RetryInterval = 5

Run App Service with the configuration and EDGEX_SECURITY_SECRET_STORE=true

Verify logs contain following messages

level=INFO ts=2022-03-14T05:15:54.536761Z app=app- source=mqtt.go:83 msg="Initializing MQTT Trigger"
level=INFO ts=2022-03-14T05:15:54.536801Z app=app- source=server.go:156 msg="Starting HTTP Web Server on address localhost:59706"
level=ERROR ts=2022-03-14T05:15:54.538563Z app=app- source=mqtt.go:124 msg="unable to create secure MQTT Client: Error found on handling secrets from underlying data-store: Received a '404' response from the secret store. Attempt to create MQTT client again after 5 seconds..."

Wait 30 seconds and verify logs contain following messages

level=ERROR ts=2022-03-14T05:15:54.538563Z app=app- source=mqtt.go:124 msg="unable to create secure MQTT Client: Error found on handling secrets from underlying data-store: Received a '404' response from the secret store. Attempt to create MQTT client again after 5 seconds..."
level=ERROR ts=2022-03-14T05:15:59.543072Z app=app- source=mqtt.go:124 msg="unable to create secure MQTT Client: Error found on handling secrets from underlying data-store: Received a '404' response from the secret store. Attempt to create MQTT client again after 5 seconds..."
level=ERROR ts=2022-03-14T05:16:04.546163Z app=app- source=mqtt.go:124 msg="unable to create secure MQTT Client: Error found on handling secrets from underlying data-store: Received a '404' response from the secret store. Attempt to create MQTT client again after 5 seconds..."
level=ERROR ts=2022-03-14T05:16:09.552204Z app=app- source=mqtt.go:124 msg="unable to create secure MQTT Client: Error found on handling secrets from underlying data-store: Received a '404' response from the secret store. Attempt to create MQTT client again after 5 seconds..."
level=ERROR ts=2022-03-14T05:16:14.555807Z app=app- source=mqtt.go:124 msg="unable to create secure MQTT Client: Error found on handling secrets from underlying data-store: Received a '404' response from the secret store. Attempt to create MQTT client again after 5 seconds..."
level=ERROR ts=2022-03-14T05:16:19.562778Z app=app- source=mqtt.go:124 msg="unable to create secure MQTT Client: Error found on handling secrets from underlying data-store: Received a '404' response from the secret store. Attempt to create MQTT client again after 5 seconds..."
level=ERROR ts=2022-03-14T05:16:24.565683Z app=app- source=service.go:190 msg="unable to create MQTT Client: unable to create secure MQTT Client: Error found on handling secrets from underlying data-store: Received a '404' response from the secret store"
level=ERROR ts=2022-03-14T05:16:24.565721Z app=app- source=main.go:64 msg="MakeItRun returned error: failed to initialize Trigger"

Restart App Service.
Generate a TLS client certificate for test.mosquitto.org, refer to https://test.mosquitto.org/ssl/

Add clientcert to Secret Store using this App Service API

Verify logs contain following messages

level=ERROR ts=2022-03-14T05:17:37.329887Z app=app- source=mqtt.go:124 msg="unable to create secure MQTT Client: AuthModeCert selected however the key or cert PEM block was not found for secret=mqtt. Attempt to create MQTT client again after 5 seconds..."

Add clientcert and clientkey to Secret Store

Verify logs contain following messages

level=INFO ts=2022-03-14T05:17:42.331827Z app=app- source=mqtt.go:250 msg="Connecting to mqtt broker for MQTT trigger at: tls://104.171.201.135:8884"
level=INFO ts=2022-03-14T05:17:43.053758Z app=app- source=mqtt.go:256 msg="Connected to mqtt server for MQTT trigger"
level=INFO ts=2022-03-14T05:17:43.053796Z app=app- source=service.go:200 msg="StoreAndForward disabled. Not running retry loop."
level=INFO ts=2022-03-14T05:17:43.053809Z app=app- source=service.go:203 msg="app-external-mqtt-trigger has Started"
level=INFO ts=2022-03-14T05:17:43.229374Z app=app- source=mqtt.go:161 msg="Subscribed to topic(s) 'edgex/#' for MQTT trigger"

New Dependency Instructions (If applicable)

FelixTing · 2022-03-10T18:17:33Z

On a second thought, for code readability, conciseness, and maintainability, it might be worth eliminating the use of channel and simplifying the retry mechanism in the MqttFactory.Create function (e.g. periodic retry)
@lenny-intel @cloudxxx8 @judehung What do you think?

lenny-goodell · 2022-03-10T19:06:29Z

What do you think?

@FelixTing , yes I agree. This is simpler and less impact on other areas. Needs to have a duration that if exceeded it fails out.

judehung · 2022-03-11T06:09:56Z

On a second thought, for code readability, conciseness, and maintainability, it might be worth eliminating the use of channel and simplifying the retry mechanism in the MqttFactory.Create function (e.g. periodic retry) @lenny-intel @cloudxxx8 @judehung What do you think?

I also prefer to switch to retry mechanism instead of using a channel. In the channel approach of #1047, the only receiver of the channel is MqttFactory itself, and the channel should be closed or ditched right after MqttFactory passes the secrets validation. However, per golang's suggestion, only the sender should close a channel, never the receiver, so it's against channel closing principle to close the channel by MqttFactory. Moreover, given that #1047 is to ensure that external MQTT trigger can retrieve secrets during initialization, the impact of code changes should apply to the case of using external MQTT trigger only. Putting a channel in the PostSecrets controller have broader impact and may lead to unintentional behaviors that we fail to consider right now. As a result, implementing a retry mechanism inside the external MQTT trigger may be a better approach to fix #1046.

cloudxxx8 · 2022-03-11T10:21:21Z

ok, it's too complicated in this simple case. let's use the retry mechanism instead.

This reverts commit 6bcd8b5. Signed-off-by: Felix Ting <[email protected]>

codecov-commenter · 2022-03-14T02:08:33Z

Codecov Report

Merging #1054 (6d2e93a) into main (c4fff4f) will increase coverage by 0.03%.
The diff coverage is 16.00%.

@@            Coverage Diff             @@
##             main    #1054      +/-   ##
==========================================
+ Coverage   68.22%   68.25%   +0.03%     
==========================================
  Files          35       35              
  Lines        2867     2848      -19     
==========================================
- Hits         1956     1944      -12     
+ Misses        800      793       -7     
  Partials      111      111

Impacted Files	Coverage Δ
internal/app/service.go	`54.01% <0.00%> (+1.30%)`	⬆️
internal/trigger/mqtt/mqtt.go	`38.40% <0.00%> (-3.71%)`	⬇️
pkg/transforms/mqttsecret.go	`20.68% <0.00%> (ø)`
pkg/secure/mqttfactory.go	`59.18% <42.85%> (+1.43%)`	⬆️
internal/webserver/server.go	`22.58% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4fff4f...6d2e93a. Read the comment docs.

- Start webserver before trigger initialization - Add retry mechanism for External MQTT Trigger initialization Signed-off-by: Felix Ting <[email protected]>

cloudxxx8

LGTM

lenny-goodell

A couple minor things

lenny-goodell · 2022-03-14T15:31:36Z

internal/trigger/mqtt/mqtt.go

+	timer := startup.NewTimer(brokerConfig.RetryDuration, brokerConfig.RetryInterval)
+	for timer.HasNotElapsed() {
+		if mqttClient, err = createMqttClient(sp, lc, brokerConfig, opts); err != nil {
+			lc.Errorf("%s. Attempt to create MQTT client again after %d seconds...", err.Error(), brokerConfig.RetryInterval)


Suggested change

lc.Errorf("%s. Attempt to create MQTT client again after %d seconds...", err.Error(), brokerConfig.RetryInterval)

lc.Warnf("%s. Attempt to create MQTT client again after %d seconds...", err.Error(), brokerConfig.RetryInterval)

lenny-goodell · 2022-03-14T15:36:57Z

internal/trigger/mqtt/mqtt.go

+		return nil, fmt.Errorf("unable to create secure MQTT Client: %s", err.Error())
+	}
+
+	brokerUrl, err := url.Parse(config.Url)


Remove. This is already verified in Initialize()

Signed-off-by: Felix Ting <[email protected]>

lenny-goodell

LGTM

FelixTing force-pushed the issue-1052 branch 2 times, most recently from c232fbe to 0d3f1dc Compare March 14, 2022 01:53

revert: Improve service initialization process (edgexfoundry#1047)

e34d653

This reverts commit 6bcd8b5. Signed-off-by: Felix Ting <[email protected]>

FelixTing force-pushed the issue-1052 branch from 0d3f1dc to 5511c62 Compare March 14, 2022 01:57

FelixTing marked this pull request as ready for review March 14, 2022 01:59

FelixTing marked this pull request as draft March 14, 2022 02:45

FelixTing force-pushed the issue-1052 branch 3 times, most recently from 79d8fa5 to 70ff995 Compare March 14, 2022 04:30

FelixTing marked this pull request as ready for review March 14, 2022 04:47

fix: Improve service initialization process

1e2cbf3

- Start webserver before trigger initialization - Add retry mechanism for External MQTT Trigger initialization Signed-off-by: Felix Ting <[email protected]>

FelixTing force-pushed the issue-1052 branch from 70ff995 to 1e2cbf3 Compare March 14, 2022 05:20

cloudxxx8 previously approved these changes Mar 14, 2022

View reviewed changes

lenny-goodell requested changes Mar 14, 2022

View reviewed changes

fix: Update code per review comments

6d2e93a

Signed-off-by: Felix Ting <[email protected]>

FelixTing dismissed cloudxxx8’s stale review via 6d2e93a March 15, 2022 01:21

FelixTing requested a review from lenny-goodell March 15, 2022 01:22

lenny-goodell approved these changes Mar 15, 2022

View reviewed changes

lenny-goodell merged commit 50cceb0 into edgexfoundry:main Mar 15, 2022

FelixTing deleted the issue-1052 branch March 15, 2022 16:41

lenny-goodell linked an issue Mar 15, 2022 that may be closed by this pull request

App service with MQTT export received event, but it is not exported to MQTT broker #1053

Closed

lenny-goodell added this to the Kamakura milestone Mar 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improper use of secretAddedSignal channel #1054

fix: improper use of secretAddedSignal channel #1054

FelixTing commented Mar 10, 2022 •

edited

Loading

FelixTing commented Mar 10, 2022

lenny-goodell commented Mar 10, 2022

judehung commented Mar 11, 2022

cloudxxx8 commented Mar 11, 2022

codecov-commenter commented Mar 14, 2022 •

edited

Loading

cloudxxx8 left a comment

lenny-goodell left a comment

lenny-goodell Mar 14, 2022

lenny-goodell Mar 14, 2022

lenny-goodell left a comment

	lc.Errorf("%s. Attempt to create MQTT client again after %d seconds...", err.Error(), brokerConfig.RetryInterval)
	lc.Warnf("%s. Attempt to create MQTT client again after %d seconds...", err.Error(), brokerConfig.RetryInterval)

fix: improper use of secretAddedSignal channel #1054

fix: improper use of secretAddedSignal channel #1054

Conversation

FelixTing commented Mar 10, 2022 • edited Loading

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

FelixTing commented Mar 10, 2022

lenny-goodell commented Mar 10, 2022

judehung commented Mar 11, 2022

cloudxxx8 commented Mar 11, 2022

codecov-commenter commented Mar 14, 2022 • edited Loading

Codecov Report

cloudxxx8 left a comment

Choose a reason for hiding this comment

lenny-goodell left a comment

Choose a reason for hiding this comment

lenny-goodell Mar 14, 2022

Choose a reason for hiding this comment

lenny-goodell Mar 14, 2022

Choose a reason for hiding this comment

lenny-goodell left a comment

Choose a reason for hiding this comment

FelixTing commented Mar 10, 2022 •

edited

Loading

codecov-commenter commented Mar 14, 2022 •

edited

Loading