feat: Improve service initialization process #1047

FelixTing · 2022-02-25T09:23:17Z

Start the Web Server before the trigger initialization.
Add retry mechanism for MqttFactory.Create()

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?)
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?)
feat: Update External MQTT Trigger Docs edgex-docs#705

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="test/#"
  ClientId ="app-external-mqtt-trigger"
  Qos            = 0
  KeepAlive      = 10
  Retained       = false
  AutoReconnect  = true
  ConnectTimeout = "30s"
  SkipCertVerify = false
  AuthMode = "clientcert"
  SecretPath = "mqtt"

Run App Service with the configuration and EDGEX_SECURITY_SECRET_STORE=true

Verify logs contain following messages

level=INFO ts=2022-03-03T17:20:58.040262Z app=app- source=mqtt.go:76 msg="Initializing MQTT Trigger"
level=INFO ts=2022-03-03T17:20:58.040284Z app=app- source=server.go:156 msg="Starting HTTP Web Server on address localhost:59706"
level=ERROR ts=2022-03-03T17:20:58.04159Z app=app- source=mqttfactory.go:68 msg="failed to get secret data from the secret provider, error: Error found on handling secrets from underlying data-store: Received a '404' response from the secret store"
level=INFO ts=2022-03-03T17:20:58.04162Z app=app- source=mqttfactory.go:70 msg="Waiting for the secret creation API call to seed the proper credentials..."

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-03T17:47:33.693386Z app=app- source=mqttfactory.go:74 msg="invalid secret data, error: AuthModeCert selected however the key or cert PEM block was not found for secret=mqtt"
level=INFO ts=2022-03-03T17:47:33.693453Z app=app- source=mqttfactory.go:70 msg="Waiting for the secret creation API call to seed the proper credentials..."

Add clientkey to Secret Store

Verify logs contain following messages

level=INFO ts=2022-03-03T17:52:22.404456Z app=app- source=mqtt.go:127 msg="Connecting to mqtt broker for MQTT trigger at: tls://test.mosquitto.org:8884"
level=INFO ts=2022-03-03T17:52:24.14913Z app=app- source=mqtt.go:133 msg="Connected to mqtt server for MQTT trigger"
level=INFO ts=2022-03-03T17:52:24.149168Z app=app- source=service.go:200 msg="StoreAndForward disabled. Not running retry loop."
level=INFO ts=2022-03-03T17:52:24.149183Z app=app- source=service.go:203 msg="app-external-mqtt-trigger has Started"
level=INFO ts=2022-03-03T17:52:24.841152Z app=app- source=mqtt.go:161 msg="Subscribed to topic(s) 'edgex/#' for MQTT trigger"

New Dependency Instructions (If applicable)

codecov-commenter · 2022-02-25T10:09:46Z

Codecov Report

Merging #1047 (2dccacc) into main (18aea91) will decrease coverage by 0.82%.
The diff coverage is 15.51%.

@@            Coverage Diff             @@
##             main    #1047      +/-   ##
==========================================
- Coverage   68.23%   67.41%   -0.83%     
==========================================
  Files          35       35              
  Lines        2827     2863      +36     
==========================================
+ Hits         1929     1930       +1     
- Misses        787      822      +35     
  Partials      111      111

Impacted Files	Coverage Δ
internal/app/service.go	`53.19% <0.00%> (-1.33%)`	⬇️
internal/trigger/mqtt/mqtt.go	`42.10% <0.00%> (-2.34%)`	⬇️
pkg/transforms/mqttsecret.go	`20.68% <0.00%> (ø)`
pkg/secure/mqttfactory.go	`42.25% <19.44%> (-16.94%)`	⬇️
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 18aea91...2dccacc. Read the comment docs.

FelixTing · 2022-03-01T02:40:31Z

@lenny-intel @cloudxxx8 @judehung for awareness.
This draft PR is ready for review. I will add unit tests and update the related docs once the proposed code change is accepted. Thanks!

judehung · 2022-03-01T06:27:32Z

pkg/secure/mqttfactory.go

+		case r := <-ch:
+			if r.err != nil {
+				factory.logger.Errorf("failed to get valid secret data, error: %s. ", r.err)
+				if factory.secretAddedSignal == nil || !secret.IsSecurityEnabled() {
+					return nil, r.err
+				}
+				factory.logger.Debug("Waiting for a SecretAdded signal...")
+			} else {
+				return r.secretData, nil
+			}
+		case <-factory.secretAddedSignal:
+			go f(ch)


Note that if more than one of the channels are ready, go randomly picks which one to receive from. Under such case, is it possible to meet race condition for this select block?

Good catch, thanks. No need to use channel here.

cloudxxx8 · 2022-03-01T09:14:36Z

internal/trigger/mqtt/mqtt.go

@@ -100,12 +102,21 @@ func (trigger *Trigger) Initialize(_ *sync.WaitGroup, _ context.Context, backgro
 	opts.KeepAlive = brokerConfig.KeepAlive
 	opts.Servers = []*url.URL{brokerUrl}

+	var secretAddedSignal chan struct{}
+	var ok bool
+	if secret.IsSecurityEnabled() {


why do you make this check? the secretAddedSignal is created no matter the security is enabled.

@cloudxxx8 , it is no longer created in non-secure mode. It will be nil in non secure mode.

ok, @FelixTing should add the check back

cloudxxx8 · 2022-03-01T15:20:06Z

pkg/secure/mqttfactory.go

+		return secretData, nil
+	}
+	for {
+		factory.logger.Debug("Waiting for a SecretAdded signal...")


print the INFO level log "Waiting for the secret creation API call to seed the proper credentials..."

cloudxxx8

LGTM

internal/app/service.go

lenny-goodell · 2022-03-01T16:09:37Z

internal/app/service.go

@@ -179,6 +184,13 @@ func (svc *Service) MakeItRun() error {
 	// deferred is a function that needs to be called when services exits.
 	svc.addDeferred(deferred)

+	// add a deferred function for the SecretAddedSignal channel created during service initialization.


Suggested change

// add a deferred function for the SecretAddedSignal channel created during service initialization.

// add a deferred function to close the SecretAddedSignal channel created during service initialization.

lenny-goodell · 2022-03-01T16:13:41Z

internal/app/service.go

@@ -484,6 +491,9 @@ func (svc *Service) Initialize() error {
 	svc.ctx.appCtx, svc.ctx.appCancelCtx = context.WithCancel(context.Background())
 	svc.ctx.appWg = &sync.WaitGroup{}

+	secretAddedSignal := make(chan struct{})


Need a buffered channel so code pushing to the channel doesn't block. Linter will fail w/o this. Please make sure you run the linter (may need to install it).

Suggested change

secretAddedSignal := make(chan struct{})

secretAddedSignal := make(chan struct{}, 1)

lenny-goodell · 2022-03-01T16:15:29Z

internal/controller/rest/controller_test.go

@@ -176,7 +176,10 @@ func TestAddSecretRequest(t *testing.T) {
 	mockProvider.On("StoreSecrets", "/mqtt", map[string]string{"password": "password", "username": "username"}).Return(nil)
 	mockProvider.On("StoreSecrets", "/no", map[string]string{"password": "password", "username": "username"}).Return(errors.New("Invalid w/o Vault"))

-	target := NewController(nil, dic, uuid.NewString())
+	ch := make(chan struct{})


Suggested change

ch := make(chan struct{})

ch := make(chan struct{}, 1)

lenny-goodell · 2022-03-01T16:23:20Z

pkg/secure/mqttfactory.go

+	secretData, err := f()
+	if err != nil {
+		factory.logger.Error(err.Error())
+	} else {
+		return secretData, nil
+	}
+	for {
+		factory.logger.Info("Waiting for the secret creation API call to seed the proper credentials...")
+		if _, ok := <-factory.secretAddedSignal; ok {
+			secretData, err := f()
+			if err != nil {
+				factory.logger.Error(err.Error())
+			} else {
+				return secretData, nil
+			}
+		}
+	}
+}


This code should be in the calling function. I.e. it goes beyond get and validate. Then the above code be the body of the func instead of in-line define func.

lenny-goodell · 2022-03-01T16:25:54Z

pkg/secure/mqttfactory.go

+		factory.logger.Info("Waiting for the secret creation API call to seed the proper credentials...")
+		if _, ok := <-factory.secretAddedSignal; ok {
+			secretData, err := f()
+			if err != nil {
+				factory.logger.Error(err.Error())
+			} else {
+				return secretData, nil
+			}
+		}


This should only happen when running in secure mode. Otherwise it should error out because the secret wasn't found in the InsecureSecrets section of the config. Probably shouldn't not create the secretAddedSignal channel unless running in secure mode.

FelixTing · 2022-03-02T07:43:32Z

Updated per all the suggestions.

pkg/secure/mqttfactory.go

lenny-goodell

LGTM, ready to take out of draft and rebase?

FelixTing · 2022-03-03T15:31:51Z

rebased

lenny-goodell · 2022-03-03T15:35:40Z

rebased

@FelixTing , still show This branch is out-of-date with the base branch

FelixTing · 2022-03-03T15:45:58Z

rebased

@FelixTing , still show This branch is out-of-date with the base branch

Uh, have some trouble with my fork repo... will fix this ASAP

- Start the Web Server before the trigger initialization. - Add retry mechanism for MqttFactory.Create() Signed-off-by: Felix Ting <[email protected]>

FelixTing · 2022-03-03T16:16:26Z

rebased

@FelixTing , still show This branch is out-of-date with the base branch

Uh, have some trouble with my fork repo... will fix this ASAP

Sorry about the mess. This branch is now up-to-date with the main branch.

FelixTing · 2022-03-04T06:48:11Z

Added testing instructions and opened a PR for docs.

lenny-goodell

LGTM

…)" This reverts commit 6bcd8b5.

This reverts commit 6bcd8b5.

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

* revert: Improve service initialization process (#1047) This reverts commit 6bcd8b5. * fix: Improve service initialization process - 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-1046 branch from 65cbdfe to d223859 Compare February 25, 2022 10:00

judehung reviewed Mar 1, 2022

View reviewed changes

FelixTing force-pushed the issue-1046 branch from d223859 to d11a6c1 Compare March 1, 2022 11:42

cloudxxx8 requested changes Mar 1, 2022

View reviewed changes

cloudxxx8 previously approved these changes Mar 1, 2022

View reviewed changes

lenny-goodell requested changes Mar 1, 2022

View reviewed changes

FelixTing dismissed cloudxxx8’s stale review via 91fb470 March 2, 2022 06:45

FelixTing force-pushed the issue-1046 branch from 91fb470 to 2dccacc Compare March 2, 2022 07:21

FelixTing requested a review from lenny-goodell March 2, 2022 07:43

lenny-goodell reviewed Mar 2, 2022

View reviewed changes

pkg/secure/mqttfactory.go Show resolved Hide resolved

lenny-goodell previously approved these changes Mar 3, 2022

View reviewed changes

FelixTing dismissed lenny-goodell’s stale review via 2dc7f65 March 3, 2022 15:29

FelixTing force-pushed the issue-1046 branch from a7cb416 to 2dc7f65 Compare March 3, 2022 15:29

FelixTing marked this pull request as ready for review March 3, 2022 15:29

FelixTing force-pushed the issue-1046 branch from 2dc7f65 to 64a95ea Compare March 3, 2022 15:42

feat: Improve service initialization process

7929a1c

- Start the Web Server before the trigger initialization. - Add retry mechanism for MqttFactory.Create() Signed-off-by: Felix Ting <[email protected]>

FelixTing force-pushed the issue-1046 branch from 64a95ea to 7929a1c Compare March 3, 2022 15:58

FelixTing requested review from lenny-goodell, cloudxxx8 and judehung March 4, 2022 06:48

lenny-goodell approved these changes Mar 7, 2022

View reviewed changes

lenny-goodell merged commit 6bcd8b5 into edgexfoundry:main Mar 7, 2022

judehung mentioned this pull request Mar 11, 2022

fix: improper use of secretAddedSignal channel #1054

Merged

5 tasks

FelixTing added a commit to FelixTing/app-functions-sdk-go that referenced this pull request Mar 14, 2022

Revert "feat: Improve service initialization process (edgexfoundry#1047…

a4c8176

…)" This reverts commit 6bcd8b5.

FelixTing added a commit to FelixTing/app-functions-sdk-go that referenced this pull request Mar 14, 2022

revert: Improve service initialization process (edgexfoundry#1047)

97e05fd

This reverts commit 6bcd8b5.

FelixTing added a commit to FelixTing/app-functions-sdk-go that referenced this pull request Mar 14, 2022

revert: Improve service initialization process (edgexfoundry#1047)

e34d653

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Improve service initialization process #1047

feat: Improve service initialization process #1047

FelixTing commented Feb 25, 2022 •

edited

Loading

codecov-commenter commented Feb 25, 2022 •

edited

Loading

FelixTing commented Mar 1, 2022

judehung Mar 1, 2022

FelixTing Mar 1, 2022

cloudxxx8 Mar 1, 2022

lenny-goodell Mar 2, 2022

cloudxxx8 Mar 2, 2022

cloudxxx8 Mar 1, 2022

cloudxxx8 left a comment

lenny-goodell Mar 1, 2022

lenny-goodell Mar 1, 2022

lenny-goodell Mar 1, 2022

lenny-goodell Mar 1, 2022

lenny-goodell Mar 1, 2022

FelixTing commented Mar 2, 2022

lenny-goodell left a comment

FelixTing commented Mar 3, 2022

lenny-goodell commented Mar 3, 2022

FelixTing commented Mar 3, 2022

FelixTing commented Mar 3, 2022

FelixTing commented Mar 4, 2022

lenny-goodell left a comment

	// add a deferred function for the SecretAddedSignal channel created during service initialization.
	// add a deferred function to close the SecretAddedSignal channel created during service initialization.

	secretAddedSignal := make(chan struct{})
	secretAddedSignal := make(chan struct{}, 1)

feat: Improve service initialization process #1047

feat: Improve service initialization process #1047

Conversation

FelixTing commented Feb 25, 2022 • edited Loading

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

codecov-commenter commented Feb 25, 2022 • edited Loading

Codecov Report

FelixTing commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudxxx8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelixTing commented Mar 2, 2022

lenny-goodell left a comment

Choose a reason for hiding this comment

FelixTing commented Mar 3, 2022

lenny-goodell commented Mar 3, 2022

FelixTing commented Mar 3, 2022

FelixTing commented Mar 3, 2022

FelixTing commented Mar 4, 2022

lenny-goodell left a comment

Choose a reason for hiding this comment

FelixTing commented Feb 25, 2022 •

edited

Loading

codecov-commenter commented Feb 25, 2022 •

edited

Loading