Schema a d by blackboxsw · Pull Request #1211 · canonical/cloud-init

blackboxsw · 2022-01-24T23:38:54Z

Based on James' schema-a-d PR 1155l migrate legacy schema definitions to static cloud-init-schema.json.

modules migrated, extended and schema unittests added:

cc_apt_configure
cc_bootcmd
cc_byobu
cc_chef (migrated)
cc_debug
cc_disable_ec2_metadata

Proposed Commit Message

schema: add static JSON schema for some a-c config modules

Migrate from legacy schema or define new schema in 
cloud-init-schema.json, adding extensive schema tests for:
- cc_apt_configure
- cc_bootcmd
- cc_byobu
- cc_ca_certs
- cc_chef
- cc_debug
- cc_disable_ec2_metadata
- cc_disk_setup

- Add schema alias `ca_certs.remove_defaults` for `ca-certs.remove-defauts`
- Add deprecation warnings for use or ca-certs and remove-defaults
- Extend apt_configure schema
   - Define more strict schema below object opaque keys using
     patternProperties
  -  create common $def apt_configure.mirror for reuse in 'primary' 
     and 'security' schema definitions within cc_apt_configure

 Co-Authored-by: James Falcon <james.falcon@canonical.com>

Additional Context

Test Steps

# review generated docs
PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs cc_apt_configure
PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs cc_bootcmd
PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs cc_byobu
PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs cc_chef
PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs cc_ca_certs
PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs cc_debug
PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs cc_disable_ec2_metadata

# Exercise valid/invalid schema
tox -e py3 tests/unittests/config

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

Move definition into cloud-init-schema.json

Migrate legacy apt_configure schema to new cloud-init-schea.json Add common $defs for 'primary' and 'security' keys. Use patterProperties for opaque "apt.sources" key. Make schema more strict by adding minProperties:1 and additionalProperties: False where applicable for objects. this should avoid some typos and accidental ommisions.

Migrate legacy chef schema to new cloud-init-schea.json. Add more strict schema definition disallowing additionalProperties. Add extensive unittests for invalid schemas.

migrate to statis cloud-init-schema.json and add unittests

Migrate to static cloud-init-schema.json and add unittests

Migrate to static cloud-init-schema.json and add unittests Needs more unittests for failure cases

holmanb

What I've seen looks good so far; I like the general approach. I still have more to review, but there are a couple of minor nits I'll share right now.

TheRealFalcon

