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

Strict schema for k8s objects for values.yaml #19181

Merged
merged 4 commits into from
Dec 5, 2021

Conversation

mik-laj
Copy link
Member

@mik-laj mik-laj commented Oct 23, 2021

Some fields in values.yml accept value that complies with the Kubernetes API, so we can verify that too.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@mik-laj
Copy link
Member Author

mik-laj commented Oct 23, 2021

We are now only missing a few types to have full and strict coverage.
Before:

$ cat chart/values.schema.json | jq 'recurse | objects | select(.type == "object") | select(.properties == null) | select(.["$ref"] == null) | select(.additionalProperties == null)' | jq -s 'length'
57
$ cat chart/values.schema.json | jq 'recurse | objects | select(.type  == "array") | select(.items == null)' | jq -s 'length'
51

After:

$ cat chart/values.schema.json | jq 'recurse | objects | select(.type == "object") | select(.properties == null) | select(.["$ref"] == null) | select(.additionalProperties == null)' | jq -s 'length'
3
$ cat chart/values.schema.json | jq 'recurse | objects | select(.type  == "array") | select(.items == null)' | jq -s 'length'
10

@mik-laj
Copy link
Member Author

mik-laj commented Oct 23, 2021

CC: @simplylizz

Copy link

@0ex-d 0ex-d left a comment

Choose a reason for hiding this comment

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

Still looks like WIP to me, keep it up

@mik-laj
Copy link
Member Author

mik-laj commented Oct 25, 2021

@mrbaguvix I have currently solved all the issues with the structure of objects. Now only the issues with the structures of the array remain, but I think we can solve that as a separate contribution. It is important to me that most of the problems are resolved, so following contributions that follow the conventions in this file will also declare the complete structure of the objects. Previously, even when someone wanted to use a reference, they weren't sure if it was a good idea. Here is example: #19175 (comment)

@mik-laj
Copy link
Member Author

mik-laj commented Nov 2, 2021

I have resolved all the problems except the following

$ cat chart/values.schema.json | jq 'recurse | objects | select(.type  == "array") | select(.items == null)'

{
  "description": "HTTP paths to add to the web Ingress before the default path.",
  "type": "array",
  "default": []
}
{
  "description": "HTTP paths to add to the web Ingress after the default path.",
  "type": "array",
  "default": []
}
{
  "description": "Additional mappings for StatsD exporter.",
  "type": "array",
  "default": []
}

1 and 2: I don't know the correct structure. This field is also not covered by tests.
3: I did not find the schema.json file for this parameter.

chart/values.schema.json Outdated Show resolved Hide resolved
chart/values.schema.json Outdated Show resolved Hide resolved
chart/values.schema.json Show resolved Hide resolved
chart/values.schema.json Outdated Show resolved Hide resolved
Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

I'm wondering if we should hold this until after we cut 1.3.0 and have our next release be 2.0.0. I could easily see folks having extra config in their values and this stricter schema being surprising. Thoughts?

chart/tests/test_flower.py Outdated Show resolved Hide resolved
chart/tests/test_flower.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Nov 3, 2021
@github-actions
Copy link

github-actions bot commented Nov 3, 2021

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@mik-laj
Copy link
Member Author

mik-laj commented Nov 3, 2021

I could easily see folks having extra config in their values and this stricter schema being surprising. Thoughts?

As for fields that have a structure defined with an external reference, users cannot define additional fields as this will cause an invalid k8s object to be generated. Here is an example: #19379

The problem is only with the objects that are defined in this schema, but we can remove them from this contribution and change them in the future.

@mik-laj mik-laj force-pushed the strict-schema-values branch 2 times, most recently from ed4be14 to 6c95d20 Compare November 7, 2021 13:46
@mik-laj
Copy link
Member Author

mik-laj commented Nov 7, 2021

I deleted all added additionalProperties fields. It should be safe now.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM. There are some helm unit tests failures though.

@mik-laj
Copy link
Member Author

mik-laj commented Dec 4, 2021

I looked at the CI problem and I think the OpenAPI specification has a bug.

ResourceQuotaSpec has the following definition:
https://github.com/yannh/kubernetes-json-schema/blob/master/v1.21.0/resourcequotaspec.json

{
  "properties": {
    "hard": {
      "additionalProperties": {
        "$ref": "https://raw.githubusercontent.com/yannh/kubernetes-json-schema/master/v1.21.0/_definitions.json#/definitions/io.k8s.apimachinery.pkg.api.resource.Quantity"
      },
      "type": [
        "object",
        "null"
      ]
    },
...

and
quantity has been defined as follow:
https://github.com/yannh/kubernetes-json-schema/blob/master/v1.21.0/quantity.json

{
  "oneOf": [
    {
      "type": "string"
    },
    {
      "type": "number"
    }
  ],
  "$schema": "http://json-schema.org/schema#",
  "type": "object"
}

The fields oneOf and type are mutually exclusive. For now, I've copied the type definitions directly into the spec.

@mik-laj
Copy link
Member Author

mik-laj commented Dec 5, 2021

Here issue: yannh/kubernetes-json-schema#11

@mik-laj mik-laj changed the title More strict schema for values.yaml Strict schema for k8s objects for values.yaml Dec 5, 2021
@mik-laj mik-laj merged commit 2391b19 into apache:main Dec 5, 2021
@jedcunningham jedcunningham added this to the Airflow Helm Chart 1.4.0 milestone Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants