Tim.
On 09.04.21 18:55, Tim Düsterhus wrote:
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.
Thank you for your feedback.
> 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?
Well I have created a issue in coreJSON how to handle the "." in the key.
https://github.com/FreeRTOS/coreJSON/issues/92
I have choose the mjson library because it was small and offers the JSON path
feature.
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
maybe json_get_string because there could be some more getter like bool, int,
...
+ Returns the string value of the given json path.
It should be "JSON" (in uppercase) here and everywhere else.
Okay and agree.
+ 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.
Okay.
+ 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.
The '$.iss' is the generic JWT field.
https://tools.ietf.org/html/rfc7519#section-4.1
"iss" (Issuer) Claim
But maybe I could look for a "normal" JSON sting and only JWT.
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.
Okay.
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.
Yeah the difference between fetchers and converters in not fully clear for me.
I think when a value is fetched from any data then it's a fetcher like this
JSON "fetcher".
@@ -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.
Okay.
+ 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.
I was not sure about that. Let's wait for Willy's answer.
#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.
Okay.
+ */
+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.
This was intentionally. I asked my self why no Debug option is set.
This will only be printed with 'DEBUG=-DDEBUG_FULL'.
Maybe there should be a "DBUG_SAMPLES" like the other "DEBUG_*" options.
+
+ 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.
Full Ack.
+ 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".
Okay.
+ 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.
Okay.
+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.
Okay.
+ 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
