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: move config-default.yaml to hardcoded lua file #11343

Merged
merged 24 commits into from
Jul 22, 2024

Conversation

bzp2010
Copy link
Contributor

@bzp2010 bzp2010 commented Jun 8, 2024

Description

For the reasons mentioned in the email https://lists.apache.org/thread/nc7qr5bl0z2bmkfb5rqc01c32lkx0gv2, I'm submitting this PR. The front blocking has been resolved #11312

It moves the config-default.yaml in APISIX to a hardcoded Lua. While it is still a script file in the distribution of APISIX, you can think of it as a "binary" and the user gets the clear and unambiguous message "it should never be modified".

And since it's a script file, it'll be more expressive than a pure Lua, e.g. we can read some of the values from environment variables (although I have a better way of doing that).


Email copy:

Hi, community.

I am posting to this mailing list a proposal to remove the config-default.yaml default configuration file, and below I will describe the background and reasons for this and consider the benefits to us.

The proposal does not represent a community resolution, although I am a committer in the APISIX community. We still want to hear a variety of voices to make sure this is truly beneficial.

Background

Now APISIX has two configuration files, the user configuration file config.yaml and the default configuration file config-default.yaml.
When APISIX starts up, it reads these two configuration files and merges and overwrites the user configuration file with the default configuration to create the configuration that is actually used at runtime.

The recommendation from the developers is that users should only modify the user configuration file config.yaml, while ensuring that config-default.yaml remains intact.
This is documented here: https://apisix.apache.org/docs/apisix/next/installation-guide/

APISIX's default configuration can be found in conf/config-default.yaml file and it should not be modified.
It is bound to the source code and the configuration should only be changed by the methods mentioned above.

Users often ask how they should modify custom configurations, or encounter unintended problems after modifying the default configuration file.
When explaining to them why they should not modify the default configuration file but copy those keys that need to be modified to the user configuration file and then modify them, and explaining the logic of merging the user configuration file with the default configuration file, I do find this issue to be a troubling one.

Also, some PRs encountered errors when improving config-default.yaml. For example: #11284

Let's shift our attention to other major open source projects in the world, I have indeed seen very few implementations with such built-in annoyances. Often software can have its default configuration hard-coded into the program, bundled into a binary for distribution, and the user can use a new configuration file, which they will load at runtime and override the modified keys into the default configuration.
This way, the user only needs to remember that he should modify config.yaml and nothing else; and APISIX will start normally without providing any configuration. This really reduces the cost of understanding.

Therefore, I suggest the following.

Proposal

  1. Remove config-default.yaml and move its contents to a Lua file as a Lua table
  2. Fix test errors
  3. Use documentation to explicitly document all configuration items, which ideally should be generated from the schema.

This is APISIX enhancement work, which is not perceived by users unless they are using APISIX in a non-correct way.

Benefit

  1. Reduce the cost of explaining about the section, and the cost of understanding for the user.
  2. Reduce the APISIX code base
  3. Improve the documentation quality through the entire process.

The End

The discussion is asking for feedback, and you can reply to the email directly to the mailing list.
We can discuss constructive feedback. If there is no constructive feedback, I will start working on it.

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)
@bzp2010
Copy link
Contributor Author

bzp2010 commented Jun 8, 2024

This PR has completed most of the work in the proposal, but the update of the documentation is not yet complete.

The schema included in APISIX for checking configuration files is outdated and not at all representative of the actual situation. So it cannot be used to generate usable documentation. It needs to be overhauled.
https://github.com/apache/apisix/blob/master/apisix/cli/schema.lua

Currently, many configurations are not properly documented, some incorrect fields are documented, and the JSON Schema library used by APISIX is highly non-standard and outdated. JSON Schema does not prohibit unlisted fields, so a user can certainly enter something that is not validated.

If the issue were to be addressed in this PR, this PR would become very large, which is not conducive to submission. So I would like to make changes in the next PR to include schema's and document generation.


Evidence:

Configurations not properly documented: https://github.com/apache/apisix/blob/master/apisix/cli/schema.lua#L323-L331

Non-existent fields: https://github.com/apache/apisix/blob/master/apisix/cli/schema.lua#L270-L277

@bzp2010 bzp2010 marked this pull request as ready for review June 8, 2024 18:55
moonming
moonming previously approved these changes Jun 9, 2024
conf/config-default.yaml Outdated Show resolved Hide resolved
conf/config-default.yaml Show resolved Hide resolved
t/core/etcd-mtls.t Show resolved Hide resolved
apisix/cli/config.lua Outdated Show resolved Hide resolved
nic-6443
nic-6443 previously approved these changes Jun 13, 2024
SkyeYoung
SkyeYoung previously approved these changes Jun 18, 2024
@bzp2010 bzp2010 changed the title feat: move config-default.yaml to hardcoded lua file Jul 4, 2024
@bzp2010 bzp2010 dismissed stale reviews from SkyeYoung, nic-6443, and moonming via 608a4c6 July 13, 2024 04:29
@bzp2010 bzp2010 changed the title [WIP] feat: move config-default.yaml to hardcoded lua file Jul 13, 2024
@bzp2010 bzp2010 requested review from juzhiyuan, shreemaan-abhishek, nic-6443, SkyeYoung and moonming and removed request for juzhiyuan July 13, 2024 05:48
@moonming moonming merged commit da5039d into apache:master Jul 22, 2024
37 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants