Add option to enable/disable unknown fields in OCI config #12052
Conversation
|
Please squash the commits into 1. Currently this branch has 7 commits. If we were to merge this, all the intermediate commits will make it to the master branch. |
ioterw
force-pushed
the
allow_unknown_fields
branch
from
9c3cb2d to
e740c7e
Compare
|
Should be good |
runsc/config/config.go
Outdated
|
|
||
| // AllowUnknownFields allows unknown fields in the OCI config. If disabled, | ||
| // unknown fields will result in an error. | ||
| AllowUnknownFields bool `flag:"allow-unknown-fields"` |
There was a problem hiding this comment.
Seems a bit too vaguely-named. How about allow-unknown-oci-spec-fields?
There was a problem hiding this comment.
I agree. Changed to allow-unknown-oci-spec-fields
runsc/specutils/specutils.go
Outdated
| decoder := json.NewDecoder(bytes.NewReader(specBytes)) | ||
| if !conf.AllowUnknownFields { | ||
| decoder.DisallowUnknownFields() | ||
| } |
There was a problem hiding this comment.
Sorry I only thought about this after seconds review, but what do you think about removing the flag, and instead having the behavior here be like:
- Try to parse with the
DisallowUnknownFieldsoption set. - If it succeeds, great, go ahead.
- If it fails, try to parse again, this time without the
DisallowUnknownFieldsoption. - If that fails too (malformed JSON?), return the error from the second parsing attempt.
- If the second parsing attempt succeeds, then log a warning with the error from the first parsing attempt, but still continue with the originally-decoded JSON.
This way avoids introducing a new flag, and also makes any lack of specific OCI option support in runsc more obvious even with default flags.
There was a problem hiding this comment.
Not sure what is better in general, but looks fine for me too. Can change a bit later today :)
ioterw
force-pushed
the
allow_unknown_fields
branch
from
4dfb562 to
fc2f4df
Compare
|
No more fields. Only small warning. Should we make it more loud? Or small log.Warningf is fine? |
runsc/specutils/specutils.go
Outdated
| return nil, fmt.Errorf("error unmarshaling spec from file %q: %v\n %s", specFile.Name(), err, string(specBytes)) | ||
| decoder := json.NewDecoder(bytes.NewReader(specBytes)) | ||
| decoder.DisallowUnknownFields() | ||
| if err := decoder.Decode(&spec); err != nil { |
There was a problem hiding this comment.
Please give the 2 errors distinctive names to differentiate them from each other, like errStrictDecode and errLooseDecode or something.
runsc/specutils/specutils.go
Outdated
| if err2 := json.Unmarshal(specBytes, &spec); err2 != nil { | ||
| return nil, fmt.Errorf("error unmarshaling spec from file %q: %v\n %s", specFile.Name(), err2, string(specBytes)) | ||
| } else { | ||
| log.Warningf("Problem with spec file %q: %v. Consider removing unnecessary fields.", specFile.Name(), err) |
There was a problem hiding this comment.
This can only happen in the case where there are unknown fields, and the message should also make it clear that this isn't a fatal error because the fields will be ignored.
log.Warningf("OCI spec file %q contains fields unknown to `runsc`: %v. Ignoring these fields and continuing anyway.")
|
@EtiennePerot Thx for showing the way. Good to squash? |
Currently in OCI config it is possible to add any extra json data, which will be ignored by runsc. I found it is a bit confusing when was learning OCI implementation of gvisor (adding wrong/non existent fields does not produce any error or warning), therefore decided to add flag `allow-unknown-fields`, which is `true` by default (does not modify current behavior). In case of being `false`, gvisor produces error like this: ``` reading spec: error unmarshaling spec from file "config.json": json: unknown field "oooooo" ``` FUTURE_COPYBARA_INTEGRATE_REVIEW=#12052 from ioterw:allow_unknown_fields 0662917 PiperOrigin-RevId: 797846468
Currently in OCI config it is possible to add any extra json data, which will be ignored by runsc. I found it is a bit confusing when was learning OCI implementation of gvisor (adding wrong/non existent fields does not produce any error or warning), therefore decided to add flag `allow-unknown-fields`, which is `true` by default (does not modify current behavior). In case of being `false`, gvisor produces error like this: ``` reading spec: error unmarshaling spec from file "config.json": json: unknown field "oooooo" ``` FUTURE_COPYBARA_INTEGRATE_REVIEW=#12052 from ioterw:allow_unknown_fields 0662917 PiperOrigin-RevId: 797846468
3 participants
Currently in OCI config it is possible to add any extra json data, which will be ignored by runsc.
I found it is a bit confusing when was learning OCI implementation of gvisor (adding wrong/non existent fields does not produce any error or warning), therefore decided to add flag
allow-unknown-fields, which istrueby default (does not modify current behavior).In case of being
false, gvisor produces error like this: