Skip to content
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

feat: validate apisix.yaml for config_provider yaml #10630

Closed

Conversation

boekkooi-lengoo
Copy link
Contributor

Description

This prototype tries to show a way to validate the apisix.yaml by starting openresty and only running the validation code.

As part of the validation we initialize the configuration and then check if no errors where logs. If any errors are logged then the apisix test command fails.

Special thanks to https://github.com/openresty/resty-cli/!

Fixes #10583

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

This prototype tries to show a way to validate the `apisix.yaml` by starting openresty and only running the validation code.

As part of the validation we initialize the configuration and then check if no errors where logs. If any errors are logged then the `apisix test` command fails.

Special thanks to https://github.com/openresty/resty-cli/!
@boekkooi-lengoo boekkooi-lengoo changed the title feat: validate apisix.yaml for config_provider yaml feat: validate apisix.yaml for config_provider yaml Dec 11, 2023
@monkeyDluffy6017
Copy link
Contributor

monkeyDluffy6017 commented Dec 12, 2023

Nice work! We really appreciate this! we will check it later


local valid = true
for module_name, cfg_func in pairs(modules) do
apisix_core.log.no_error_logs = true
Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i assume this was added by mistake (duplicate exists in L#919)

Copy link
Contributor Author

@boekkooi-lengoo boekkooi-lengoo Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually done on purpose.
As apisix_core.log.error can set the variable to false.
By setting it to true here we can have a valid check per module.

@@ -153,7 +153,7 @@ local function get_lua_path(conf)
end


local function init(env)
local function init_sysconf(env)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain what have you done here?

Copy link
Contributor Author

@boekkooi-lengoo boekkooi-lengoo Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extracted this method from local function init(env) so it can also be used by local function test_standalone(env).

As the information from init_sysconf is used to render apisix.cli.ngx_cli_tpl.

@monkeyDluffy6017
Copy link
Contributor

hi @boekkooi-lengoo the code is a bit complicated, could you describe your thought in detail?
Which part of apisix.yaml can be validated?
Are you just going to check if the plugin is loaded correctly from your demos?

@boekkooi-lengoo
Copy link
Contributor Author

Hey @monkeyDluffy6017

I agree the code is a bit complicated.
So my original goal was to validate the apisix.yaml schema (using "apisix.core.schema") at a minimum and if possible to validate the values of the config file as well.

Now it turns out that to use "apisix.core.schema" I needed to require some of the ngx modules.
To do this I created a setup similar to https://github.com/openresty/resty-cli/ which allows the execution of lua in nginx as a command line command.
This allow the ability to run code with all the requires modules.

After some reseacth it seems that it would be very hard to simply validate schemas as it seems that each module/part is doing validation differently.
So for the sake of the prototype I split each module/part to it's own validation call.

Now for the validation we assume that any call to the logger with level error means that the configuration is invalid.

Personally this entire thing feels a bit hacky but due to my lack of understanding of the internal design choices and lua knowledge I wanted to show this so we could analyses and discuss better options.

I hope this clarifies my thoughts a bit.

@monkeyDluffy6017
Copy link
Contributor

ok, we will figure out your design, please be patient

@monkeyDluffy6017
Copy link
Contributor

We have the command apisix test already, but it only can validate the generated nginx conf file, how can we unify the two functions?

@boekkooi-lengoo
Copy link
Contributor Author

@monkeyDluffy6017 I don't really understand your question as this code already adds the validation to apisix test in https://github.com/apache/apisix/pull/10630/files#diff-3936316fb5318f20b79ffe05648cf3d6b8c61e6987b51b52e2571c8a8e73a68bR795 can you maybe clarify what you mean?

@monkeyDluffy6017
Copy link
Contributor

@boekkooi-lengoo what i mean is that the apisix test comman only test nginx.conf before, you add the ability to test apisix.yaml, Can the 2 functions be confused?

@boekkooi-lengoo
Copy link
Contributor Author

@monkeyDluffy6017 they could be confusing as they confused me already because I assumed wrongly that apisix test would test the apisix.yaml. However, we could create a apisix text-standalone or something along those lines command.

@monkeyDluffy6017
Copy link
Contributor

OK, LGTM, please go ahead

@boekkooi-lengoo
Copy link
Contributor Author

Hey y'all,
I'm sorry but I sadly don't have time to spend on this feature at the moment.
For this reason I'm closing this PR.

Thanks again for the reviews, input and support I very much appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: validate apisix.yaml before deployment for standalone mode
3 participants