ACM-26271: Only vivify ClusterMetadata.Platform for fake clusters

[2uasimojo]

For purposes of this work, there are three kinds of clusters:

• IPI, where hive manages the provisioning via installer.
• UPI, where a ClusterInstall implementation provisions the cluster and hive just watches and copies over the status.
• Fake, used for testing purposes.

When we implemented HIVE-2302 / #2729, we didn't account for UPI/ClusterInstall, which we don't really have a good way to test, and ended up injecting a bug:

Via that effort, we started populating a new Secret containing metadata.json produced by the installer. For legacy clusters (those that existed before upgrading to a version with this feature) we need to retrofit that Secret based on the ClusterMetadata (among other things), which was previously how we saved off the metadata.json. For IPI, that ClusterMetadata always had a Platform section. The changes we had to make for fake clusters saw us spoofing a very sparse metadata.json and then populating it later. That process relied on the existence of the CD.Spec.ClusterMetadata.Platform section, so we were creating it for the sake of that fake cluster path. However, it turns out that ClusterInstall subclasses don't (and don't need to) populate the ClusterMetadata.Platform section. Since we blindly copy the ClusterMetadata into the ClusterDeployment, we could end up with that Platform section being absent when we come to retrofit the metadata.json. We would then hit the path designed for fake clusters where we would create that section (empty). No problem, right? Except that we have a validating admission webhook that forbids making changes to the ClusterMetadata section, and that new, empty Platform field was flagged as such a change, and bounced by the webhook.

Phew.

So with this change, we condition populating that Platform section explicitly on fake clusters only.

[openshift-ci-robot]

@2uasimojo: This pull request references ACM-26271 which is a valid jira issue.

In response to this:

For purposes of this work, there are three kinds of clusters:

• IPI, where hive manages the provisioning via installer.
• UPI, where a ClusterInstall implementation provisions the cluster and hive just watches and copies over the status.
• Fake, used for testing purposes.

When we implemented HIVE-2302 / #2729, we didn't account for UPI/ClusterInstall, which we don't really have a good way to test, and ended up injecting a bug:

Via that effort, we started populating a new Secret containing metadata.json produced by the installer. For legacy clusters (those that existed before upgrading to a version with this feature) we need to retrofit that Secret based on the ClusterMetadata (among other things), which was previously how we saved off the metadata.json. For IPI, that ClusterMetadata always had a Platform section. The changes we had to make for fake clusters saw us spoofing a very sparse metadata.json and then populating it later. That process relied on the existence of the CD.Spec.ClusterMetadata.Platform section, so we were creating it for the sake of that fake cluster path. However, it turns out that ClusterInstall subclasses don't (and don't need to) populate the ClusterMetadata.Platform section. Since we blindly copy the ClusterMetadata into the ClusterDeployment, we could end up with that Platform section being absent when we come to retrofit the metadata.json. We would then hit the path designed for fake clusters where we would create that section (empty). No problem, right? Except that we have a validating admission webhook that forbids making changes to the ClusterMetadata section, and that new, empty Platform field was flagged as such a change, and bounced by the webhook.

Phew.

So with this change, we condition populating that Platform section explicitly on fake clusters only.

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@2uasimojo: This pull request references ACM-26271 which is a valid jira issue.

In response to this:

For purposes of this work, there are three kinds of clusters:

• IPI, where hive manages the provisioning via installer.
• UPI, where a ClusterInstall implementation provisions the cluster and hive just watches and copies over the status.
• Fake, used for testing purposes.

When we implemented HIVE-2302 / #2729, we didn't account for UPI/ClusterInstall, which we don't really have a good way to test, and ended up injecting a bug:

Via that effort, we started populating a new Secret containing metadata.json produced by the installer. For legacy clusters (those that existed before upgrading to a version with this feature) we need to retrofit that Secret based on the ClusterMetadata (among other things), which was previously how we saved off the metadata.json. For IPI, that ClusterMetadata always had a Platform section. The changes we had to make for fake clusters saw us spoofing a very sparse metadata.json and then populating it later. That process relied on the existence of the CD.Spec.ClusterMetadata.Platform section, so we were creating it for the sake of that fake cluster path. However, it turns out that ClusterInstall subclasses don't (and don't need to) populate the ClusterMetadata.Platform section. Since we blindly copy the ClusterMetadata into the ClusterDeployment, we could end up with that Platform section being absent when we come to retrofit the metadata.json. We would then hit the path designed for fake clusters where we would create that section (empty). No problem, right? Except that we have a validating admission webhook that forbids making changes to the ClusterMetadata section, and that new, empty Platform field was flagged as such a change, and bounced by the webhook.

Phew.

So with this change, we condition populating that Platform section explicitly on fake clusters only.

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 openshift-eng/jira-lifecycle-plugin repository.

[2uasimojo]

/assign @dlom

[dlom]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, dlom

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:

• OWNERS [2uasimojo,dlom]

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

[openshift-ci]

@2uasimojo: all tests passed!

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-sigs/prow repository. I understand the commands that are listed here.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 50.34%. Comparing base (47d2de1) to head (074127c).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
.../clusterdeployment/clusterdeployment_controller.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2780   +/-   ##
=======================================
  Coverage   50.34%   50.34%           
=======================================
  Files         279      279           
  Lines       34167    34167           
=======================================
  Hits        17201    17201           
  Misses      15612    15612           
  Partials     1354     1354           

Files with missing lines	Coverage Δ
.../clusterdeployment/clusterdeployment_controller.go	61.99% <0.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[2uasimojo]

Konflux not reporting back successful runs. Reported here. Suggested workaround: force merge. Doing that.