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

breaking changes #558

Closed
nsitbon opened this issue Dec 4, 2019 · 7 comments
Closed

breaking changes #558

nsitbon opened this issue Dec 4, 2019 · 7 comments

Comments

@nsitbon
Copy link

nsitbon commented Dec 4, 2019

Hi I just want to let you know that between v2.0.0 and v2.2.7 at least 2 breaking changes sneaked in without you bumping major version. The first one is the ability to use forward reference for anchors and the second comes from this PR #515 I have yaml files that don't parse anymore because of yaml: document contains excessive aliasing. For the first breaking change I manage to reorder properly the anchors and reference but for the second BC I don't know what to do and am stuck.
Please do follow semver as it's a nightmare for us to manage particularly using go module (you can't stick to a particular version).

Best regards.

@niemeyer
Copy link
Contributor

niemeyer commented Dec 4, 2019

Do you have an example of the first breaking change at hand? I don't recall seeing anything that should have affected this.

The second breaking change is intended. This prevents abusive use of yaml as described in:

https://en.wikipedia.org/wiki/Billion_laughs_attack

Do you have a realistic document showing this being an issue for your case?

@nsitbon
Copy link
Author

nsitbon commented Dec 4, 2019

for the first BC:

git clone git@github.com:nsitbon/demo-yaml-bc.git
cd demo-yaml-bc
git checkout v2.0.0
./build.sh
./launch.sh

git checkout v2.2.7
./build.sh
./launch.sh

For the second BC I need to check with my boss if I can disclose our YAML files but whether I can or not that's a breaking change anyway.

@niemeyer
Copy link
Contributor

niemeyer commented Dec 4, 2019

Thanks. The snippet at hand is this:

hello:
  <<: *mydefaults
mydefaults: &mydefaults
  greeting: "Hello"

This is invalid yaml, and go-yaml stopped supporting that in v2.1.0, more specifically with add015b which went in two years ago.

For the second BC I need to check with my boss if I can disclose our YAML files but whether I can or not that's a breaking change anyway.

The problem at hand is that the abuse described is being used in the wild to break down services, and because it's valid yaml, to prevent the attack we need to reject some class of valid documents. It's a trade-off. I'm happy to evaluate where to draw the line in terms of valid or invalid documents, but the decision to draw that line somewhere has already been taken and that's for everyone's benefit.

@niemeyer niemeyer closed this as completed Dec 4, 2019
@nsitbon
Copy link
Author

nsitbon commented Dec 4, 2019

I know that in the first case according to the specification it's invalid yet it worked in 2.0.0 so that's a BC and it requires a bump of the major version. Same thing for the second problem from my point of view my yaml is trusted in my organization and suddenly it's not working anymore without bumping major version once again. I don't see my benefits in both cases. Please follow semver with go module of do not use semver at all. Golang has special rules for both cases which work great if you don't violate the rules.

@niemeyer
Copy link
Contributor

niemeyer commented Dec 4, 2019

I understand your feeling completely. I hate breaking changes, and try hard not to have them. I would almost certainly have rejected this change if the issue was raised two years ago when it was integrated. At the same time, if you rely on broken yaml that happens to be parsed correctly due to the way the implementation used to work, and the implementation changes to respect the specification, and then you come to me two years later to complain... 🤷‍♂️

@nsitbon
Copy link
Author

nsitbon commented Dec 4, 2019

Well first of all nothing prevents someone from supporting more feature than the original specification (for example the GNU C compiler supports more feature than the draft) so for me it's ok to rely on extra behaviour. Secondly semver has no notion of time and what works on 2.0.0 must work on 2.2.7 period. Anyway like I said I manage to work around the first BC but for the second I'm stuck. You could have at least provide a way to disable the check...

@niemeyer
Copy link
Contributor

niemeyer commented Dec 4, 2019

First of all, the spec requires ordering to be implemented correctly, as you can read by yourself.

Second of all, this is the kind of interaction that makes me regret publishing source code.

@go-yaml go-yaml locked as too heated and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants