Add an elasticsearch scaler based on search template #2304

nicolasadeo · 2021-11-19T08:45:50Z

Signed-off-by: Nicolas L nicolas.lassalle@zenika.com

I am pleased to propose you the implementation of an Elasticsearch scaler based on a search template. Using a search template make things easy on the scaler configuration. The user must provide a search template name and eventually its params. The search result can be parsed using the gjson library like the metrics api scaler does.
For now, the authentication is based on username/password ; on the future we could imagine provide a CA certificate to validate a self signed certificate or ECE credentials.

I had a great time working on this scaler and I would be very pleased to get your code review's feedback.

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable)
Changelog has been updated

Fixes #1756

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

tomkerkhove · 2021-11-19T08:56:29Z

Thanks for the PR! Would you mind adding a new changelog entry please?

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

nicolasadeo · 2021-11-19T09:07:13Z

the changelog is in progress. Ive opened the PR with a wrong github account. Would you be OK if I reopen it with the proper github account?

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

JorTurFer

Hi,
In general looks good, only some small changes :)
Thanks a ton for your contribution ❤️

JorTurFer · 2021-11-21T22:29:27Z

pkg/scalers/elasticsearch_scaler.go

+	if val, ok := config.TriggerMetadata["unsafeSsl"]; ok {
+		meta.unsafeSsl, err = strconv.ParseBool(val)
+		if err != nil {
+			return nil, fmt.Errorf("error parsing unsafeSsL: %s", err)


Suggested change

return nil, fmt.Errorf("error parsing unsafeSsL: %s", err)

return nil, fmt.Errorf("error parsing unsafeSsl: %s", err)

Does the last L need to be in capital letter?

JorTurFer · 2021-11-21T22:36:44Z

pkg/scalers/elasticsearch_scaler.go

+	if meta.unsafeSsl {
+		tr := &http.Transport{
+			TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+		}
+		config.Transport = tr
+	}


I'm not an expert in elastic and maybe it's not possible but, could we avoid the if block using something like this?

Suggested change

if meta.unsafeSsl {

tr := &http.Transport{

TLSClientConfig: &tls.Config{InsecureSkipVerify: true},

}

config.Transport = tr

}

config.Transport = &http.Transport{

TLSClientConfig: &tls.Config{InsecureSkipVerify: meta.unsafeSsl},

}

We are using that approach inside utils package for the HTTPClient

make senses. I dig further to check

JorTurFer · 2021-11-21T22:42:01Z

pkg/scalers/elasticsearch_scaler.go

+	// Run the templated search
+	res, err := s.esClient.SearchTemplate(
+		&body,
+		s.esClient.SearchTemplate.WithIndex(s.metadata.indexes...),


You should pass the context until here because you could add .WithContext. With this change, the request context can be managed from outside the scaler.

JorTurFer · 2021-11-21T22:47:58Z

pkg/scalers/elasticsearch_scaler.go

+
+	externalMetric := &v2beta2.ExternalMetricSource{
+		Metric: v2beta2.MetricIdentifier{
+			Name: GenerateMetricNameWithIndex(s.metadata.targetValue, metricName),


This name should be generated using the ScalerIndex and the "metricName". The ScalerIndex is a parameter that all scalers receive in the constructor and which should be propagated to here in order to avoid conflicts. Also, it'd be tested during the unit test to ensure that it's correctly propagated.
This point is explained more in depth here.
All the other scalers propagate it, so you can check any other :)

JorTurFer · 2021-11-21T22:52:42Z

/run-e2e elasticsearch*

JorTurFer · 2021-11-21T23:29:25Z

tests/scalers/elasticsearch.test.ts

+    fs.writeFileSync(elasticsearchTmpFile.name, elasticsearchStatefulsetYaml.replace('{{ELASTIC_PASSWORD}}', elasticPassword))
+
+    t.is(0, sh.exec(`kubectl apply --namespace ${elasticsearchNamespace} -f ${elasticsearchTmpFile.name}`).code, 'creating an elasticsearch statefulset should work.')
+    t.is(0, waitForRollout('statefulset', "elasticsearch", elasticsearchNamespace))


I think e2e tests are failing because this could need a bit more time. WDYT?

Suggested change

t.is(0, waitForRollout('statefulset', "elasticsearch", elasticsearchNamespace))

t.is(0, waitForRollout('statefulset', "elasticsearch", elasticsearchNamespace, 300))

the elasticsearch image is huge (~1.2GB) and on my 6 cores laptop it takes ~20-30 seconds to start.
Let's check with 300 iterations

I will run again e2e tests when you increase the waiting timeout :)

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

orphaner · 2021-11-22T08:15:56Z

the changelog is in progress. Ive opened the PR with a wrong github account. Would you be OK if I reopen it with the proper github account?

could you let me know about this question? I really would like that the PR is attached to my proper GH account 🙏

orphaner · 2021-11-22T08:18:58Z

Hi, In general looks good, only some small changes :) Thanks a ton for your contribution heart

Small changes are pushed. My pleasure, I had a great time coding on keda!

JorTurFer · 2021-11-22T08:43:47Z

could you let me know about this question? I really would like that the PR is attached to my proper GH account 🙏

I think that it's not a problem, go on :)
The only thing that you should remember is updating the PR in docs to point to the new PR

JorTurFer

Hi,
another small nits:
Could you review the comment about the metric name and add atl least 1 test with ScalerIndex > 0?
Thanks! ❤️

JorTurFer · 2021-11-22T08:54:10Z

pkg/scalers/elasticsearch_scaler.go


 	externalMetric := &v2beta2.ExternalMetricSource{
 		Metric: v2beta2.MetricIdentifier{
-			Name: GenerateMetricNameWithIndex(s.metadata.targetValue, metricName),
+			Name: GenerateMetricNameWithIndex(s.metadata.targetValue, s.metadata.metricName),


You should call only once to GenerateMetricNameWithIndex. This index is not related with any inside the scaler, it's related with the trigger index inside the ScaledObject to avoid conflicts with duplicated names in the same ScaledObject.
You could call to the function here or in the constructor like you are doing but don't do it both, because the final metric name with current code will be like s{s.metadata.targetValue}-s{config.ScalerIndex}-elasticsearch-%s. It should be like s{config.ScalerIndex}-elasticsearch-%s

JorTurFer · 2021-11-22T08:57:23Z

/run-e2e elasticsearch*

nicolasadeo · 2021-11-22T08:59:12Z

Duplicate of #2311

JorTurFer · 2021-11-22T09:17:24Z

I have commented again on the new PR, you can ignore this :)

nicolasadeo · 2021-11-22T09:26:08Z

thanks a lot :)

adds an elasticsearch scaler based on search template

8a1b7b4

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

nicolasadeo requested review from ahmelsayed and zroubalik as code owners November 19, 2021 08:45

orphaner mentioned this pull request Nov 19, 2021

Add documentation for Elasticsearch scaler kedacore/keda-docs#586

Merged

1 task

nicolasadeo changed the title ~~Adds an elasticsearch scaler based on search template~~ Add an elasticsearch scaler based on search template Nov 19, 2021

tomkerkhove assigned zroubalik Nov 19, 2021

orphaner added 2 commits November 19, 2021 09:56

chore: proper indent

44ba555

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

chore: fixes lints

cc4c9f1

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

orphaner added 9 commits November 19, 2021 10:31

chore: add changelog entry

e8c6415

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

chore: rename params to parameters

9d703c2

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

chore: fix comment

7b408fd

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

:chore: trim

6afb6ca

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

chore: fix imports order

bef268d

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

chore: fix bad if / else / else if

4664f21

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

chore: fix sort in scale_handler

0d5042f

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

chore: fix name for sort-scalers hook

6f35589

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

chore: trim

4d835d3

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

JorTurFer requested changes Nov 21, 2021

View reviewed changes

JorTurFer reviewed Nov 21, 2021

View reviewed changes

chore: fixes after code review

3108152

Signed-off-by: Nicolas L <nicolas.lassalle@zenika.com>

JorTurFer requested changes Nov 22, 2021

View reviewed changes

nicolasadeo closed this Nov 22, 2021

zroubalik modified the milestone: v2.5.0 Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an elasticsearch scaler based on search template #2304

Add an elasticsearch scaler based on search template #2304

nicolasadeo commented Nov 19, 2021 •

edited

Loading

tomkerkhove commented Nov 19, 2021

nicolasadeo commented Nov 19, 2021

JorTurFer left a comment

JorTurFer Nov 21, 2021

JorTurFer Nov 21, 2021

orphaner Nov 22, 2021

JorTurFer Nov 21, 2021 •

edited

Loading

JorTurFer Nov 21, 2021

JorTurFer commented Nov 21, 2021

JorTurFer Nov 21, 2021

orphaner Nov 22, 2021 •

edited

Loading

JorTurFer Nov 22, 2021

orphaner commented Nov 22, 2021

orphaner commented Nov 22, 2021

JorTurFer commented Nov 22, 2021

JorTurFer left a comment

JorTurFer Nov 22, 2021

JorTurFer commented Nov 22, 2021

nicolasadeo commented Nov 22, 2021

JorTurFer commented Nov 22, 2021

nicolasadeo commented Nov 22, 2021

	return nil, fmt.Errorf("error parsing unsafeSsL: %s", err)
	return nil, fmt.Errorf("error parsing unsafeSsl: %s", err)

	t.is(0, waitForRollout('statefulset', "elasticsearch", elasticsearchNamespace))
	t.is(0, waitForRollout('statefulset', "elasticsearch", elasticsearchNamespace, 300))

Add an elasticsearch scaler based on search template #2304

Add an elasticsearch scaler based on search template #2304

Conversation

nicolasadeo commented Nov 19, 2021 • edited Loading

Checklist

tomkerkhove commented Nov 19, 2021

nicolasadeo commented Nov 19, 2021

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer Nov 21, 2021

Choose a reason for hiding this comment

JorTurFer Nov 21, 2021

Choose a reason for hiding this comment

orphaner Nov 22, 2021

Choose a reason for hiding this comment

JorTurFer Nov 21, 2021 • edited Loading

Choose a reason for hiding this comment

JorTurFer Nov 21, 2021

Choose a reason for hiding this comment

JorTurFer commented Nov 21, 2021

JorTurFer Nov 21, 2021

Choose a reason for hiding this comment

orphaner Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

JorTurFer Nov 22, 2021

Choose a reason for hiding this comment

orphaner commented Nov 22, 2021

orphaner commented Nov 22, 2021

JorTurFer commented Nov 22, 2021

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer Nov 22, 2021

Choose a reason for hiding this comment

JorTurFer commented Nov 22, 2021

nicolasadeo commented Nov 22, 2021

JorTurFer commented Nov 22, 2021

nicolasadeo commented Nov 22, 2021

nicolasadeo commented Nov 19, 2021 •

edited

Loading

JorTurFer Nov 21, 2021 •

edited

Loading

orphaner Nov 22, 2021 •

edited

Loading