Bug 2098299
| Summary: | install-config: Strict unmarshalling conflicts with new fields | ||
|---|---|---|---|
| Product: | OpenShift Container Platform | Reporter: | Eric Fried <efried> |
| Component: | Installer | Assignee: | Aditya Narayanaswamy <anarayan> |
| Installer sub component: | openshift-installer | QA Contact: | Yunfei Jiang <yunjiang> |
| Status: | CLOSED ERRATA | Docs Contact: | |
| Severity: | low | ||
| Priority: | low | CC: | abutcher, anarayan, bscott, efried, jshu, mihuang, padillon |
| Version: | 4.11 | ||
| Target Milestone: | --- | ||
| Target Release: | 4.12.0 | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: |
* Previously, cluster installations using Hive could fail if Hive used an older version of the install-config.yaml file. This update allows the installer to accept older versions of the install-config.yaml file provided by Hive. (link:https://bugzilla.redhat.com/show_bug.cgi?id=2098299[*BZ#2098299*])
|
Story Points: | --- |
| Clone Of: | Environment: | ||
| Last Closed: | 2023-01-17 19:50:08 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | |||
| Bug Blocks: | 2105331 | ||
|
Description
Eric Fried
2022-06-17 22:41:51 UTC
> add `omitempty` to this field I've submitted https://github.com/openshift/installer/pull/6028 to do that. (It's not linking to this bug, which probably means I spelled things wrong -- I don't work with BZ much.) We're discussing this in slack [1]. In summary: - Adding `omitempty` is not sufficient to address the problem -- see [2]. The new fields must *also* be converted to pointers. I have updated https://github.com/openshift/installer/pull/6028 to do that for the EC2Metadata field. However... - There are other fields for which this problem manifests. We don't know all of them. But at least Azure OSImage [3] is affected. (Note that this one is already `omitempty`, but is still a problem because it's not a pointer.) I've added a commit for this field to the PR. - It is under consideration to simply revert the switch to UnmarshalStrict [4]. This would be the simplest solution, and guaranteed (?) to cover all cases, since we wouldn't need to root out individual fields. - Alternatively, it would be lovely to have unit tests that detect this class of problem so we can a) identify existing instances, and b) fail CI, preventing future violations. The challenge here is that such a test needs to operate across releases/commits, at least to address the former. For the latter, it may be possible to simply look for new fields and enforce that they're pointers with `omitempty`. Though that gets complicated for sub-structures, whose contents don't necessarily want/need to conform to that rule. [1] https://coreos.slack.com/archives/C68TNFWA2/p1655842108884639 [2] https://github.com/golang/go/issues/11939 [3] https://github.com/openshift/installer/blob/599c160a4e1c48ded0980955bc475ece0268083f/pkg/types/azure/machinepool.go#L45 [4] https://github.com/openshift/installer/pull/5307 The target release of this BZ is 4.11, but linked PR https://github.com/openshift/installer/pull/6039 is on master branch (4.12). Updated target release to 4.12 and moved this back to ON_QA. Tested against serval unknown fields in install-config.yaml, only one of them was printed in warning message. I expect all unknown fields printed in warning message. OCP Version 4.12.0-0.nightly-2022-07-27-133042 > tested with following config: apiVersion: v1 baseDomain: qe.devcluster.openshift.com compute: - architecture: amd64 hyperthreading: Enabled name: worker platform: aws: type1: aaa rootVolume: aaaa: 123 replicas: 3 controlPlane: architecture: amd64 hyperthreading: Enabled name: master platform: aws: type1: aaa rootVolume1: aaaa: 123 replicas: 3 metadata: creationTimestamp: null name: yunjiang-test1 networking: clusterNetwork: - cidr: 10.128.0.0/14 hostPrefix: 23 machineNetwork: - cidr: 10.0.0.0/16 networkType: OpenShiftSDN a: b: 123 serviceNetwork: - 172.30.0.0/16 platform: aws: region: us-east-2 publish: External > output: WARNING failed to parse first occurence of unknown field: failed to unmarshal install-config.yaml: error unmarshaling JSON: while decoding JSON: json: unknown field "aaaa" INFO Attempting to unmarshal while ignoring unknown keys because strict unmarshaling failed INFO Credentials loaded from the "de" profile in file "/home/cloud-user/.aws/credentials" INFO Consuming Install Config from target directory INFO Manifests created in: bb/manifests and bb/openshift This is a known disadvantage to the golang library where only the first unknown field is printed. There seems to be no solution for this at the moment and we agreed to do it this way knowing the pitfall. Moving this back to ON_QA due to https://bugzilla.redhat.com/show_bug.cgi?id=2098299#c10 Yunfei, thanks for checking this so thoroughly. Based on Aditya's comment, I think this is a limitation we should accept and is ultimately insignificant. Let us know if we need to do anything else here. The limitation is acceptable, moving to VERIFIED. Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory (Moderate: OpenShift Container Platform 4.12.0 bug fix and security update), and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://access.redhat.com/errata/RHSA-2022:7399 |