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

Give validation errors on unknown config fields #1246

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

kke
Copy link
Contributor

@kke kke commented Nov 4, 2021

Signed-off-by: Kimmo Lehto klehto@mirantis.com

As mentioned in #1237 (comment) - the unknown field validation has not been working as intended since the move to https://sigs.k8s.io/yaml

Seems what breaks it is the UnmarshalJSON function which runs basic json.Unmarshal, which will lose the strictness. This PR makes it use the DisallowUnknownFields JSON decoder option.

@kke kke requested a review from a team as a code owner November 4, 2021 15:10
@kke
Copy link
Contributor Author

kke commented Nov 4, 2021

I don't know what the "UnmarshalStrictIgnoringFields" was made for, apparently a field called "interval" has caused problems, maybe telemetry related.

@kke
Copy link
Contributor Author

kke commented Nov 4, 2021

There's also decoder.DisallowUnknownFields() for json 🤔

@kke kke force-pushed the fix-strict-yaml-validation branch 3 times, most recently from 1643f78 to 6e7ea2f Compare November 5, 2021 12:57
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

Tab sneaked into test

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

Better implementation

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
}
return nil
decoder := json.NewDecoder(bytes.NewReader(data))
decoder.DisallowUnknownFields()
Copy link
Contributor

@trawler trawler Nov 9, 2021

Choose a reason for hiding this comment

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

the yaml package already turns DisallowUnknownFields on. Will turning it on here solve our issue?
https://github.com/kubernetes-sigs/yaml/blob/39f74b9b8a135185d095704ab98c760ca031a9f8/yaml.go#L63-L64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The json.Unmarshal in the UnmarshalJSON function creates a new decoder that doesn't know anything about the DisallowUnknownFields set by the yaml thing.

@trawler
Copy link
Contributor

trawler commented Nov 9, 2021

Seems what breaks it is the UnmarshalJSON function which runs basic json.Unmarshal, which will lose the strictness. This PR makes it use the DisallowUnknownFields JSON decoder option.

The yaml.Unmarshalstrict is already turning on the DisallowUnknownFields option for the json unmarshaller. If this is not working as intended, I think a better route would be to submit a fix upstream.

@kke
Copy link
Contributor Author

kke commented Nov 9, 2021

I think a better route would be to submit a fix upstream.

I don't think it's possible to fix in upstream. The UnmarshalJSON function does not work like UnmarshalYAML, it does not receive a function to be called to do the unmarshaling:

func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
  	// do stuff
  	
  	if err := unmarshal(yc); err != nil { // this function can inherit options from the original decoder
		return err
	}
	
	return nil
}
func (c *Config) UnmarshalJSON(data []byte) error {
  	// do stuff

	if err := json.Unmarshal(data, c); err != nil { // this can not
		return err
	}

	return nil
}

@kke
Copy link
Contributor Author

kke commented Nov 9, 2021

Btw, I think the DisallowUnknownFields only works case-insensitively. The json decoder, I think, will accept "aPiVeRSiON": "k0s.k0sproject.io/v1beta1" and put it in the APIVersion struct field without any errors.

@trawler
Copy link
Contributor

trawler commented Nov 9, 2021

Btw, I think the DisallowUnknownFields only works case-insensitively. The json decoder, I think, will accept "aPiVeRSiON": "k0s.k0sproject.io/v1beta1" and put it in the APIVersion struct field without any errors.

Yes, that is correct. There is an issue about this same problem already open upstream: kubernetes-sigs/yaml#15

@kke kke added area/cli area/configuration bug Something isn't working labels Nov 9, 2021
@kke kke merged commit 0c104a9 into k0sproject:main Nov 9, 2021
@kke kke deleted the fix-strict-yaml-validation branch November 9, 2021 14:03
ncopa pushed a commit to ncopa/k0s that referenced this pull request Nov 18, 2021
Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

Tab sneaked into test

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

Better implementation

Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants