krun: support OCI annotations to configure microVM #1950
Conversation
Summary of ChangesHello @jakecorrenti, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the configurability of krun microVMs by integrating support for OCI annotations. This change allows users to define microVM resources and characteristics directly through container annotations, offering a more dynamic and reproducible configuration method that aligns well with container orchestration practices, while maintaining backward compatibility with existing JSON configuration files. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring the microVM using OCI annotations, which is a great feature for configurability. The implementation correctly prioritizes annotations over the krun_vm.json file.
However, I've found a few issues in the implementation:
- A critical bug that can lead to a crash due to a
NULLpointer being passed tostrcmp. - Some high-severity issues in the parsing logic for integer values from both annotations and the JSON configuration file, which can lead to incorrect behavior or using garbage values. These include unsafe use of
strtoland incorrect handling of floating-point numbers from JSON. - Minor issues include printing warnings to
stdoutinstead ofstderr.
I've left detailed comments with suggestions to fix these issues. Addressing them will make the new feature more robust and reliable.
|
TMT tests failed. @containers/packit-build please check. |
Support the use of OCI annotations as a way for the container engine to configure the microVM. The user may add the `krun.variant` annotation to specify the flavor of libkrun they want to use. The current supported values are ["sev", "aws-nitro"]. Signed-off-by: Jake Correnti <[email protected]>
Support the use of OCI annotations as a way for the container engine to configure the microVM's CPU count and RAM amount via `krun.cpus` and `krun.ram_mib` respectively. Signed-off-by: Jake Correnti <[email protected]>
jakecorrenti
force-pushed
the
annotations
branch
from
bff10de to
4c3dc10
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring the krun microVM using OCI annotations, falling back to the existing krun_vm.json file. This is a great feature for improving configurability.
The implementation looks good overall, but I've found a few issues:
- A critical issue in
libkrun_configure_flavorthat could lead to a segmentation fault due to a missingNULLcheck. - A high-severity issue in the new
libkrun_parse_resource_configurationfunction regarding incorrect error handling for missing JSON keys and lack of validation for negative resource values. - A medium-severity issue related to type correctness (
char *vsconst char *) in a couple of places.
I've provided detailed comments and suggestions to address these points. Once these are fixed, the PR should be in good shape.
| if (val_flavor != NULL && YAJL_IS_STRING (val_flavor)) | ||
| // Check if the user provided the krun variant through OCI annotations. | ||
| flavor = (char *) find_annotation (container, "krun.variant"); | ||
| if (flavor == NULL) |
There was a problem hiding this comment.
There is a potential null pointer dereference on the next line. *config_tree can be NULL if libkrun_read_vm_config doesn't find or fails to parse the VM config file. yajl_tree_get will then be called with a NULL node, leading to a crash. You should add a check that *config_tree is not NULL before dereferencing it.
if (flavor == NULL && *config_tree)| libkrun_parse_resource_configuration (yajl_val *config_tree, libcrun_container_t *container, char *annotation, const char *path[], int default_val) | ||
| { | ||
| char *annotation_val; | ||
| char *endptr; | ||
| int resource_val = default_val; | ||
| yajl_val krun_file_val = NULL; | ||
|
|
||
| annotation_val = (char *) find_annotation (container, annotation); | ||
| if (annotation_val != NULL) | ||
| { | ||
| errno = 0; | ||
| resource_val = strtol (annotation_val, &endptr, 10); | ||
| if (errno != 0 || endptr == annotation_val || *endptr != '\0') | ||
| { | ||
| fprintf (stderr, "failed to convert %s value to an integer: falling back to the default value %d\n", annotation, default_val); | ||
| return default_val; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| krun_file_val = yajl_tree_get (*config_tree, path, yajl_t_number); | ||
| if (! YAJL_IS_INTEGER (krun_file_val)) | ||
| { | ||
| fprintf (stderr, "failed to convert %s value in krun_vm.json to an integer: falling back to the default value %d\n", path[0], default_val); | ||
| return default_val; | ||
| } | ||
|
|
||
| if (krun_file_val != NULL) | ||
| resource_val = YAJL_GET_INTEGER (krun_file_val); | ||
| } | ||
|
|
||
| return resource_val; | ||
| } |
There was a problem hiding this comment.
The function libkrun_parse_resource_configuration has a few areas for improvement for correctness and robustness:
- The
annotationparameter should beconst char *since it is not modified. The cast(char *)on the return value offind_annotationwould then be unnecessary. - The logic for parsing the JSON configuration prints an error message if a key is not found. It should only show an error if the key exists but has an incorrect type.
- The function does not validate against negative values for resources like CPUs and RAM, which could lead to undefined behavior when these values are cast to unsigned types later.
Here is a suggested refactoring of the function to address these points:
static int
libkrun_parse_resource_configuration (yajl_val *config_tree, libcrun_container_t *container, const char *annotation, const char *path[], int default_val)
{
const char *annotation_val;
char *endptr;
int resource_val = default_val;
yajl_val krun_file_val;
annotation_val = find_annotation (container, annotation);
if (annotation_val != NULL)
{
errno = 0;
long val = strtol (annotation_val, &endptr, 10);
if (errno != 0 || endptr == annotation_val || *endptr != '\0' || val < 0)
{
fprintf (stderr, "failed to convert annotation '%s' value to a non-negative integer: falling back to the default value %d\n", annotation, default_val);
return default_val;
}
resource_val = (int)val;
}
else if (*config_tree)
{
krun_file_val = yajl_tree_get (*config_tree, path, yajl_t_any);
if (krun_file_val)
{
if (YAJL_IS_INTEGER (krun_file_val))
{
long long val = YAJL_GET_INTEGER (krun_file_val);
if (val < 0)
{
fprintf (stderr, "value for '%s' in krun_vm.json is negative: falling back to the default value %d\n", path[0], default_val);
return default_val;
}
resource_val = (int)val;
}
else
{
fprintf (stderr, "value for '%s' in krun_vm.json is not an integer: falling back to the default value %d\n", path[0], default_val);
return default_val;
}
}
}
return resource_val;
}| val_flavor = yajl_tree_get (*config_tree, path_flavor, yajl_t_string); | ||
| if (val_flavor != NULL && YAJL_IS_STRING (val_flavor)) | ||
| // Check if the user provided the krun variant through OCI annotations. | ||
| flavor = (char *) find_annotation (container, "krun.variant"); |
There was a problem hiding this comment.
The cast to (char *) is incorrect as find_annotation returns a const char *. The flavor variable (declared on line 266) should be changed to const char * to match the types of the values it is assigned from both find_annotation and YAJL_GET_STRING.
flavor = find_annotation (container, "krun.variant");1 participant
Add support for the use of OCI annotations to configure the microVM. In Podman, we can take advantage of modules to make creating krun containers highly configurable and reproducible.