add support to load kubernetes configuration for memory buffer #242

DanyT · 2024-07-01T15:58:54Z

existing api is not changed
- load_kube_config works as before
extend kubeconfig_t to hold in memory buffer
make a common flow for file/buffer code
set fileName/buffer members accordingly
provide new api: load_kube_config_buffer

- existing api is not changed - load_kube_config works as before - extend kubeconfig_t to hold in memory buffer - make a common flow for file/buffer code - set fileName/buffer members accordingly - provide new api: load_kube_config_buffer

k8s-ci-robot · 2024-07-01T15:59:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: DanyT
Once this PR has been reviewed and has the lgtm label, please assign ityuhui for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

close filehandle only if we opened one

ityuhui · 2024-07-02T13:25:14Z

Thanks for your PR. I will review it.

brendandburns · 2024-07-02T17:57:04Z

kubernetes/config/kube_config.c

+    static char fname[] = "load_kube_config()";
+    int rc = 0;
+
+    kubeconfig_t *kubeconfig = kubeconfig_create();


Given that you delete this below, I think you can stack allocate this and pass the pointer via & below.

That will save a heap allocation and also be cleaner code.

This can be done but implies additional changes in kubeconfig_free() as this function will attempt to free the kubeconfig_t* itself. I can do a kubeconfig_free_static() if needed in order to keep the original api interface unchanged.

Please let me know your thoughts on this one.

Meanwhile I added support to use both heap-based and stack-based allocated kubeconfig_t and a bit of documentation for these internal functions.

brendandburns · 2024-07-02T17:57:51Z

kubernetes/config/kube_config.c

+    static char fname[] = "load_kube_config_buffer()";
+    int rc = 0;
+
+    kubeconfig_t *kubeconfig = kubeconfig_create();


Stack allocate here to, as above.

See #242 (comment)

brendandburns · 2024-07-02T17:59:47Z

kubernetes/config/kube_config_yaml.h

