feat: Job String Processor #234

merlin-northern · 2022-09-04T10:42:44Z

Changelog: Title
Ticket: MEN-5819
Signed-off-by: Peter Grzybowski peter@northern.tech

mender-test-bot · 2022-09-04T10:43:02Z

@merlin-northern, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".

my commands and options

You can trigger a pipeline on multiple prs with:

mentioning me and start pipeline --pr mender/127 --pr mender-connect/255

You can start a fast pipeline, disabling full integration tests with:

mentioning me and start pipeline --fast

You can cherry pick to a given branch or branches with:

mentioning me and:

 cherry-pick to:
 * 1.0.x
 * 2.0.x

merlin-northern · 2022-09-04T10:45:51Z

@mender-test-bot start pipeline please

mender-test-bot · 2022-09-04T10:46:16Z

Hello 😸 I created a pipeline for you here: Pipeline-630739572

Build Configuration Matrix

Key	Value
AUDITLOGS_REV	master
BUILD_CLIENT	false
CREATE_ARTIFACT_WORKER_REV	master
DEPLOYMENTS_ENTERPRISE_REV	master
DEPLOYMENTS_REV	master
DEVICEAUTH_ENTERPRISE_REV	master
DEVICEAUTH_REV	master
DEVICECONFIG_REV	master
DEVICECONNECT_REV	master
DEVICEMONITOR_REV	master
GUI_REV	master
INTEGRATION_REV	master
INVENTORY_ENTERPRISE_REV	master
INVENTORY_REV	master
IOT_MANAGER_REV	master
MENDER_ARTIFACT_REV	master
MENDER_BINARY_DELTA_REV	master
MENDER_CLI_REV	master
MENDER_CONFIGURE_MODULE_REV	master
MENDER_CONNECT_REV	master
MENDER_CONVERT_REV	master
MENDER_GATEWAY_REV	master
MENDER_REV	master
MONITOR_CLIENT_REV	master
MTLS_AMBASSADOR_REV	master
REPORTING_REV	master
RUN_BACKEND_INTEGRATION_TESTS	true
RUN_INTEGRATION_TESTS	true
TENANTADM_REV	master
USERADM_ENTERPRISE_REV	master
USERADM_REV	master
WORKFLOWS_ENTERPRISE_REV	master
WORKFLOWS_REV	pull/234/head

merlin-northern · 2022-09-04T18:31:17Z

@mender-test-bot start pipeline please

mender-test-bot · 2022-09-04T18:31:44Z

Hello 😸 I created a pipeline for you here: Pipeline-630863595

Build Configuration Matrix

Key	Value
AUDITLOGS_REV	master
BUILD_CLIENT	false
CREATE_ARTIFACT_WORKER_REV	master
DEPLOYMENTS_ENTERPRISE_REV	master
DEPLOYMENTS_REV	master
DEVICEAUTH_ENTERPRISE_REV	master
DEVICEAUTH_REV	master
DEVICECONFIG_REV	master
DEVICECONNECT_REV	master
DEVICEMONITOR_REV	master
GUI_REV	master
INTEGRATION_REV	master
INVENTORY_ENTERPRISE_REV	master
INVENTORY_REV	master
IOT_MANAGER_REV	master
MENDER_ARTIFACT_REV	master
MENDER_BINARY_DELTA_REV	master
MENDER_CLI_REV	master
MENDER_CONFIGURE_MODULE_REV	master
MENDER_CONNECT_REV	master
MENDER_CONVERT_REV	master
MENDER_GATEWAY_REV	master
MENDER_REV	master
MONITOR_CLIENT_REV	master
MTLS_AMBASSADOR_REV	master
REPORTING_REV	master
RUN_BACKEND_INTEGRATION_TESTS	true
RUN_INTEGRATION_TESTS	true
TENANTADM_REV	master
USERADM_ENTERPRISE_REV	master
USERADM_REV	master
WORKFLOWS_ENTERPRISE_REV	master
WORKFLOWS_REV	pull/234/head

merlin-northern · 2022-09-04T18:32:13Z

@tranchitella could I kindly ask you to take a look, since you already reviewed the first try (the quck fix from the past weekend in ent)

tranchitella · 2022-09-05T04:22:03Z

app/worker/process.go

-		l.Infof("processTask: calling http task: %s %s", httpTask.Method,
-			processJobString(httpTask.URI, workflow, job))


Why did you remove this log?

I moved it below

it was doubling the very expensive operation of processing the job string, just for printing.

tranchitella · 2022-09-05T04:23:21Z

app/worker/http.go

+	l.Infof("processHTTPTask: starting with: method=%s uri=%s (options=%+v)",
+		httpTask.Method,
+		uri,
+		options,
+	)


I don't think logging option is valuable, as it will always be Encoding: URL.
By the way, I don't get why we need this log here.

I can remove it, it was moved from one level up (see the previous comment). do you want me to remove it?

tranchitella · 2022-09-05T04:40:18Z

app/worker/http.go

-			value = processJobString(value, workflow, job)
-			value = maybeExecuteGoTemplate(value, job.InputParameters.Map())
+			key = ps.ProcessJobString(key, nil)
+			key = maybeExecuteGoTemplate(key, ps.GetJobInputParameters().Map())


It looks weird obtaining the job's input parameters from a GetJobInputParameters method on the JobStringProcessor instance. It looks like you did it by convenience because processHTTPTask doesn't receive anymore the job, but honestly I don't like it too much.

I know, but there is no good way of going it without a total re-desing. would you prefer ps.GetJob() method instead?

@alfrunes there is a bit of refactoring going on here (I could not stop) in order not to redesign the whole thing, I added GetJobInputParameters in the string processor class. we (Fabio and me) both think it is not very elegant, and a bit out of place. what do you think? (Keep in mind that we do not want to go over board here :>)

Since maybeExecuteGoTemplate is called wherever ps.ProcessJobString is called; wouldn't it be easier to just move it inside the processor package?

brilliant. so I created a method MaybeExecuteGoTemplate but there were complications, in the end, this is the final version. makes sense? beware: I can refactor deeper.

tranchitella · 2022-09-05T04:41:55Z

app/worker/process.go

-		l.Infof("processTask: calling http task: %s %s", httpTask.Method,
-			processJobString(httpTask.URI, workflow, job))


why did you remove this log and moved it in HTTPTask?

it was super expensive to process the job string twice, here for only logging and then again right after. so moving it was a saver and did not change anything, as we enter the function anyway.

tranchitella · 2022-09-05T04:47:23Z

app/processor/string.go