LGTM! One inline nit and a question (in addition to Brett's comments).

You mentioned you thought there might be an issue with disk setup, but I don't remember the details. Can you explain the issue?

blackboxsw · 2022-01-31T15:53:26Z

+                  "type": ["string", "boolean", "array"],
+                  "default": false,
+                  "oneOf": [
+                    {"type": "string", "enum": ["auto", "remove"]},


@TheRealFalcon here is the error or inconsistency I was referring to in both disk-setup: documentation and schema definition I think. I can't find in our python code where layout of "auto" is honored or used so I think it might not be correct (maybe a thinko for the original module writer due to "partition" key supporting "auto"?

@blackboxsw , yes, good catch. auto isn't valid here. I suspect it may have been sometime in the past because there was also an example that had auto (that I removed), but apparently I missed taking it out of the schema. We should be able to safely remove it.

+1 will remove it now.

Add aliases in cloud-init schema and cc_ca_certs module to support both deprecated ca-certs.remove-defaults and ca_certs.remove_defaults. Update docs to show ca_certs instead of ca_certs. Also drop unused apt_configure.py mirror_property variable.

blackboxsw

@holmanb @TheRealFalcon I've just pushed additional changes to add a warning log about deprecation of "ca-certs" key in favor of "ca_certs" to make sure we don't suprise folks by rejecting that in the new strict schema.

Also, I've aliased a "ca-certs" key to "ca_certs" in the cloud-init schema so schema validation doesn't raise an error or warning on the deprecated ca-certs key.

Two more questions I have:

disk-setup: layout seems to include an "auto" value in both docs and schema, but I don't see the matching cc_disk_setup logic that honors this value. So, I'm not certain we need it.
apt_configure: has a convert_v1_to_v2_apt_format which supports a different schema for apt_configure than we have captured in our current strict schema. Shall we emit deprecation warnings for old uses of apt in this release?
Should we define our current strict schema to still support the "old schema" for apt_configure?

TheRealFalcon

apt_configure: has a convert_v1_to_v2_apt_format which supports a different schema for apt_configure than we have captured in our current strict schema. Shall we emit deprecation warnings for old uses of apt in this release?

Yes, I think that makes sense, especially since we don't even document v1 anymore.

Should we define our current strict schema to still support the "old schema" for apt_configure?

I don't think so. It's still just a warning, right? If they got the warning before, they'll continue to get the warning now.

TheRealFalcon · 2022-01-31T20:21:32Z

+    if "ca-certs" in cfg:
+        log.warning(
+            "DEPRECATION: key 'ca-certs' is now deprecated. Use 'ca_certs' by"
+            " version 23.1."


I don't think this date is reasonable unless it's only for upstream. In order to maintain backwards compatibility, we either will have to patch downstream or extend this date to April 2025.

Agreed, setting this drop version or date is arbitrary. Ideally, I'd like to drop this everywhere, but I don't know what line in the sand we should set for this. I don't love the idea of patching out behavior on older releases..... but in order to retain original behavior, maybe that is a debt we need to pay to move upstream forward to a unified/strict schema. We can sort how to handle deprecation timing separately in a different PR for these types of conditions (ca-certs -> ca_certs deprecation. and apt_configure v1 vs v2 deprecation).

TheRealFalcon · 2022-01-31T20:23:34Z

        return
-
-    ca_cert_cfg = cfg["ca-certs"]
+    else:


There's that small possibility they provide both ca-certs and ca_certs. In that case I think we should log a warning and use ca_certs.

+1 added warning and unit test

TheRealFalcon · 2022-01-31T20:28:00Z

+    if "remove-defaults" in ca_cert_cfg:
+        log.warning(
+            "DEPRECATION: key 'ca-certs.remove-defaults' is now deprecated."
+            " Use 'ca_certs.remove_defaults' by version 23.1."


Same thing about the date here

+1. as it is currently. I've just added the DEPRECATION warning and dropped the version suggestion from this PR. We can sort how/when to deprecate a number of schema related values as a separate PR since there will be multiple cases by the time we get through all cc_* modules.

holmanb · 2022-01-31T20:50:04Z

2. `apt_configure`: has a [convert_v1_to_v2_apt_format](https://github.com/canonical/cloud-init/blob/main/cloudinit/config/cc_apt_configure.py#L889) which supports a different schema for `apt_configure` than we have captured in our current strict schema. Shall we emit deprecation warnings for old uses of `apt` in this release?

I'm generally in favor of deprecating old formats so we can lay this kind of conversion code to rest. That said, user experience around these kinds of "forced changes" is probably important.

Do we have a well defined user story around handling config deprecation in cloud-init? If we don't, maybe we could enhance cloud-init devel schema to warn of deprecated config format usage (which would maybe check some metadata stored in the schema?). In my mind this lands outside the scope of this PR, but since this PR touches on deprecation in a couple different ways I wanted to bring it up to see what others' thoughts are.

holmanb

Spelling nits:

spellintian cloudinit/config/cloud-init-schema.json
cloudinit/config/cloud-init-schema.json: dependancies -> dependencies
cloudinit/config/cloud-init-schema.json: partion -> partition

blackboxsw · 2022-01-31T22:09:38Z

Do we have a well defined user story around handling config deprecation in cloud-init? If we don't, maybe we could enhance cloud-init devel schema to warn of deprecated config format usage (which would maybe check some metadata stored in the schema?). In my mind this lands outside the scope of this PR, but since this PR touches on deprecation in a couple different ways I wanted to bring it up to see what others' thoughts a

Unfortunately, we don't. I agree with a deprecation warning generally from schema validate to better raise awareness of that expected failure in the future. I've seen a couple of public JSON schemas that add a "deprecated" bool to a property in the schema definition but we also risk mixing metadata w/ true/strict schema definitions. Maybe it's still worth injecting that data into our schema to better raise awareness of the eventual drop of said properties. We could also leverage such an attribute for our generated docs to render that deprecation warning as well in readthedocs content. +1 on outside the scope of this PR, but let's talk on this and come up with a consensus on best approach to handle this case and put a PR during our schema work to address it.

Created this JIRA issue to track better deprecated cloud-config schema in docs/tooling/logs https://warthogs.atlassian.net/browse/SC-774

…ning

TheRealFalcon

Looks good to me! Thanks for getting this the rest of the way to the finish line.

blackboxsw requested review from TheRealFalcon and holmanb January 24, 2022 23:39

blackboxsw added 5 commits January 24, 2022 19:46

schema: migrate legacy cc_bootcmd schema definition

facd9a2

Move definition into cloud-init-schema.json

schema: pull in james cc_byobu schema into cloud-init-schema.json

f9b85d3

schema: pull in james' ca_certs schema. Migrate legacy schema

72c20c9

schema: migrate legacy cc_chef schema to cloud-init-schema

0e0f207

Migrate legacy chef schema to new cloud-init-schea.json. Add more strict schema definition disallowing additionalProperties. Add extensive unittests for invalid schemas.

blackboxsw force-pushed the schema-a-d branch from 93443c9 to 0e0f207 Compare January 25, 2022 03:25

blackboxsw added 2 commits January 24, 2022 20:58

schema: import James cc_debug schema to cloud-init-schema

ac02327

migrate to statis cloud-init-schema.json and add unittests

schema: migrate James cc_disable_ec2_metadata schema to static schema

7da4f29

Migrate to static cloud-init-schema.json and add unittests

blackboxsw mentioned this pull request Jan 25, 2022

Ensure schemas for all modules starting A-D (SC-646) #1155

Closed

3 tasks

blackboxsw added 2 commits January 25, 2022 16:50

schema: forgot to drop cc_chef.schema

fb219cd

schema: migrate James cc_disk_setup schema to static schema

c1f38f8

Migrate to static cloud-init-schema.json and add unittests Needs more unittests for failure cases

blackboxsw force-pushed the schema-a-d branch from 552e48b to c1f38f8 Compare January 26, 2022 04:17

holmanb requested changes Jan 27, 2022

View reviewed changes

Comment thread cloudinit/config/cc_apt_configure.py

Comment thread cloudinit/config/cloud-init-schema.json Outdated

TheRealFalcon approved these changes Jan 27, 2022

View reviewed changes

Comment thread cloudinit/config/cc_bootcmd.py

blackboxsw commented Jan 31, 2022

View reviewed changes

schema: disk_setup.layout cannot accept "auto" value

0db3716

TheRealFalcon suggested changes Jan 31, 2022

View reviewed changes

holmanb requested changes Jan 31, 2022

View reviewed changes

schema: spellintian fixes and drop specific 23.1 from deprecation war…

7207765

…ning

blackboxsw force-pushed the schema-a-d branch from 0d9671b to 7207765 Compare January 31, 2022 22:15

schema: warnings for apt_configure and ca_certs deprecated keys

69cdcb9

TheRealFalcon approved these changes Feb 1, 2022

View reviewed changes

blackboxsw merged commit af7eb1d into canonical:main Feb 1, 2022

ubuntu-server-builder mentioned this pull request May 12, 2023

Publish jsonschema with documention #3850

Closed

ubuntu-server-builder mentioned this pull request May 12, 2023

Release 22.1 #3948

Closed

Conversation

blackboxsw commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Commit Message

Additional Context

Test Steps

Checklist:

Uh oh!

holmanb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheRealFalcon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blackboxsw Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holmanb commented Jan 31, 2022

Uh oh!

holmanb left a comment

Choose a reason for hiding this comment

Uh oh!

blackboxsw commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

blackboxsw commented Jan 24, 2022 •

edited

Loading

TheRealFalcon left a comment •

edited

Loading

blackboxsw Jan 31, 2022 •

edited

Loading

blackboxsw commented Jan 31, 2022 •

edited

Loading