kedge does not fail when extra fields specified

[concaf]

I have an extra field in my kedge definition -

name: httpd
batman: true              # extra field
containers:
- image: centos/httpd
services:
  - name: httpd
    type: NodePort
    ports:
    - port: 8080
      targetPort: 80

Running kedge generate on this file does not fail and simply ignores the extra field, while it should have failed saying something like unknown field 'batman' passed as input

This creates a lot of room for error since a simple spelling mistake in the spec will render the generated artifacts useless and the user will never get to know about it.

[kadel]

This is not going to be easy thing to solve :(

[surajssd]

If it would have been yaml we could have done something like we did in opencompose.
Here while unmarshalling we just create another struct with our struct embedded in it and then another field for LeftOvers of type map[string]interface{}. So if there is any data in LeftOvers we know extra data is given.

But with json there is no way like that we can do:

There are lot of manual ways to handle it like the following which is influenced from stackoverflow question.

package main

import (
	"encoding/json"
	"fmt"
)

type Foo struct {
	A int                    `json:"a"`
	B int                    `json:"b"`
	X map[string]interface{} `json:"-"` // Rest of the fields should go here.
}

func main() {
	s := `{"a":1, "b":2, "x":1, "y":1}`
	f := Foo{}
	if err := json.Unmarshal([]byte(s), &f); err != nil {
		panic(err)
	}
	// 'f' has:
	// {A:1 B:2 X:map[]}


	if err := json.Unmarshal([]byte(s), &f.X); err != nil {
		panic(err)
	}
	// 'f' has:
	// {A:1 B:2 X:map[a:1 b:2 x:1 y:1]}
	
	// now manually delete fields we know are valid from
	// 'X' which is 'a' and 'b'
	delete(f.X, "a")
	delete(f.X, "b")

	// 'f' has:
	// {A:1 B:2 X:map[x:1 y:1]}
}

I am not sure if wanna do it this way.

Also there is an open issue in golang upstream for adding support to json tags to find the extra fields golang/go#15314 .

When I was working on opencompose health feature redhat-developer/opencompose#143 I came across this problem so we(me and @kadel) decided that we won't do extra fields checks for health field.

[surajnarwade]

@surajssd @kadel , I am trying to do it with Jsonschema and gojsonschema,
I am getting same error (not so explainatory),

format must be a valid format int32

[concaf]

fwiw, @surajnarwade and I sat down brainstorming this issue last week, and we're 90% there, some code remains.
Let's get that in, in some time. The code is very similar to #135, so I can help review it.

[kadel]

Is there any progress on this?

[concaf]

@surajnarwade let me know whenever you have time, we can sit and finish this off

[kadel]

@surajnarwade are you working on this? What is progress?

[surajnarwade]

@containscafeine @kadel , I wasn't working on it, I did some investigation though, rather than code hack as @surajssd mentioned, if we add additionalkey: false, something like this in Jsonschema only which will identify the extra key and will give an error

[surajssd]

@surajnarwade can you try whatever you are doing with the jsonschema at https://github.com/kedgeproject/kedge-jsonschema/blob/master/configs/deploymentspecmod.json

[kadel]

@surajnarwade can you try whatever you are doing with the jsonschema at https://github.com/kedgeproject/kedge-jsonschema/blob/master/configs/deploymentspecmod.json

the link is not valid

[surajssd]

@kadel this should work https://github.com/kedgeproject/json-schema/blob/master/schema/deploymentspecmod.json

Today I separated the configs/schema from the schema generator!

[kadel]

@kadel this should work https://github.com/kedgeproject/json-schema/blob/master/schema/deploymentspecmod.json

I'm confused from content of that directory :-(
Do we need all the other files?

[surajssd]

@kadel not sure yet so keeping them!

[concaf]

@surajssd @surajnarwade correct me if I'm wrong, but the JSON schema is not helping here, right? Is this because Kubernetes does not support this?
If this is the case, we need to do this via reflect.
This is bug, let's fix this soon-ish.

During demos, I've come across this multiple times since extra fields are not caught, bumping priority.

[concaf]

Related PR #192
Well, the PR is blocked on kubeval, which is blocked on a couple of Kubernetes issues, this is going to take some time :|
Idk if we should push a temporary fix for now and later switch to upstream, or remain blocked!

[kadel]

how is this blocked on kubeval?
there is no other way to validate yaml files against json schema?

[concaf]

how is this blocked on kubeval?

ping @surajnarwade

there is no other way to validate yaml files against json schema?

we could validate using reflect magic on golang structs, but I'm not aware of anything to validate against the JSON schema.

[kadel]

Why we can't use one of those?:

• https://github.com/xeipuuv/gojsonschema
• https://github.com/lestrrat/go-jsschema

There is even talk about it from one really smart guy https://www.slideshare.net/surajssd009005/jsonschema-with-golang ?

@surajssd

Or Am I missing something?

[surajssd]

There is even talk about it from one really smart guy

LOL 🙈

@surajnarwade told the library has some additional checks wrt kubernetes and openshift specifically!

[surajnarwade]

@surajssd @containscafeine @kadel , once instrumenta/openapi2jsonschema#4 gets solved, we are ready to sort out this issue.
(it will not take much time)

[surajnarwade]

Why we can't use one of those?:

https://github.com/xeipuuv/gojsonschema

Problem is not with gojsonschema module, problem is with json schema of kubernetes, there is no mechanism to validate root level extra fields.
kubeval does upto some extent. I have opened up an issue there, instrumenta/openapi2jsonschema#4
once it gets resolved, we are good to go

[kadel]

Can we somehow create a workaround for it?

Can we inject "additionalProperties": false in our end?

[surajnarwade]

@kadel, we can do this, I will send temporary fix which will validate everything except root level extra fields.
when upstream issue gets fixed, it will automatically reflect in our jsonschema (so no extra work in future )
@kadel, @surajssd WDYT ?

[kadel]

@kadel, we can do this, I will send temporary fix which will validate everything except root level extra fields.

It means that it will validate extra keys that are in deeper levels, just root level's won't be validated?
That would be great 👍

when upstream issue gets fixed, it will automatically reflect in our jsonschema (so no extra work in future )

There was no movement in the upstream repo. If it doesn't change maybe we should fork and fix it ourselves.

[surajnarwade]

it seems there's some problem with JSON schema, they are reported here
kedgeproject/json-schema-generator#15,
kedgeproject/json-schema-generator#14

[kadel]

any progress on this?

[surajnarwade]

@kadel , here is the PR #481, I am trying to keep this validation as a optional part, something like

kedge validate -f <kedgefile>

Because, our JSON schema is not stable yet

[kadel]

We should validate it automatically by default, and have flag for disabling it if needed.

We need to work on stabilizing it.
What can we do stabilize it?
Is there another way to detect extra fields other than jsonSchema?

[surajnarwade]

Closing this Since #481 got merged