+	}
+}
+
+func (j *JobStringProcessor) ProcessJobString(data string, options *Options) string {


If you use a variadic function parameter, you can save the , nil where you don't need otpions:

Suggested change

func (j *JobStringProcessor) ProcessJobString(data string, options *Options) string {

func (j *JobStringProcessor) ProcessJobString(data string, opts ...*Options) string {

At this point, you can loop over options (not tested):

options := Options{Encoding: Plain} for _, o := range opts { options.Encoding = o.Encoding }

If you want to spend a bit more time, you can use the functional options pattern:
https://www.sohamkamani.com/golang/options-pattern/

I was following your previous suggestions :) I like the .... the functional options: nice! I have never used it. I think ... is nice here. is it ok now?

will you believe it? app/processor/string.go:67:1: cyclomatic complexity 21 of func (*JobStringProcessor).ProcessJobString is high (> 20) (gocyclo) there is always something.

merlin-northern · 2022-09-05T19:50:41Z

@mender-test-bot start pipeline please sugar

mender-test-bot · 2022-09-05T19:51:08Z

Hello 😸 I created a pipeline for you here: Pipeline-631733398

Build Configuration Matrix

Key	Value
AUDITLOGS_REV	master
BUILD_CLIENT	false
CREATE_ARTIFACT_WORKER_REV	master
DEPLOYMENTS_ENTERPRISE_REV	master
DEPLOYMENTS_REV	master
DEVICEAUTH_ENTERPRISE_REV	master
DEVICEAUTH_REV	master
DEVICECONFIG_REV	master
DEVICECONNECT_REV	master
DEVICEMONITOR_REV	master
GUI_REV	master
INTEGRATION_REV	master
INVENTORY_ENTERPRISE_REV	master
INVENTORY_REV	master
IOT_MANAGER_REV	master
MENDER_ARTIFACT_REV	master
MENDER_BINARY_DELTA_REV	master
MENDER_CLI_REV	master
MENDER_CONFIGURE_MODULE_REV	master
MENDER_CONNECT_REV	master
MENDER_CONVERT_REV	master
MENDER_GATEWAY_REV	master
MENDER_REV	master
MONITOR_CLIENT_REV	master
MTLS_AMBASSADOR_REV	master
REPORTING_REV	master
RUN_BACKEND_INTEGRATION_TESTS	true
RUN_INTEGRATION_TESTS	true
TENANTADM_REV	master
USERADM_ENTERPRISE_REV	master
USERADM_REV	master
WORKFLOWS_ENTERPRISE_REV	master
WORKFLOWS_REV	pull/234/head

alfrunes · 2022-09-06T06:39:10Z

Is this a feature? I would argue that this is a refactor of existing functionality.

alfrunes · 2022-09-06T06:40:19Z

app/processor/job.go

+type JsonOptions struct {
+}


I don't understand the purpose of this.

this is on purpose: to keep the interface consistent. in case there is a new round of refactoring (i.e.: a general processor interface) can we keep it?

alfrunes · 2022-09-06T06:46:57Z

app/processor/job.go

+func (j JobProcessor) ProcessJSON(
+	data interface{},
+	ps *JobStringProcessor,
+	options ...*JsonOptions,


This variadic argument is unused.

this is on purpose: to keep the interface consistent. in case there is a new round of refactoring (i.e.: a general processor interface) can we keep it?

merlin-northern · 2022-09-06T07:00:19Z

Is this a feature? I would argue that this is a refactor of existing functionality.

something in between feat, refact, fix, I will change to fix, since the ticket is about a bug. makes sense?

alfrunes · 2022-09-06T13:46:51Z

app/worker/http.go

+	l *log.Logger,
+) (*model.TaskResult, error) {
+	options := &processor.Options{Encoding: processor.URL}
+	uri := ps.ProcessJobString(httpTask.URI, options)


This is going to break certain URLs. For example if the url contains a host:port address. To be honest, I think the query escape should be done at the templating level and thus done as part of the workflow definition.

wait how can it be? it only URL-encodes the value:

app/processor/string.go:97

value = url.QueryEscape(value)

This transforms all of the following characters: '$', '&', '+', ',', '/', ':', ';', '=', '?', '@' many of them are valid in different parts of the schema. For example

url.QueryEscape("https://hosted.mender.io") // -> https%3A%2F%2Fhosted.mender.io

but the URL is never a workflow input variable, we take it only from env and with default values ("uri": "http://${env.USERADM_ADDR|mender-useradm:8080}/api/internal/v1/useradm/tokens?tenant_id=${workflow.input.tenant_id}"). this assumes that we should encode the workflow input variables present in the URL (${workflow.input.tenant_id}).

ok, how about now? we support cool encoding=plain|url;identifier syntax that overrides everything you may pass. if you do not pass it, by default we URL encode all input variables that exist in the URL.

@tranchitella I hope you are happy that you used Alf as a mole to have this feature :>

Changelog: Title Ticket: MEN-5819 Signed-off-by: Peter Grzybowski <peter@northern.tech>

merlin-northern · 2022-09-07T11:23:48Z

@mender-test-bot start pipeline please sugar pretty please

mender-test-bot · 2022-09-07T11:24:21Z

Hello 😸 I created a pipeline for you here: Pipeline-633418905

Build Configuration Matrix

Key	Value
AUDITLOGS_REV	master
BUILD_CLIENT	false
CREATE_ARTIFACT_WORKER_REV	master
DEPLOYMENTS_ENTERPRISE_REV	master
DEPLOYMENTS_REV	master
DEVICEAUTH_ENTERPRISE_REV	master
DEVICEAUTH_REV	master
DEVICECONFIG_REV	master
DEVICECONNECT_REV	master
DEVICEMONITOR_REV	master
GUI_REV	master
INTEGRATION_REV	master
INVENTORY_ENTERPRISE_REV	master
INVENTORY_REV	master
IOT_MANAGER_REV	master
MENDER_ARTIFACT_REV	master
MENDER_BINARY_DELTA_REV	master
MENDER_CLI_REV	master
MENDER_CONFIGURE_MODULE_REV	master
MENDER_CONNECT_REV	master
MENDER_CONVERT_REV	master
MENDER_GATEWAY_REV	master
MENDER_REV	master
MONITOR_CLIENT_REV	master
MTLS_AMBASSADOR_REV	master
REPORTING_REV	master
RUN_BACKEND_INTEGRATION_TESTS	true
RUN_INTEGRATION_TESTS	true
TENANTADM_REV	master
USERADM_ENTERPRISE_REV	master
USERADM_REV	master
WORKFLOWS_ENTERPRISE_REV	master
WORKFLOWS_REV	pull/234/head

alfrunes

Take the following suggestions with a grain of 🧂.
As a minimum, I would refine the regular expression for the flags/options. The rest are more suggestions for refactoring.

alfrunes · 2022-09-07T12:44:03Z

app/processor/string.go

+	workflowEnvVariable   = "env."
+	workflowInputVariable = "workflow.input."
+	regexVariable         = `\$\{([^\}\|]+)(?:\|([^\}]+))?}`
+	regexVariableFlags    = `(?:([a-zA-Z]+)+=([a-zA-Z0-9]+);)([^\}\|]+)(?:\|([^\}]+))?`


It took me some time to digest the above regex if I'm being honest. I found one extra + quantifier and a missing semicolon in the "ignore" character set for the variable name. However, for the sake of readability and allowing multiple options I would name the capturing groups (and also allow matching multiple options ;).

Suggested change

regexVariableFlags = `(?:([a-zA-Z]+)+=([a-zA-Z0-9]+);)([^\}\|]+)(?:\|([^\}]+))?`

regexVariableFlags = `(?P<options>(?:(?:[a-zA-Z]+)=(?:[a-zA-Z0-9]+);)+)(?P<name>[^;\}\|]+)(?:\|(?P<default>[^\}]+))?`

Wait, now we almost have an expression for the entire variable, we only need the brackets and change the options quantifier from + to *.

alfrunes · 2022-09-07T12:47:41Z

app/processor/string.go

+const (
+	workflowEnvVariable   = "env."
+	workflowInputVariable = "workflow.input."
+	regexVariable         = `\$\{([^\}\|]+)(?:\|([^\}]+))?}`


Following from my regexVariableFlags suggestion.

Suggested change

regexVariable = `\$\{([^\}\|]+)(?:\|([^\}]+))?}`

regexVariable = `\$\{(?P<options>(?:(?:[a-zA-Z]+)=(?:[a-zA-Z0-9]+);)*)(?P<name>[^;\}\|]+)(?:\|(?P<default>[^\}]+))?}`

This require manually splitting the option key/value pairs.

alfrunes · 2022-09-07T12:58:42Z

app/processor/string.go

+		expectedMatches      = 4
+		flagMatchIndex       = 1
+		flagValueMatchIndex  = 2
+		identifierMatchIndex = 3


it took me some time before I understood what these were...

alfrunes · 2022-09-07T13:58:45Z

app/processor/string.go

+		// now it is possible to override the encoding with flags: ${encoding=plain;identifier}
+		// if encoding is supplied via flags, it takes precedence, we return the match
+		// without the flags, otherwise fail back to original match and encoding
+		match, option.Encoding = getEncodingFromData(match, option.Encoding)


Can we apply the encoding options to all replacements? (See following suggestions)

alfrunes · 2022-09-07T13:59:00Z

app/processor/string.go

+		// if encoding is supplied via flags, it takes precedence, we return the match
+		// without the flags, otherwise fail back to original match and encoding
+		match, option.Encoding = getEncodingFromData(match, option.Encoding)
+		defaultValue := submatch[2]


Suggested change

defaultValue := submatch[2]

value := submatch[2]

alfrunes · 2022-09-07T13:59:55Z

app/processor/string.go

+			if !found {
+				data = strings.ReplaceAll(data, submatch[0], defaultValue)
+			}


Suggested change

if !found {

data = strings.ReplaceAll(data, submatch[0], defaultValue)

}

alfrunes · 2022-09-07T14:00:56Z

app/processor/string.go

+			envValue := os.Getenv(envName)
+			if envValue == "" {
+				envValue = defaultValue
+			}
+			data = strings.ReplaceAll(data, submatch[0], envValue)


Suggested change

envValue := os.Getenv(envName)

if envValue == "" {

envValue = defaultValue

}

data = strings.ReplaceAll(data, submatch[0], envValue)

if envValue := os.Getenv(envName); envValue != "" {

value = envValue

}

alfrunes · 2022-09-07T14:03:55Z

app/processor/string.go

+					if err == nil {
+						if varValueString == "" {
+							varValueString = defaultValue
+						}
+						data = strings.ReplaceAll(data, submatch[0], varValueString)
+					}


Suggested change

if err == nil {

if varValueString == "" {

varValueString = defaultValue

}

data = strings.ReplaceAll(data, submatch[0], varValueString)

}

if err == nil {

if varValueString != "" {

value = varValueString

}

} else {

continue SubmatchLoop

}

alfrunes · 2022-09-07T14:04:22Z

app/processor/string.go

+	option := getOption(options...)
+
+	// search for ${...} expressions in the data string
+	for _, submatch := range matches {


Suggested change

for _, submatch := range matches {

SubmatchLoop:

for _, submatch := range matches {

alfrunes · 2022-09-07T14:05:18Z

app/processor/string.go

+					break
+				}
+			}
+		}


Finally, this applies the option to all the replacements above.

Suggested change

}

}

if option.Encoding == URL {

value = url.QueryEscape(value)

}

data = strings.ReplaceAll(data, submatch[0], value)

merlin-northern · 2022-09-07T16:45:22Z

@alfrunes I moved to this PR where I merged your commit: #235 I fixed the units, and added tests for a new processOptionString, which I had to rewrite as it was not working.

merlin-northern · 2022-09-09T10:29:39Z

this one went with #235

merlin-northern force-pushed the men_5819_escape_query_params_processor branch from ebca634 to fb79f7e Compare September 4, 2022 10:45

mendersoftware deleted a comment from mender-test-bot Sep 4, 2022

merlin-northern force-pushed the men_5819_escape_query_params_processor branch 3 times, most recently from 571aeb4 to 6eb3278 Compare September 4, 2022 12:03

merlin-northern requested a review from tranchitella September 4, 2022 18:31

tranchitella reviewed Sep 5, 2022

View reviewed changes

merlin-northern force-pushed the men_5819_escape_query_params_processor branch from 6eb3278 to 775bc4f Compare September 5, 2022 06:33

merlin-northern requested a review from tranchitella September 5, 2022 06:36

merlin-northern force-pushed the men_5819_escape_query_params_processor branch 4 times, most recently from bf6a9df to 1cfe463 Compare September 5, 2022 19:48

merlin-northern requested a review from alfrunes September 5, 2022 19:52

alfrunes reviewed Sep 6, 2022

View reviewed changes

merlin-northern requested a review from alfrunes September 6, 2022 06:55

alfrunes reviewed Sep 6, 2022

View reviewed changes

merlin-northern requested a review from alfrunes September 7, 2022 04:29

feat: Job String Processor

07f7953

Changelog: Title Ticket: MEN-5819 Signed-off-by: Peter Grzybowski <peter@northern.tech>

merlin-northern force-pushed the men_5819_escape_query_params_processor branch from 1cfe463 to 07f7953 Compare September 7, 2022 11:18

alfrunes reviewed Sep 7, 2022

View reviewed changes

merlin-northern closed this Sep 9, 2022

		l.Infof("processTask: calling http task: %s %s", httpTask.Method,
		processJobString(httpTask.URI, workflow, job))

	func (j JobStringProcessor) ProcessJobString(data string, options Options) string {
	func (j JobStringProcessor) ProcessJobString(data string, opts ...Options) string {

	regexVariableFlags = `(?:([a-zA-Z]+)+=([a-zA-Z0-9]+);)([^\}\\|]+)(?:\\|([^\}]+))?`
	regexVariableFlags = `(?P<options>(?:(?:[a-zA-Z]+)=(?:[a-zA-Z0-9]+);)+)(?P<name>[^;\}\\|]+)(?:\\|(?P<default>[^\}]+))?`

	regexVariable = `\$\{([^\}\\|]+)(?:\\|([^\}]+))?}`
	regexVariable = `\$\{(?P<options>(?:(?:[a-zA-Z]+)=(?:[a-zA-Z0-9]+);)*)(?P<name>[^;\}\\|]+)(?:\\|(?P<default>[^\}]+))?}`

	if !found {
	data = strings.ReplaceAll(data, submatch[0], defaultValue)
	}

	for _, submatch := range matches {
	SubmatchLoop:
	for _, submatch := range matches {

-		}
+		}
+		if option.Encoding == URL {
+			value = url.QueryEscape(value)
+		}
+		data = strings.ReplaceAll(data, submatch[0], value)

feat: Job String Processor #234

feat: Job String Processor #234

Conversation

merlin-northern commented Sep 4, 2022

mender-test-bot commented Sep 4, 2022

merlin-northern commented Sep 4, 2022

mender-test-bot commented Sep 4, 2022

merlin-northern commented Sep 4, 2022

mender-test-bot commented Sep 4, 2022

merlin-northern commented Sep 4, 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

merlin-northern Sep 5, 2022 • edited Loading

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

merlin-northern Sep 5, 2022 • edited Loading

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

merlin-northern commented Sep 5, 2022

mender-test-bot commented Sep 5, 2022

alfrunes commented Sep 6, 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

merlin-northern commented Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

merlin-northern Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlin-northern Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlin-northern commented Sep 7, 2022

mender-test-bot commented Sep 7, 2022

alfrunes left a comment

Choose a reason for hiding this comment

alfrunes Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfrunes Sep 7, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlin-northern commented Sep 7, 2022 • edited Loading

merlin-northern commented Sep 9, 2022

merlin-northern Sep 5, 2022 •

edited

Loading

merlin-northern Sep 5, 2022 •

edited

Loading

merlin-northern commented Sep 6, 2022 •

edited

Loading

merlin-northern Sep 6, 2022 •

edited

Loading

merlin-northern Sep 6, 2022 •

edited

Loading

alfrunes Sep 7, 2022 •

edited

Loading

alfrunes Sep 7, 2022 •

edited

Loading

merlin-northern commented Sep 7, 2022 •

edited

Loading