[WIP] HIVE-2859: Add ManifestsSecretRef to ClusterPool for manifest management

[eliorerz]

Add support for user-provided manifests in ClusterPool to apply custom manifests to all clusters created from the pool, similar to ClusterDeployment.
This is done as part of the Nutanix provider support for supporting ClusterPool resource .

This PR introduces support for user-provided manifests in ClusterPool. This allows a common set of custom manifests to be applied to all clusters claimed from the pool, mirroring the existing functionality in ClusterDeployment.

Note: This is a prerequisite for adding ClusterPool support for the Nutanix provider.

[openshift-ci-robot]

@eliorerz: This pull request references HIVE-2859 which is a valid jira issue.

In response to this:

Add support for user-provided manifests in ClusterPool to apply custom manifests to all clusters created from the pool, similar to ClusterDeployment.
This is done as part of the Nutanix provider support for supporting ClusterPool resource .

This PR introduces support for user-provided manifests in ClusterPool. This allows a common set of custom manifests to be applied to all clusters claimed from the pool, mirroring the existing functionality in ClusterDeployment.

Note: This is a prerequisite for adding ClusterPool support for the Nutanix provider.

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.

[eliorerz]

/hold

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: eliorerz
Once this PR has been reviewed and has the lgtm label, please assign dlom for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[codecov]

Codecov Report

❌ Patch coverage is 43.33333% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.15%. Comparing base (301f422) to head (633998e).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/awsclient/mock/client_generated.go	0.00%	27 Missing ⚠️
...g/controller/clusterpool/clusterpool_controller.go	72.00%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2745      +/-   ##
==========================================
- Coverage   50.17%   50.15%   -0.03%     
==========================================
  Files         288      288              
  Lines       34071    34124      +53     
==========================================
+ Hits        17096    17115      +19     
- Misses      15626    15657      +31     
- Partials     1349     1352       +3     

Files with missing lines	Coverage Δ
pkg/test/clusterpool/clusterpool.go	95.41% <100.00%> (+0.17%)	⬆️
...s/hive/v1/clusterpool_validating_admission_hook.go	81.37% <100.00%> (+0.52%)	⬆️
...m/openshift/hive/apis/hive/v1/clusterpool_types.go	0.00% <ø> (ø)
...g/controller/clusterpool/clusterpool_controller.go	58.41% <72.00%> (+0.36%)	⬆️
pkg/awsclient/mock/client_generated.go	68.76% <0.00%> (-4.13%)	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[2uasimojo]

At a glance, this looks sane as far as it goes, and I think it's something we want to do.

However...

It assumes that the manifests will want to be the same for all clusters in the pool. This isn't necessarily a bad thing, but it seems pretty likely that at some point we'll want them to be customizable per cluster. As such, they would want to be added to ClusterDeploymentCustomization so they can be part of the Inventory.

But perhaps I'm getting ahead of myself, and we're not going to need that any time soon...

WDYT?

[eliorerz]

@2uasimojo I had similar thoughts about this.
I think it largely depends on the long-term direction we want Hive to take.

The main reasons I chose the current approach are:

1. In our case, setting this configuration per cluster adds unnecessary complexity, and we don’t yet know whether we’ll ever need it per cluster.

2. Even if, in the future, users do want to set it per cluster, we can always extend the functionality to support that. From my perspective, both approaches could coexist without impacting the general behavior.

[2uasimojo]

Right, so on that note, we recently added the ability to patch installer manifests.

And we did it by adding a field to the CDC.

And we made it so a single CDC can be applied to all pool clusters as well as per cluster via Inventory.

So fitting into that architecture, it seems like it would make the most sense to make the new field part of CDC, and then you would inherently get both the pool-wide and per-cluster behaviors at the same time.

Mechanically for the consumer, it's a little more involved, because you have to create an extra CR -- the CDC -- but you only have to create one of them, not a whole inventory.

[openshift-ci]

@eliorerz: 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.