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

validation: Enforce strict unmarshalling of config #5307

Conversation

rna-afk
Copy link
Contributor

@rna-afk rna-afk commented Oct 18, 2021

In order to stop the user from adding unrecognized properties
in the install config, a validation check is added to force the
user to remove them. This is done by enforcing a strict
unmarshalling of the install config yaml file that will raise an
error when an unknown property is added.

This will force the installer to return an error for the first
occurence of an unknown property since the current library
returns only the first unknown field.

@rna-afk rna-afk changed the title validation: Enforce strict unmarshalling of config [WIP] validation: Enforce strict unmarshalling of config Oct 18, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2021
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

  1. Can you add a unit test case covering an unknown field?
  2. Can you show me the full error message from the installer when an unknown field is used?

pkg/asset/installconfig/installconfig.go Show resolved Hide resolved
@rna-afk
Copy link
Contributor Author

rna-afk commented Oct 19, 2021

Fixed the ordering and this is the output

FATAL failed to fetch Master Machines: failed to load asset "Install Config":
failed to parse first occurence of unknown field: failed to unmarshal install-config.yaml: 
error unmarshaling JSON: while decoding JSON: json: unknown field "wrong_key" 

I am facing some problems with handling deprecated install configs. There is a unit test failing that has an old valid install config and not sure how to handle deprecated fields that are not in the schema.

@staebler
Copy link
Contributor

I am facing some problems with handling deprecated install configs. There is a unit test failing that has an old valid install config and not sure how to handle deprecated fields that are not in the schema.

What is the deprecated field? There should not be any fields that are valid and not in the InstallConfig struct.

@rna-afk
Copy link
Contributor Author

rna-afk commented Oct 19, 2021

It's a test here. The field "network" is causing problems. It's changed to "networking" in the new schema.

name: "old valid InstallConfig",

@staebler
Copy link
Contributor

It's a test here. The field "network" is causing problems. It's changed to "networking" in the new schema.

name: "old valid InstallConfig",

Ha! That is actually a bad test case. The networking field was never named network. The network field in that test case is being ignored.

@staebler
Copy link
Contributor

It's a test here. The field "network" is causing problems. It's changed to "networking" in the new schema.

name: "old valid InstallConfig",

Ha! That is actually a bad test case. The networking field was never named network. The network field in that test case is being ignored.

See here where the field name in v1beta3 was networking.

@rna-afk
Copy link
Contributor Author

rna-afk commented Oct 19, 2021

Nice! I'll fix it then.

@rna-afk rna-afk force-pushed the reject_unrecognized_properties_install_config branch from 837686c to b2b76b8 Compare October 19, 2021 21:39
@@ -189,7 +189,21 @@ platform:
region: us-east-1
pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}"
network:
type: OpenShiftSDN
type: OpenshiftSDN
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change this to an invalid network type?

@@ -189,7 +189,21 @@ platform:
region: us-east-1
pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}"
network:
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a field name that is clearly unknown rather than a field name that is similar to a known field. It is hard reading the test case input to spot immediately which field is wrong.

@rna-afk rna-afk force-pushed the reject_unrecognized_properties_install_config branch from b2b76b8 to 1a2f368 Compare October 20, 2021 12:41
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

Looks good. Just one minor nit.

{
name: "unknown field",
data: `
apiVersion: v1beta3
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
apiVersion: v1beta3
apiVersion: v1

In order to stop the user from adding unrecognized properties
in the install config, a validation check is added to force the
user to remove them. This is done by enforcing a strict
unmarshalling of the install config yaml file that will raise an
error when an unknown property is added.

This will force the installer to return an error for the first
occurence of an unknown property since the current library
returns only the first unknown field.
@rna-afk rna-afk force-pushed the reject_unrecognized_properties_install_config branch from 1a2f368 to 785921b Compare October 20, 2021 14:24
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2021
@rna-afk rna-afk changed the title [WIP] validation: Enforce strict unmarshalling of config validation: Enforce strict unmarshalling of config Nov 1, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2021
@rna-afk
Copy link
Contributor Author

rna-afk commented Nov 1, 2021

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 5, 2021

@rna-afk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-workers-rhel8 785921b link false /test e2e-aws-workers-rhel8
ci/prow/e2e-aws-single-node 785921b link false /test e2e-aws-single-node
ci/prow/e2e-aws-workers-rhel7 785921b link false /test e2e-aws-workers-rhel7
ci/prow/e2e-crc 785921b link false /test e2e-crc
ci/prow/e2e-metal-ipi-ovn-ipv6 785921b link false /test e2e-metal-ipi-ovn-ipv6

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 5943605 into openshift:master Nov 6, 2021
kwoodson pushed a commit to kwoodson/installer that referenced this pull request Nov 12, 2021
The openstack manifests tests use an install config with the
clusterID field that is no longer supported by the installer.
Changes to the installer to enforce strict unmarshalling of the
install config is in place but is being hindered by the openstack
manifests tests in this PR.

openshift#5307
rna-afk added a commit to rna-afk/release that referenced this pull request Dec 14, 2021
The validation for the install config was tightened in this PR
openshift/installer#5307 and it's causing
the e2e-azurestack tests to fail so fixing the install config to
conform to the validation.
AnnaZivkovic pushed a commit to AnnaZivkovic/installer that referenced this pull request Apr 1, 2022
The openstack manifests tests use an install config with the
clusterID field that is no longer supported by the installer.
Changes to the installer to enforce strict unmarshalling of the
install config is in place but is being hindered by the openstack
manifests tests in this PR.

openshift#5307
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants