-
Notifications
You must be signed in to change notification settings - Fork 883
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
feature(schema): add networkv2 schema #4892
Conversation
66f8234
to
a1eddc5
Compare
Need to fix linting and unit tests still |
a1eddc5
to
3d74512
Compare
2d2a1eb
to
39d5246
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I'm on triage this week, I probably won't get fully reviewing this until next week or later, so I'm going to unassign myself if somebody else would like to pick it up in the mean time.
I'll leave some comments from when I started the review, but my review isn't complete.
Also, we'll need some way of wiring this schema up to the cloud-init schema
command so that somebody can pass a v1 or v2 schema and have it verified. E.g., right now you can do something like
cloud-init schema --config-file my-schema.yaml -t network-config
but we'll probably need to expand -t
to include v1 and v2 (or auto-detect it) and use the correct schema accordingly.
cloudinit/config/schema.py
Outdated
"latest": NETWORK_CONFIG_V1_SCHEMA_FILE, | ||
"latest": NETWORK_CONFIG_V2_SCHEMA_FILE, | ||
"1": NETWORK_CONFIG_V1_SCHEMA_FILE, | ||
"2": NETWORK_CONFIG_V2_SCHEMA_FILE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way we've named things is confusing, but this isn't really what we want here. The versioning here is for versions of the schemas themselves (which as of now and possibly forever, they aren't explicitly versioned), not the network configuration version.
Part of me is tempted to remove this section entirely, but if we keep it we should have a NETWORK_CONFIG_V1
and NETWORK_CONFIG_V2
under the SchemaType
enum with separate latest
versions here.
31d6182
to
45abafa
Compare
Thanks to James' comments I have updated how we handle the different schema versions. I elected to leave |
6d5ce4b
to
5e6d213
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @catmsred, for working on this!
validate_cloudconfig_schema
is called from:
cloudinit.stages
to validate the network config at cloud-init runtime before applying it and- From
validate_cloudconfig_file
, which is used in thecloud-init schema
cli command handler.
As it is right now, validate_cloudconfig_schema
unconditionally uses netplan_validate_network_schema
to validate v2 netconfigs and netplan_validate_network_schema
uses Netplan machinery to validate if present, but returns False when Netplan APIs are not installed. Thus, the schema added in this PR is not used in both cases, but it could be used on such systems, and that was the intention of this effort, or am I missing something?
In addition to that, ideally, I think it might be worth adding integration tests covering both cases.
schema_type = SchemaType.NETWORK_CONFIG_V2 | ||
elif network_version == 1: | ||
schema_type = SchemaType.NETWORK_CONFIG_V1 | ||
schema = get_schema(schema_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, schema
gets overwritten if given by the user as argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does since in the places it's called within this code, schema is incorrectly set to V2 so it's important to reset it after the version has been dynamically determined from the actual config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks. Then the call-sites that pass schema_type=SchemaType.NETWORK_CONFIG
and a given schema
, are going to read some schema two times. I think we could probably optimize that and not pass a schema in that case.
No, that is not the intention and thanks for catching that. I'll refactor to make sure that netplan_validate_network_schema is only used when netplan is present.
Will do! |
I went back to work on this and realized I think I'm not fully understanding your concerns. On a system with netplan, netplan_validate_network_schema will validate the schema and validate_cloudconfig_schema will return True as it should since netplan will be the program implementing the config on such a system. On a system without netplan but with networkv2, netplan_validate_network_schema will return False (since there is no netplan) and validate_cloudconfig_schema will continue running to validate the schema against the networkv2 schema included in this PR, which also seems to be correct behavior to me. Note that when validate_cloudconfig_schema calls netplan_validate_network_schema it doesn't break on return on a False from netplan_validate_network_schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is right now, validate_cloudconfig_schema unconditionally uses netplan_validate_network_schema to validate v2 netconfigs and netplan_validate_network_schema uses Netplan machinery to validate if present, but returns False when Netplan APIs are not installed. Thus, the schema added in this PR is not used in both cases, but it could be used on such systems, and that was the intention of this effort, or am I missing something?
No, that is not the intention and thanks for catching that. I'll refactor to make sure that netplan_validate_network_schema is only used when netplan is present.
I went back to work on this and realized I think I'm not fully understanding your concerns. On a system with netplan, netplan_validate_network_schema will validate the schema and validate_cloudconfig_schema will return True as it should since netplan will be the program implementing the config on such a system. On a system without netplan but with networkv2, netplan_validate_network_schema will return False (since there is no netplan) and validate_cloudconfig_schema will continue running to validate the schema against the networkv2 schema included in this PR, which also seems to be correct behavior to me. Note that when validate_cloudconfig_schema calls netplan_validate_network_schema it doesn't break on return on a False from netplan_validate_network_schema.
Yeah, that's right, I read too fast that part of the code. Thanks for pointing that out.
I left some in-line comments, but this LGTM, thanks!
schema_type = SchemaType.NETWORK_CONFIG_V2 | ||
elif network_version == 1: | ||
schema_type = SchemaType.NETWORK_CONFIG_V1 | ||
schema = get_schema(schema_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks. Then the call-sites that pass schema_type=SchemaType.NETWORK_CONFIG
and a given schema
, are going to read some schema two times. I think we could probably optimize that and not pass a schema in that case.
@@ -699,7 +708,8 @@ def validate_cloudconfig_schema( | |||
for the cloud config module (config.cc_*). If None, validate against | |||
global schema. | |||
@param schema_type: Optional SchemaType. | |||
One of: SchemaType.CLOUD_CONFIG or SchemaType.NETWORK_CONFIG. | |||
One of: SchemaType.CLOUD_CONFIG or SchemaType.NETWORK_CONFIG_V1 or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also accepts SchemaType.NETWORK_CONFIG
, do you think is worth simplifying this?
@param schema_type: Instance of SchemaType.
No problem; there's no rush on this one. I just wanted to make sure nobody was blocked or waiting for somebody else. |
2bf3e1a
to
eccc6d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @catmsred, for adding integration tests. I have done a partial review and left some comments / requests.
The test with valid config does not pass, thus something is off either with the test or the implementation.
Adds networkv2 schema definition file, removes skipping of schema validation for networkv2, and adds unit tests for same.
Co-authored-by: Alberto Contreras <[email protected]>
This reverts commit eccc6d2.
0882d8c
to
fa0780f
Compare
Since there are no automated tests in a non-netplan environment, I did some manual testing to validate the new schema on a non-netplan distro. I used alpine, as follows:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Adds networkv2 schema definition file, removes skipping of schema validation for networkv2, and adds unit tests for same.
Proposed Commit Message
Additional Context
Having a defined schema will help track drift from netplan schema.
Checklist
Merge type