kubernetes-client / c

Official C client library for Kubernetes
Apache License 2.0
141 stars 45 forks source link

Unexpected processing behavior of null value in yaml/json #138

Closed caesar0301 closed 1 year ago

caesar0301 commented 1 year ago

Env

Kernel 4.19 GCC 4.9.2 Cmake 3.23.2 kubernetes-client-c 0.4.0

Problem

Original yaml which could be well processed by kubectl:

apiVersion: v1
kind: ServiceAccount
metadata:
    name: test-sa
    namespace: smoking-test
    annotations: null

Converted in legal JSON semantics:

{
    "apiVersion": "v1",
    "kind": "ServiceAccount",
    "metadata": {
        "name": "test-sa",
        "namespace": "smoking-test",
        "annotations": null
    }
}

Code snippet in C++

#include <cstdio>
#include <string>

#ifdef __cplusplus
extern "C" {
#endif
#include <kubernetes/api/CoreV1API.h>
#include <kubernetes/external/cJSON.h>
#ifdef __cplusplus
}
#endif

using namespace std;

int main() {
  string rawJson =
      "{\"apiVersion\":\"v1\",\"kind\":\"ServiceAccount\",\"metadata\":{\"name\":\"test-sa\","
      "\"namespace\":\"smoking-test\",\"annotations\":null}}";

  cJSON *json = cJSON_Parse(rawJson.c_str());
  cJSON *meta = cJSON_GetObjectItem(json, "metadata");
  cJSON *anno = cJSON_GetObjectItem(meta, "annotations");

  printf("anno == NULL: %d\n", anno == NULL);
  printf("anno is object: %d\n", cJSON_IsObject(anno));
  printf("anno type & 0xFF: %d\n", anno->type & 0xFF);

  v1_service_account_t *obj = v1_service_account_parseFromJSON(json);

  printf("metadata == NULL: %d\n", obj->metadata == NULL);

  v1_service_account_free(obj);
  cJSON_free(json);
  return 0;
}

Output

anno == NULL: 0
anno is object: 0
anno type & 0xFF: 4
metadata == NULL: 1

Expected

In above example, metadata is NOT null by ignoring null subfields (say annotationa), or converting the null subfields into empty data object, such as empty cJSON_Object.

Generally, this c-sdk skips all following fields processing when encountering null (not always violating k8s yaml specification), as we can see "goto end" in v1_object_meta_parseFromJSON specific to above example.

ityuhui commented 1 year ago

Thank you for reporting this. We will fix it soon.

If you want a quick fix with your own build, change https://github.com/kubernetes-client/c/blob/9a78e0fe02dac4aa65b86d98a7fa894be2fee872/kubernetes/model/v1_object_meta.c#L354

to

    if(!cJSON_IsObject(annotations) && !cJSON_IsNull(annotations)) {
caesar0301 commented 1 year ago

Thank you for reporting this. We will fix it soon.

If you want a quick fix with your own build, change

https://github.com/kubernetes-client/c/blob/9a78e0fe02dac4aa65b86d98a7fa894be2fee872/kubernetes/model/v1_object_meta.c#L354

to

    if(!cJSON_IsObject(annotations) && !cJSON_IsNull(annotations)) {

This method seems not REALLY quick, because potential any fields not violating k8s specification would be null. I need to modify all fields parsing logic.

A straight-forward and fast fix could be modifying cJSON as following. Yet I am not sure if this modification is in compliance with cJSON's design principle.

diff --git a/kubernetes/external/cJSON.c b/kubernetes/external/cJSON.c
index 2139b5e..2ea338c 100644
--- a/kubernetes/external/cJSON.c
+++ b/kubernetes/external/cJSON.c
@@ -1794,6 +1794,11 @@ static cJSON *get_object_item(const cJSON * const object, const char * const nam
         }
     }

+    if (cJSON_IsNull(current_element))
+    {
+        return NULL;
+    }
+
     return current_element;
 }
ityuhui commented 1 year ago

Thank you! The best solution is upgrading cJSON to the latest release as your suggestion. (But I haven't tried this solution now)

The change

diff --git a/kubernetes/external/cJSON.c b/kubernetes/external/cJSON.c
index 2139b5e..2ea338c 100644
--- a/kubernetes/external/cJSON.c
+++ b/kubernetes/external/cJSON.c
@@ -1794,6 +1794,11 @@ static cJSON *get_object_item(const cJSON * const object, const char * const nam
         }
     }

+    if (cJSON_IsNull(current_element))
+    {
+        return NULL;
+    }
+
     return current_element;
 }

is not a good idea because it will break compatibility with upstream cJSON. We don't plan to maintain a fork of our own.

I plan to fix this issue via upgrading cJSON finally.

caesar0301 commented 1 year ago

@ityuhui as I tested, mere upgrading cJSON does NOT fix this problem.

Latest cJSON just enhanced current_element validity check in get_object_item, not fixed our problem.

ityuhui commented 1 year ago

OK. I got it.

Then we can fix it via the first solution https://github.com/kubernetes-client/c/issues/138#issuecomment-1226844487

The fix will commit into the template file https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/C-libcurl/model-body.mustache, so it will update all where needed automatically.