@@ -24,13 +24,15 @@ extern "C" {
 *
 * IN:
 * kubeconfig->fileName: kubernetes cluster configuration file name


Maybe note that either one, but not both should be supplied?

I have added a more explicit note there. There was already a hint in the doc for kubeconfig->buffer.

IN:

kubeconfig->fileName: kubernetes cluster configuration file name

kubeconfig->buffer: kubernetes cluster configuration data; this is considered only if kubeconfig-> fileName is set to NULL

Note: One may use either kubeconfig->fileName or kubeconfig->buffer but not both at the same time.

brendandburns · 2024-07-02T18:00:20Z

kubernetes/config/kube_config_yaml.c

@@ -430,25 +430,34 @@ int kubeyaml_load_kubeconfig(kubeconfig_t * kubeconfig)
 {
    static char fname[] = "kubeyaml_load_kubeconfig()";

-    /* Set a file input. */
+    /* Set a file input or use the provided buffer. */
    FILE *input = NULL;


Add a test here to verify that only one of fileName and buffer is non-null.

brendandburns · 2024-07-02T18:00:44Z

Minor comments.

…uffer is set Add documentation note stating that only of the kubeconfig_t::fileName and kubeconfig_t::buffer may be set

brendandburns · 2024-07-03T16:12:03Z

This looks good to me. Thanks for the quick turn around. I'll wait for @ityuhui to take a look though in case there are additional comments.

ityuhui · 2024-07-05T00:45:41Z

Thanks @brendandburns

@DanyT Can you please fix the issues reported by the code-static-check and code-sytle-check https://github.com/kubernetes-client/c/actions/runs/9781006686/job/27005098287?pr=242 in your code changes ? (Just update your code changes)

You can run the checks locally with

sh ./code-check/code-static-check.sh ./kubernetes/config/

and

find ./kubernetes/config/ -type f -regextype posix-extended -regex ".*\.(c|h)" -exec sh ./code-check/code-style-check.sh {} \;

ityuhui · 2024-07-05T00:49:46Z

@DanyT
Could you please write an example that uses your new function so we can add it to the automated tests in case your functions are broken by future code changes. We also have memory leak testing in automated testing.

FYI

c/examples/Makefile

Line 33 in dc7d670

test:

kubernetes/config/kube_config_yaml.c

DanyT · 2024-07-05T06:29:34Z

@ityuhui I have added:

example for load_kube_config_buffer: ./example/list_pod_buffer
fixed static and style code checks.

Let me know if there is something that i missed.

ityuhui · 2024-07-05T13:19:32Z

@DanyT

It seems that the build failed

cd list_pod_buffer; make test
make[1]: Entering directory '/home/runner/work/c/c/examples/list_pod_buffer'
./list_pod_buffer_bin
Cannot get kubernetes configuration from file.
make[1]: *** [Makefile:11: test] Error 255
make[1]: Leaving directory '/home/runner/work/c/c/examples/list_pod_buffer'
make: *** [Makefile:40: test] Error 2
Error: Process completed with exit code 2.

examples/exec_provider/list_pod_by_exec_provider.c~

examples/exec_provider/my_exec_provider.c~

DanyT · 2024-07-05T14:07:26Z

Interesting... it is not the build itself that fails but at runtime it cannot find/load the kube config file. I will look into it. Is there a special location where the kube config file is delivered in this test environment? @ityuhui

ityuhui · 2024-07-06T13:33:56Z

Nothing special, the configuration is located in $HOME/.kube/config

FYI

c/kubernetes/config/kube_config.c

Line 130 in dc7d670

homedir_env = getenv(ENV_HOME);

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 1, 2024

k8s-ci-robot requested review from brendandburns and ityuhui July 1, 2024 15:59

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 1, 2024

fix typo

8f759fc

close filehandle only if we opened one

brendandburns reviewed Jul 2, 2024

View reviewed changes

DanyT added 2 commits July 3, 2024 09:18

Check that only one of the kubeconfig_t::fileName and kubeconfig_t::b…

34d1b13

…uffer is set Add documentation note stating that only of the kubeconfig_t::fileName and kubeconfig_t::buffer may be set

add support to free a stack based allocated kubeconfig_t

4a19bc2

ityuhui reviewed Jul 5, 2024

View reviewed changes

kubernetes/config/kube_config_yaml.c Outdated Show resolved Hide resolved

DanyT added 2 commits July 5, 2024 07:55

resolve code-sytle-check issues

70f05e5

add example for load_kube_config_buffer

1d79e0f

ityuhui reviewed Jul 5, 2024

View reviewed changes

examples/exec_provider/list_pod_by_exec_provider.c~ Outdated Show resolved Hide resolved

ityuhui reviewed Jul 5, 2024

View reviewed changes

examples/exec_provider/my_exec_provider.c~ Outdated Show resolved Hide resolved

remove files that are not needed

2525271

log attempts to load kube data

fb4170b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support to load kubernetes configuration for memory buffer #242

add support to load kubernetes configuration for memory buffer #242

DanyT commented Jul 1, 2024

k8s-ci-robot commented Jul 1, 2024

ityuhui commented Jul 2, 2024

brendandburns Jul 2, 2024

DanyT Jul 3, 2024 •

edited

Loading

DanyT Jul 3, 2024

brendandburns Jul 2, 2024

DanyT Jul 3, 2024 •

edited

Loading

brendandburns Jul 2, 2024

DanyT Jul 3, 2024 •

edited

Loading

brendandburns Jul 2, 2024

DanyT Jul 3, 2024

brendandburns commented Jul 2, 2024

brendandburns commented Jul 3, 2024

ityuhui commented Jul 5, 2024 •

edited

Loading

ityuhui commented Jul 5, 2024 •

edited

Loading

DanyT commented Jul 5, 2024

ityuhui commented Jul 5, 2024

DanyT commented Jul 5, 2024

ityuhui commented Jul 6, 2024

add support to load kubernetes configuration for memory buffer #242

Are you sure you want to change the base?

add support to load kubernetes configuration for memory buffer #242

Conversation

DanyT commented Jul 1, 2024

k8s-ci-robot commented Jul 1, 2024

ityuhui commented Jul 2, 2024

brendandburns Jul 2, 2024

Choose a reason for hiding this comment

DanyT Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

DanyT Jul 3, 2024

Choose a reason for hiding this comment

brendandburns Jul 2, 2024

Choose a reason for hiding this comment

DanyT Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

brendandburns Jul 2, 2024

Choose a reason for hiding this comment

DanyT Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

brendandburns Jul 2, 2024

Choose a reason for hiding this comment

DanyT Jul 3, 2024

Choose a reason for hiding this comment

brendandburns commented Jul 2, 2024

brendandburns commented Jul 3, 2024

ityuhui commented Jul 5, 2024 • edited Loading

ityuhui commented Jul 5, 2024 • edited Loading

DanyT commented Jul 5, 2024

ityuhui commented Jul 5, 2024

DanyT commented Jul 5, 2024

ityuhui commented Jul 6, 2024

DanyT Jul 3, 2024 •

edited

Loading

DanyT Jul 3, 2024 •

edited

Loading

DanyT Jul 3, 2024 •

edited

Loading

ityuhui commented Jul 5, 2024 •

edited

Loading

ityuhui commented Jul 5, 2024 •

edited

Loading