Aleks,
I have taken a first look. Find my remarks below. Please note that for
the actual source code there might be further remarks by Willy (put in
CC) or so. I might have missed something or might have told you
something incorrect. So maybe before making changes wait for their opinion.
Generally I must say that I don't like the mjson library, because it
uses 'int' for sizes. It doesn't really bring the point home that it is
a safe library. This one looks much better to me:
https://github.com/FreeRTOS/coreJSON. It does not support JSON path,
though. Not sure how much of an issue that would be?
On 4/8/21 10:21 PM, Aleksandar Lazic wrote:
From 7ecb80b1dfe37c013cf79bc5b5b1caa3c0112a6a Mon Sep 17 00:00:00 2001
From: Alekesandar Lazic <[email protected]>
Date: Thu, 8 Apr 2021 21:42:00 +0200
Subject: [PATCH] MINOR: sample: add json_string
I'd add 'converter' to the subject line to make it clear that this is a
conveter.
This sample get's the value of a JSON key
Typo: It should be 'gets'.
---
Makefile | 3 +-
doc/configuration.txt | 15 +
include/import/mjson.h | 213 +++++
reg-tests/sample_fetches/json_string.vtc | 25 +
src/mjson.c | 1052 ++++++++++++++++++++++
src/sample.c | 63 ++
6 files changed, 1370 insertions(+), 1 deletion(-)
create mode 100644 include/import/mjson.h
create mode 100644 reg-tests/sample_fetches/json_string.vtc
create mode 100644 src/mjson.c
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 01a01eccc..7f2732668 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -19043,6 +19043,21 @@ http_first_req : boolean
This is the 'fetch' section. Move the documentation to the 'converter'
section.
from some requests when a request is not the first one, or to help grouping
requests in the logs.
+json_string(<json_path>) : string
I don't like the name. A few suggestions:
- json_query
- json_get
- json_decode
+ Returns the string value of the given json path.
It should be "JSON" (in uppercase) here and everywhere else.
+ The <json_path> is required.
+ This sample uses the mjson library https://github.com/cesanta/mjson
+ The json path syntax is defined in this repo
https://github.com/json-path/JsonPath
Overall the description of the converter does not read nicely / feels
inconsistent compared to other converters / uses colloquial language.
Let me suggest something like:
Extracts the value located at <json_path> from the JSON string in the
input value. <json_path> must be a valid JsonPath string as defined at
https://goessner.net/articles/JsonPath/
I changed the link, because that appears to be the canonical reference.
+ Example :
No space in front of the colon.
+ # get the value from the key kubernetes.io/serviceaccount/namespace
+ # => openshift-logging
+ http-request set-var(sess.json)
req.hdr(Authorization),b64dec,json_string('$.kubernetes\\.io/serviceaccount/namespace')
+
+ # get the value from the key iss
+ # => kubernetes/serviceaccount
+ http-request set-var(sess.json)
req.hdr(Authorization),b64dec,json_string('$.iss')
I don't like that the example is that specific for Kubernetes usage. A
more general example would be preferred, because it makes it easier to
understand the concept.
method : integer + string
Returns an integer value corresponding to the method in the HTTP request. For
example, "GET" equals 1 (check sources to establish the matching). Value 9
diff --git a/include/import/mjson.h b/include/import/mjson.h
new file mode 100644
index 000000000..ff46e7950
--- /dev/null
+++ b/include/import/mjson.h
@@ -0,0 +1,213 @@
[...]
+// Aleksandar Lazic
+// git clone from 2021-08-04 because of this fix
+//
https://github.com/cesanta/mjson/commit/7d8daa8586d2bfd599775f049f26d2645c25a8ee
Please don't edit third party libraries, even if it is just a comment.
This makes updating hard.
diff --git a/reg-tests/sample_fetches/json_string.vtc
b/reg-tests/sample_fetches/json_string.vtc
new file mode 100644
index 000000000..fc387519b
--- /dev/null
+++ b/reg-tests/sample_fetches/json_string.vtc
Again, this is a converter. Move the test into the appropriate folder.
And please make sure you understand the difference between fetches and
converters.
@@ -0,0 +1,25 @@
+varnishtest "Test to get value from json sting"
+#REQUIRE_VERSION=2.4
+
+feature ignore_unknown_macro
+
+haproxy h1 -conf {
+ defaults
+ mode http
+ timeout connect 1s
+ timeout client 1s
+ timeout server 1s
+
+ frontend fe1
+ bind "fd@${fe1}"
+ http-request set-var(sess.iss)
req.hdr(Authorization),b64dec,json_string('$.iss')
+ http-request return status 200 hdr x-var "json-value=%[var(sess.iss)]"
+} -start
+
+client c1 -connect ${h1_fe1_sock} {
+ txreq -req GET -url "/" \
+ -hdr "Authorization:
eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJvcGVuc2hpZnQtbG9nZ2luZyIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VjcmV0Lm5hbWUiOiJkZXBsb3llci10b2tlbi1tOTh4aCIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50Lm5hbWUiOiJkZXBsb3llciIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50LnVpZCI6IjM1ZGRkZWZkLTNiNWEtMTFlOS05NDdjLWZhMTYzZTQ4MDkxMCIsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDpvcGVuc2hpZnQtbG9nZ2luZzpkZXBsb3llciJ9"
The use of base64 makes it hard to check the test for correctness. I
suggest to a simpler input string that can be embedded into the test as-is.
Please also add additional tests for other cases, e.g.:
- If the key cannot be found in the JSON.
- If the value is not a string, but a number or a boolean.
Have a look at the other tests to see how they are structured.
+ rxresp
+ expect resp.status == 200
+ expect resp.http.x-var ~ "json-value=kubernetes/serviceaccount"
+} -run
diff --git a/src/mjson.c b/src/mjson.c
new file mode 100644
index 000000000..729d671c6
--- /dev/null
+++ b/src/mjson.c
@@ -0,0 +1,1052 @@
[...]
+// Aleksandar Lazic
+// git clone from 2021-08-04 because of this fix
+//
https://github.com/cesanta/mjson/commit/7d8daa8586d2bfd599775f049f26d2645c25a8ee
Same as above.
diff --git a/src/sample.c b/src/sample.c
index 835a18115..0096de3ae 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -18,6 +18,7 @@
#include <import/sha1.h>
#include <import/xxhash.h>
+#include <import/mjson.h>
I believe these should be sorted alphabetically.
#include <haproxy/api.h>
#include <haproxy/arg.h>
@@ -3653,6 +3654,65 @@ static int sample_conv_rtrim(const struct arg *arg_p,
struct sample *smp, void *
return 1;
}
+/* This function checks the "json_string" converter's arguments.
+ * This function returns 1 when everything is good else 0
This is very colloquial language. It is also not true: This function
returns 1, even when the JSON path is not a valid JSON path.
+ */
+static int sample_check_json_string(struct arg *arg, struct sample_conv *conv,
+ const char *file, int line, char **err)
+{
+ DPRINTF(stderr, "%s: arg->type=%d, arg->data.str.data=%ld\n",
+ __FUNCTION__,
+ arg->type, arg->data.str.data);
Debug code above.
+
+ if (arg->type != ARGT_STR) {
+ memprintf(err, "unexpected argument type");
+ return 0;
+ }
I believe it should not be necessary to check this, but don't quote me
on that and wait for an authoritative answer.
+ if (arg->data.str.data == 0) { /* empty */
+ memprintf(err, "empty argument");
This error message does not read nicely, because it is overly technical.
Use something like "json_path must not be empty".
+ return 0;
+ }
+
+ DPRINTF(stderr, "%s: check passed\n",
+ __FUNCTION__);
Debug code.
+ return 1;
+}
+
+/* This sample function get the value from a given json string.
+ * The mjson library is used to parse the json struct
+*/
Missing space here.
+static int sample_conv_json_string(const struct arg *args, struct sample *smp,
void *private)
+{
+ struct buffer *trash = get_trash_chunk();
+ char *value = (char*) trash->area;
The cast is useless. The variable is useless as well, use trash->area
directly. That's more readable.
+ int value_len = trash->size;
You are assigning a size_t to an int. Don't do this. I suggest to use
trash->size directly.
+ int rc;
This is useless, see below.
+ DPRINTF(stderr, "%s: smp->data.u.str.area=%s, smp->data.u.str.data=%ld
args[0].data.str.area=%s\n",
+ __FUNCTION__,
+ smp->data.u.str.area,
smp->data.u.str.data, args[0].data.str.area);
Debug code.
+ rc = mjson_get_string(smp->data.u.str.area, smp->data.u.str.data,
args[0].data.str.area, value, value_len);
Directly assign the return value to `trash->data` to get rid of the
intermediate variable. This prevents bugs.
+ DPRINTF(stderr, "%s: rc=%d, value=%s value_len=%d\n",
+ __FUNCTION__,
+ rc, value, value_len);
Debug code.
+ if (rc == -1 ) {
Extra space that should not be there.
+ return 0;
+ }
+
+ trash->data = rc;
+ smp->data.u.str = *trash;
+ smp->data.type = SMP_T_STR;
+ smp->flags &= ~SMP_F_CONST;
+
+ return 1;
+}
+
+
/************************************************************************/
/* All supported sample fetch functions must be declared here */
/************************************************************************/
@@ -4165,6 +4225,9 @@ static struct sample_conv_kw_list sample_conv_kws = {ILH,
{
{ "cut_crlf", sample_conv_cut_crlf, 0, NULL, SMP_T_STR,
SMP_T_STR },
{ "ltrim", sample_conv_ltrim, ARG1(1,STR), NULL, SMP_T_STR,
SMP_T_STR },
{ "rtrim", sample_conv_rtrim, ARG1(1,STR), NULL, SMP_T_STR,
SMP_T_STR },
+
+ { "json_string", sample_conv_json_string, ARG1(1,STR),
sample_check_json_string , SMP_T_STR, SMP_USE_CONST },
+
{ NULL, NULL, 0, 0, 0 },
}};
--
2.25.1
Best regards
Tim Düsterhus
