autoinstall: reject null values in autoinstall top level sections

[ogayot]

opening this PR to split the original #1375

When working on the above PR, I realized that the top level autoinstall sections accepted the null value although such a configuration should have been rejected according to the JSON schema:

source: null      # invalid - source should be of type object
keyboard: null    # invalid - keyboard should be of type object
snaps: null       # invalid - snaps should be of type array
...

This happened because the JSON validation was skipped when a section was set to null. The goal was to allow the controllers to set autoinstall_default to None (which is a sensible thing to do).

However, instead of skipping validation when the section resolves to None, the better thing to do is to skip the validation when autoinstall_default is effectively used.

If we want to accept null for a given section, we can by adding "null" to the list of accepted types:

- "type": "object"
+ "type": ["object", "null"]

the proxy section is an example.

[ogayot]

Original comment from @dbungert:

I think we should make the needed schema changes to allow foo: null for all toplevel keys except version. There is a small risk that someone has used that to explicitly say "give me the defaults for this section" rather than relying on obtaining that by omission, and proxy serves as a sort of precedent for that.

If there are other cases like version where that doesn't make sense, let's discuss.

[ogayot]

Thank you, both for addressing the immediate need for source selection and the general schema validation improvements. All of this is great.

I think we should make the needed schema changes to allow foo: null for all toplevel keys except version. There is a small risk that someone has used that to explicitly say "give me the defaults for this section" rather than relying on obtaining that by omission, and proxy serves as a sort of precedent for that.

If there are other cases like version where that doesn't make sense, let's discuss.

Yes, I see your point but I don't think this is sane to let the users think they can pass section: null and expect to get the default values because in many cases, this is not what they get:

For the following controllers, we set a custom value that gets used when said section is absent:

subiquity/server/controllers/cmdlist.py:    autoinstall_default = []
subiquity/server/controllers/debconf.py:    autoinstall_default = ""
subiquity/server/controllers/drivers.py:    autoinstall_default = {"install": False}
subiquity/server/controllers/locale.py:    autoinstall_default = 'en_US.UTF-8'
subiquity/server/controllers/package.py:    autoinstall_default = []
subiquity/server/controllers/snaplist.py:    autoinstall_default = []
subiquity/server/controllers/source.py:    autoinstall_default = {"search_drivers": True}
subiquity/server/controllers/timezone.py:    autoinstall_default = ''
subiquity/server/controllers/updates.py:    autoinstall_default = 'security'
subiquity/server/controllers/userdata.py:    autoinstall_default = {}

but if the section is set to null, we won't load the default autoinstall value, we will try to load None instead.
For instance with locale, this is what we get:

    def load_autoinstall_data(self, data):
        log.debug("data is %s", data)
        os.environ["LANG"] = data

with no locale section:

2022-08-03 16:50:55,038 DEBUG subiquity.server.controllers.locale:36 data is en_US.UTF-8

with locale: null:

2022-08-03 16:52:10,087 DEBUG subiquity.server.controllers.locale:36 data is None

(and also it fails later on):

2022-08-03 16:52:10,088 ERROR root:37 finish: subiquity/Locale/load_autoinstall_data: FAIL: str expected, not NoneType
2022-08-03 16:52:10,089 ERROR subiquity.server.server:422 top level error
Traceback (most recent call last):
  File "/home/olivier/dev/canonical/subiquity/subiquity/server/server.py", line 653, in start
    self.load_autoinstall_config(only_early=False)
  File "/home/olivier/dev/canonical/subiquity/subiquity/server/server.py", line 492, in load_autoinstall_config
    controller.setup_autoinstall()
  File "/home/olivier/dev/canonical/subiquity/subiquity/server/controller.py", line 69, in setup_autoinstall
    self.load_autoinstall_data(ai_data)
  File "/home/olivier/dev/canonical/subiquity/subiquity/server/controllers/locale.py", line 37, in load_autoinstall_data
    os.environ["LANG"] = data
  File "/usr/lib/python3.10/os.py", line 684, in __setitem__
    value = self.encodevalue(value)
  File "/usr/lib/python3.10/os.py", line 756, in encode
    raise TypeError("str expected, not %s" % type(value).__name__)
TypeError: str expected, not NoneType

I would prefer rejecting null values where they weren't supposed to be.
But if we decide otherwise and want to accept null everywhere, we need to make these controllers smarter for sure.

[mwhudson]

I agree this behaviour is a wholly unintended consequence of the way the validation works and we should fix it. Is there any way we can test this sensibly?

[ogayot]

I agree this behaviour is a wholly unintended consequence of the way the validation works and we should fix it. Is there any way we can test this sensibly?

Probably! Are you asking about an integration test that expects subiquity to reject section: null (unless specifically allowed) or a different kind of testing?

Also, do you have an opinion on breaking backward compatibility for users that have potentially (ab)used this behavior to get the default on some of the sections? This is what @dbungert is concerned about.