-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support to load kubernetes configuration for memory buffer #242
add support to load kubernetes configuration for memory buffer #242
Conversation
DanyT
commented
Jul 1, 2024
- 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
Thanks for your PR. I will review it. |
kubernetes/config/kube_config.c
Outdated
static char fname[] = "load_kube_config()"; | ||
int rc = 0; | ||
|
||
kubeconfig_t *kubeconfig = kubeconfig_create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meanwhile I added support to use both heap-based and stack-based allocated kubeconfig_t and a bit of documentation for these internal functions.
kubernetes/config/kube_config.c
Outdated
static char fname[] = "load_kube_config_buffer()"; | ||
int rc = 0; | ||
|
||
kubeconfig_t *kubeconfig = kubeconfig_create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stack allocate here to, as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #242 (comment)
@@ -24,13 +24,15 @@ extern "C" { | |||
* | |||
* IN: | |||
* kubeconfig->fileName: kubernetes cluster configuration file name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe note that either one, but not both should be supplied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test here to verify that only one of fileName
and buffer
is non-null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Minor comments. |
…uffer is set Add documentation note stating that only of the kubeconfig_t::fileName and kubeconfig_t::buffer may be set
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. |
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 I have added:
Let me know if there is something that i missed. |
It seems that the build failed
|
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 |
Nothing special, the configuration is located in FYI c/kubernetes/config/kube_config.c Line 130 in dc7d670
|
@ityuhui Sorry for the delay. This should fix the issue. |
Done |
I have no question about this PR. Thanks to @DanyT for the contribution ! @brendandburns I'm OK with the changes. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, DanyT, ityuhui The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |