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

change emissary CRDs to non-strict + update the CRDs #155

Merged
merged 6 commits into from
Aug 1, 2023

Conversation

FrancoisPoinsot
Copy link
Contributor

How it started

I was trying kubecomform with emissary CRDs and was getting some odd error on the schema.

example with this file emissary.yaml :

apiVersion: getambassador.io/v2
kind: Mapping
metadata:
  name: emissary
  namespace: default
spec:
  ambassador_id:
  - this-ambassador
  host: some-host.dev
  prefix: /
  service: some-service.this-namespace.cluster.local

the command:

kubeconform  -schema-location default -schema-location 'https://raw.githubusercontent.com/datreeio/CRDs-catalog/main/{{.Group}}/{{.ResourceKind}}_{{.ResourceAPIVersion}}.json'  kubeconform/emissary.yaml

kubeconform/emissary.yaml - Mapping emissary is invalid: problem validating schema. Check JSON formatting: jsonschema: '/spec' does not validate with https://raw.githubusercontent.com/datreeio/CRDs-catalog/main/getambassador.io/mapping_v2.json#/properties/spec/additionalProperties: additionalProperties 'ambassador_id' not allowed

A bit of history around emissary CRDs

There has been a few changes with emissary CRDs when kubernetes started to enforce a new standard for schema of the CRDs around k8s 1.22.
And to comply, emissary team had to release a v3 version of their schema.

The v1 and v2 lost some field declaration to pass the new requirement from k8s.
They are intended to be used with some unkown field.
Such as ambassador_id, which could not be anymore a "string or array".

The problem in the schema of this repo

The script used to extract the json schema from the CRDs essentially adds additionalProperties: false everywhere, which seems to denies the ability to have unkown field in the resource you validate.

The bit of code is the reason why it is added:
https://github.com/yannh/kubeconform/blob/9294e94a8d6471ed825e98401b36a5783382080a/scripts/openapi2jsonschema.py#L28
This kind of schema would be considered the "strict" version for kubeconform.

This is kind of related to that issue: #102
Except here, I am missing the non-strict version of the schema.

Support for strict and non-strict

For kubeconform, having a strict and non-strict version of the scema is resolved using . StrictSuffix var in -schema-location
I see that on this repository, there is only 1 other schema that has a strict and non-strict version: the openshift ones.
But I don't think I can reuse that file structure without breaking how to use this repository.
I tried to get something mostly retro-compatible so that the template string used for -schema-location when calling kubeconform would not be completly broken.

Missing CRDs

I noticed some CRDs don't exist anymore:

  • ratelimit
  • filterPolicy
  • filter

I kept them around in this PR.

What that PR does

Running the validation

running against the same file as above:

kubeconform  -schema-location default -schema-location 'https://raw.githubusercontent.com/cognitedata/CRDs-catalog/emissary-strict/{{.Group}}{{.StrictSuffix}}/{{.ResourceKind}}_{{.ResourceAPIVersion}}.json'  kubeconform/emissary.yaml

@eyarz
Copy link
Member

eyarz commented Apr 15, 2023

thank you for the detailed PR :)
if we merge this PR, although all the schemas in this repo are strict, for the getambassador.io schemas, the user will need to be explicit (by adding the strict suffix) to get the strict schemas.

maybe we should put the non-strict schemas in their own dir so when a user will want to use the non-strict he will need to be explicit and add the suffix?

@FrancoisPoinsot
Copy link
Contributor Author

I am not sure what is the intented way to use kubeconform.

from the readme

StrictSuffix - "-strict" or "" depending on whether validation is running in strict mode or not

It seems to me there is there is some naming convention intended here, that the path to the strict variante contains literaly "-strict" and "" for the non-strict.
But then you still need to add -strict argument for the cli anyway.
Since this decision will matter for this repository, I will follow whatever naming comvention you want to set

Also, thinking back to this PR, I should not have edited the v3 version of the schema.
I will at least make another commit to remove that part (keeping the upgrade though)

@eyarz
Copy link
Member

eyarz commented Apr 19, 2023

I agree this behavior is different from kubeconform, but as you mentioned, it is a separate project, so I'm OK with that.
It will make much more sense to put the non-strict schemas in their own dir for backward compatibility reasons (all the schemas in this repo are strict by default)

@FrancoisPoinsot
Copy link
Contributor Author

I thought a bit about that situation.
it make sens to keep a single opinionated version of the schema and have it be strict.

And the case with emissary is unusual. It just so happen the v1 and v2 were updated way after they were created as a fix to k8s 1.22 breaking changes.
As far as I know, they are kind of intended to be used in a non-strict way now, but only because it was the only way to have them work on k8s >= 1.22.
This strictness loss is a problem and a reason why it is recommended to migrate to v3

I think I will make that PR simpler:

  • keep only 1 version of the resources I added
  • keep all the resources in the same folder as before
  • have v3 generated as strict
  • have v1 and v2 generated as non-strict
  • don't touch the ratelimit, filterPolicy, or filter
  • still use to the latest CRDs definition (v3.5.0) to regenerate all the schemas

I think this is the easiest way to validate schemas in a sane way.
It will abstract away this specificity due to history for any user of this schema repo.

would you be ok with that?

@eyarz
Copy link
Member

eyarz commented Apr 20, 2023

sounds good to me :)
ping me when the PR is a ready for review

@eyarz eyarz self-requested a review April 20, 2023 06:34
@eyarz
Copy link
Member

eyarz commented Jul 18, 2023

@FrancoisPoinsot do you want to work on it or should I close this?

@FrancoisPoinsot
Copy link
Contributor Author

right, I keep delaying
I still intend to finish that

specifically using this version of the kubeconform script:
yannh/kubeconform#225
@FrancoisPoinsot
Copy link
Contributor Author

FrancoisPoinsot commented Jul 31, 2023

Ok so in the end I figured a bit more precisly what fields should or should not receive this "additionalProperties": false
Made a PR to fix the original kubeconform script: yannh/kubeconform#225
More info about that issue in the related PR.

This last commit of the current PR is the result running this version of the script.
Didn't even had to make a special rules for v3 schema.

It is now reviewable @eyarz

@eyarz eyarz merged commit 2abdb7a into datreeio:main Aug 1, 2023
@FrancoisPoinsot FrancoisPoinsot deleted the emissary-strict branch August 2, 2023 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants