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 tinyyaml to lyaml #11312

Merged
merged 35 commits into from
Jun 3, 2024
Merged

Conversation

bzp2010
Copy link
Contributor

@bzp2010 bzp2010 commented Jun 2, 2024

Description

APISIX now uses a YAML parser called tinyyaml, which has a number of problems, such as:

  • accept use cases that do not conform to the YAML specification
routes:
    -       # Such indentation is obviously wrong, but certain developers think it is acceptable. Unbelievable!
    id: 1
    uri: "/test"
such-key:
    - id: second
      service:
        schema: "http",    # This is also obviously wrong; YAML does not need to use "," as a end character.
        host: "127.0.0.1", # This is not "optional", the standard parser does not accept this error at all.
  • No support for the new YAML standard (e.g. anchors syntax)
upstream:
  - &u
    nodes: xxx
    type: roundrobin

routes:
  - id: 1
    upstream: *u
  - id: 2
    upstream: *u

There have been many issues logged for tinyyaml. https://github.com/apache/apisix/issues?q=tinyyaml

This PR therefore introduces a change to replace tinyyaml with a wrapper for the libyaml C library, which is the C reference implementation of the YAML project. It is more standardized and advanced.


Theoretically, if the user is using a fully YAML-compliant document, the change is insensitive. Otherwise it will report an error.

So I think it's largely backward compatible. But as stated in the background above, in the past our developers created so many buggy test cases that they had to be fixed one by one. The good thing is that they have been fixed completely and without substantially changing the testing process.

The outliers in config-default.yaml have also been fixed.

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)

Copy link
Member

@juzhiyuan juzhiyuan left a comment

Choose a reason for hiding this comment

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

LGTM

@bzp2010
Copy link
Contributor Author

bzp2010 commented Jun 3, 2024

Please wait for some more REVIEWERS and don't merge for now.

请再等待一些reviewer,暂时不要合并。

Copy link
Contributor

@ShenFeng312 ShenFeng312 left a comment

Choose a reason for hiding this comment

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

LGTM

@bzp2010
Copy link
Contributor Author

bzp2010 commented Jun 3, 2024

Regarding the changes in config-default.yaml:

  • 1:2 will be parsed as number 62 instead of "1:2" in string
  • http&stream contains the start character & of the YAML anchor syntax, which causes parsing errors

Copy link
Contributor

@Revolyssup Revolyssup left a comment

Choose a reason for hiding this comment

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

I think this hack can be removed https://github.com/apache/apisix/pull/11080/files#diff-ab3c55c5825b9d6c1f2a2e296c658ea10092998b5a53feca14fd3988bcf8209aR87
where a placeholder string was replaced to get around null problems. Will you remove this in this PR?

@moonming
Copy link
Member

moonming commented Jun 3, 2024

  • http&stream contains the start character & of the YAML anchor syntax, which causes parsing errors

Do we need to change it to http, stream?

@bzp2010
Copy link
Contributor Author

bzp2010 commented Jun 3, 2024

Reply to @moonming

Do we need to change it to http, stream?

This should be controlled by some boolean value, e.g.

proxy_mode:
  http: true
  stream: true

It should even be further integrated with port settings, e.g.

listen:
  - port: 443
    http: true
  - port: 5432
    stream: true

These bad designs will be corrected by me (maybe), but not in this PR. there are many things just waiting to be revolutionized.

@bzp2010
Copy link
Contributor Author

bzp2010 commented Jun 3, 2024

Reply to @Revolyssup

It could probably be deleted, but I don't want it here. Because I also want to make some changes to where the YAML is generated.

The reason: overwriting the old config.yaml file with the regenerated YAML does not preserve user comments, which may break some user efforts.

So I think it's better to generate it at runtime and use logging output or output to file like conf/.random_admin_key

@moonming
Copy link
Member

moonming commented Jun 3, 2024

Reply to @moonming

Do we need to change it to http, stream?

This should be controlled by some boolean value, e.g.

proxy_mode:
  http: true
  stream: true

It should even be further integrated with port settings, e.g.

listen:
  - port: 443
    http: true
  - port: 5432
    stream: true

These bad designs will be corrected by me (maybe), but not in this PR. there are many things just waiting to be revolutionized.

This design was imported by #9600, looking forword a better one.

@bzp2010
Copy link
Contributor Author

bzp2010 commented Jun 3, 2024

This design was imported by #9600, looking forword a better one.

Yes, the current design is not good but it is still work, so it will not be modified in this PR.

If we plan to modify it, we should design it first and switch between APISIX 3 and APISIX 4.

@bzp2010 bzp2010 requested a review from Revolyssup June 3, 2024 09:21
@bzp2010 bzp2010 merged commit cf84292 into apache:master Jun 3, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants