From 633998e182d76a00836b1e6fc6be71ca1e5c79e0 Mon Sep 17 00:00:00 2001 From: Elior Erez Date: Thu, 4 Sep 2025 10:27:28 -0400 Subject: [PATCH] HIVE-2859: Add ManifestsSecretRef to ClusterPool for manifest management. Add support for user-provided manifests in ClusterPool to apply custom manifests to all clusters created from the pool, similar to ClusterDeployment. --- apis/hive/v1/clusterpool_types.go | 5 + apis/hive/v1/zz_generated.deepcopy.go | 5 + .../crds/hive.openshift.io_clusterpools.yaml | 16 ++ hack/app-sre/saas-template.yaml | 23 +++ pkg/awsclient/mock/client_generated.go | 58 +++++++ .../clusterpool/clusterpool_controller.go | 40 ++++- .../clusterpool_controller_test.go | 150 ++++++++++++++++++ pkg/test/clusterpool/clusterpool.go | 8 + .../clusterpool_validating_admission_hook.go | 10 ++ ...sterpool_validating_admission_hook_test.go | 21 +++ .../hive/apis/hive/v1/clusterpool_types.go | 5 + .../apis/hive/v1/zz_generated.deepcopy.go | 5 + 12 files changed, 345 insertions(+), 1 deletion(-) diff --git a/apis/hive/v1/clusterpool_types.go b/apis/hive/v1/clusterpool_types.go index eca37072a75..d5ad0eae36d 100644 --- a/apis/hive/v1/clusterpool_types.go +++ b/apis/hive/v1/clusterpool_types.go @@ -109,6 +109,11 @@ type ClusterPoolSpec struct { // ClusterDeployment generated by the ClusterPool. // +optional CustomizationRef *corev1.LocalObjectReference `json:"customizationRef,omitempty"` + + // ManifestsSecretRef is a reference to user-provided manifests to add to or replace manifests + // that are generated by the installer for all clusters created by this pool. + // +optional + ManifestsSecretRef *corev1.LocalObjectReference `json:"manifestsSecretRef,omitempty"` } type HibernationConfig struct { diff --git a/apis/hive/v1/zz_generated.deepcopy.go b/apis/hive/v1/zz_generated.deepcopy.go index 52b9ac2a80e..79f92b13a85 100644 --- a/apis/hive/v1/zz_generated.deepcopy.go +++ b/apis/hive/v1/zz_generated.deepcopy.go @@ -1596,6 +1596,11 @@ func (in *ClusterPoolSpec) DeepCopyInto(out *ClusterPoolSpec) { *out = new(corev1.LocalObjectReference) **out = **in } + if in.ManifestsSecretRef != nil { + in, out := &in.ManifestsSecretRef, &out.ManifestsSecretRef + *out = new(corev1.LocalObjectReference) + **out = **in + } return } diff --git a/config/crds/hive.openshift.io_clusterpools.yaml b/config/crds/hive.openshift.io_clusterpools.yaml index ead2c16ba42..906757c3c32 100644 --- a/config/crds/hive.openshift.io_clusterpools.yaml +++ b/config/crds/hive.openshift.io_clusterpools.yaml @@ -313,6 +313,22 @@ spec: Labels to be applied to new ClusterDeployments created for the pool. ClusterDeployments that have already been claimed will not be affected when this value is modified. type: object + manifestsSecretRef: + description: |- + ManifestsSecretRef is a reference to user-provided manifests to add to or replace manifests + that are generated by the installer for all clusters created by this pool. + properties: + name: + default: "" + description: |- + Name of the referent. + This field is effectively required, but due to backwards compatibility is + allowed to be empty. Instances of this type with an empty value here are + almost certainly wrong. + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + type: string + type: object + x-kubernetes-map-type: atomic maxConcurrent: description: |- MaxConcurrent is the maximum number of clusters that will be provisioned or deprovisioned at an time. This includes the diff --git a/hack/app-sre/saas-template.yaml b/hack/app-sre/saas-template.yaml index 9fa2f5addfe..0bd8c63c98b 100644 --- a/hack/app-sre/saas-template.yaml +++ b/hack/app-sre/saas-template.yaml @@ -3654,6 +3654,29 @@ objects: claimed will not be affected when this value is modified.' type: object + manifestsSecretRef: + description: 'ManifestsSecretRef is a reference to user-provided + manifests to add to or replace manifests + + that are generated by the installer for all clusters created by + this pool.' + properties: + name: + default: '' + description: 'Name of the referent. + + This field is effectively required, but due to backwards compatibility + is + + allowed to be empty. Instances of this type with an empty + value here are + + almost certainly wrong. + + More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names' + type: string + type: object + x-kubernetes-map-type: atomic maxConcurrent: description: 'MaxConcurrent is the maximum number of clusters that will be provisioned or deprovisioned at an time. This includes diff --git a/pkg/awsclient/mock/client_generated.go b/pkg/awsclient/mock/client_generated.go index bef2c881c52..1665a5e138a 100644 --- a/pkg/awsclient/mock/client_generated.go +++ b/pkg/awsclient/mock/client_generated.go @@ -5,6 +5,7 @@ package mock import ( + context "context" reflect "reflect" manager "github.com/aws/aws-sdk-go-v2/feature/s3/manager" @@ -783,3 +784,60 @@ func (mr *MockClientMockRecorder) WaitUntilVpcPeeringConnectionExists(arg0 inter mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WaitUntilVpcPeeringConnectionExists", reflect.TypeOf((*MockClient)(nil).WaitUntilVpcPeeringConnectionExists), arg0) } + +// MockIPaginator is a mock of IPaginator interface. +type MockIPaginator[Out any, OptFn any] struct { + ctrl *gomock.Controller + recorder *MockIPaginatorMockRecorder[Out, OptFn] +} + +// MockIPaginatorMockRecorder is the mock recorder for MockIPaginator. +type MockIPaginatorMockRecorder[Out any, OptFn any] struct { + mock *MockIPaginator[Out, OptFn] +} + +// NewMockIPaginator creates a new mock instance. +func NewMockIPaginator[Out any, OptFn any](ctrl *gomock.Controller) *MockIPaginator[Out, OptFn] { + mock := &MockIPaginator[Out, OptFn]{ctrl: ctrl} + mock.recorder = &MockIPaginatorMockRecorder[Out, OptFn]{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockIPaginator[Out, OptFn]) EXPECT() *MockIPaginatorMockRecorder[Out, OptFn] { + return m.recorder +} + +// HasMorePages mocks base method. +func (m *MockIPaginator[Out, OptFn]) HasMorePages() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasMorePages") + ret0, _ := ret[0].(bool) + return ret0 +} + +// HasMorePages indicates an expected call of HasMorePages. +func (mr *MockIPaginatorMockRecorder[Out, OptFn]) HasMorePages() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasMorePages", reflect.TypeOf((*MockIPaginator[Out, OptFn])(nil).HasMorePages)) +} + +// NextPage mocks base method. +func (m *MockIPaginator[Out, OptFn]) NextPage(arg0 context.Context, arg1 ...OptFn) (*Out, error) { + m.ctrl.T.Helper() + varargs := []interface{}{arg0} + for _, a := range arg1 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "NextPage", varargs...) + ret0, _ := ret[0].(*Out) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// NextPage indicates an expected call of NextPage. +func (mr *MockIPaginatorMockRecorder[Out, OptFn]) NextPage(arg0 interface{}, arg1 ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]interface{}{arg0}, arg1...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NextPage", reflect.TypeOf((*MockIPaginator[Out, OptFn])(nil).NextPage), varargs...) +} diff --git a/pkg/controller/clusterpool/clusterpool_controller.go b/pkg/controller/clusterpool/clusterpool_controller.go index 7a848d63ebe..489b77888f0 100644 --- a/pkg/controller/clusterpool/clusterpool_controller.go +++ b/pkg/controller/clusterpool/clusterpool_controller.go @@ -695,6 +695,15 @@ func (r *ReconcileClusterPool) addClusters( errs = append(errs, fmt.Errorf("%s: %w", credentialsSecretDependent, err)) } + // Get manifests if specified + manifests, err := r.getManifests(clp, logger) + if err != nil { + errs = append(errs, fmt.Errorf("error getting manifests: %w", err)) + } + if manifests != nil { + logger.WithField("manifestCount", len(manifests)).Info("retrieved manifests for cluster pool") + } + if clp.Spec.CustomizationRef != nil && clp.Spec.CustomizationRef.Name != "" { custCDC := clp.Spec.CustomizationRef.Name if cdcs.nonInventory == nil || cdcs.nonInventory.Name != custCDC { @@ -713,7 +722,7 @@ func (r *ReconcileClusterPool) addClusters( } for i := 0; i < newClusterCount; i++ { - cd, err := r.createCluster(clp, cloudBuilder, pullSecret, installConfigTemplate, poolVersion, cdcs, logger) + cd, err := r.createCluster(clp, cloudBuilder, pullSecret, installConfigTemplate, poolVersion, cdcs, manifests, logger) if err != nil { return err } @@ -730,6 +739,7 @@ func (r *ReconcileClusterPool) createCluster( installConfigTemplate string, poolVersion string, cdcs *cdcCollection, + manifests map[string][]byte, logger log.FieldLogger, ) (*hivev1.ClusterDeployment, error) { var err error @@ -778,6 +788,12 @@ func (r *ReconcileClusterPool) createCluster( builder.HibernateAfter = &clp.Spec.HibernateAfter.Duration } + // Set manifests if provided + if manifests != nil { + logger.WithField("manifestCount", len(manifests)).Info("applying manifests to cluster deployment") + builder.InstallerManifests = manifests + } + objs, err := builder.Build() if err != nil { return nil, errors.Wrap(err, "error building resources") @@ -1259,6 +1275,28 @@ func (r *ReconcileClusterPool) getPullSecret(pool *hivev1.ClusterPool, logger lo return string(pullSecret), nil } +func (r *ReconcileClusterPool) getManifests(pool *hivev1.ClusterPool, logger log.FieldLogger) (map[string][]byte, error) { + if pool.Spec.ManifestsSecretRef == nil { + logger.Debug("no manifests secret reference specified") + return nil, nil + } + + logger.WithField("secretName", pool.Spec.ManifestsSecretRef.Name).Info("retrieving manifests from secret") + manifestsSecret := &corev1.Secret{} + err := r.Client.Get( + context.Background(), + types.NamespacedName{Namespace: pool.Namespace, Name: pool.Spec.ManifestsSecretRef.Name}, + manifestsSecret, + ) + if err != nil { + logger.WithError(err).Log(controllerutils.LogLevel(err), "error reading manifests secret") + return nil, err + } + + logger.WithField("manifestCount", len(manifestsSecret.Data)).Info("successfully retrieved manifests from secret") + return manifestsSecret.Data, nil +} + func (r *ReconcileClusterPool) createCloudBuilder(pool *hivev1.ClusterPool, logger log.FieldLogger) (clusterresource.CloudBuilder, error) { switch platform := pool.Spec.Platform; { case platform.AWS != nil: diff --git a/pkg/controller/clusterpool/clusterpool_controller_test.go b/pkg/controller/clusterpool/clusterpool_controller_test.go index a0586dd2af7..03d544b9508 100644 --- a/pkg/controller/clusterpool/clusterpool_controller_test.go +++ b/pkg/controller/clusterpool/clusterpool_controller_test.go @@ -3263,3 +3263,153 @@ func Test_isBroken(t *testing.T) { }) } } + +func TestGetManifests(t *testing.T) { + scheme := scheme.GetScheme() + logger := log.NewEntry(log.New()) + + tests := []struct { + name string + pool *hivev1.ClusterPool + existing []runtime.Object + expected map[string][]byte + expectError bool + }{ + { + name: "no manifests specified", + pool: testcp.FullBuilder(testNamespace, "test-pool", scheme). + Options( + testcp.ForAWS(credsSecretName, "us-east-1"), + testcp.WithBaseDomain("test-domain"), + testcp.WithImageSet(imageSetName), + ).Build(), + existing: []runtime.Object{}, + expected: nil, + expectError: false, + }, + { + name: "manifests secret exists", + pool: testcp.FullBuilder(testNamespace, "test-pool", scheme). + Options( + testcp.ForAWS(credsSecretName, "us-east-1"), + testcp.WithBaseDomain("test-domain"), + testcp.WithImageSet(imageSetName), + testcp.WithManifestsSecretRef("test-manifests-secret"), + ).Build(), + existing: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-manifests-secret", + Namespace: testNamespace, + }, + Data: map[string][]byte{ + "manifest1.yaml": []byte("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test"), + "manifest2.yaml": []byte("apiVersion: v1\nkind: Secret\nmetadata:\n name: test"), + }, + }, + }, + expected: map[string][]byte{ + "manifest1.yaml": []byte("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test"), + "manifest2.yaml": []byte("apiVersion: v1\nkind: Secret\nmetadata:\n name: test"), + }, + expectError: false, + }, + { + name: "manifests secret not found", + pool: testcp.FullBuilder(testNamespace, "test-pool", scheme). + Options( + testcp.ForAWS(credsSecretName, "us-east-1"), + testcp.WithBaseDomain("test-domain"), + testcp.WithImageSet(imageSetName), + testcp.WithManifestsSecretRef("missing-secret"), + ).Build(), + existing: []runtime.Object{}, + expected: nil, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := testfake.NewFakeClientBuilder().WithRuntimeObjects(tt.existing...).Build() + r := &ReconcileClusterPool{ + Client: c, + } + + result, err := r.getManifests(tt.pool, logger) + + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, result) + } + }) + } +} + +func TestClusterPoolWithManifests(t *testing.T) { + scheme := scheme.GetScheme() + logger := log.NewEntry(log.New()) + + poolBuilder := testcp.FullBuilder(testNamespace, testLeasePoolName, scheme). + GenericOptions( + testgeneric.WithFinalizer(finalizer), + ). + Options( + testcp.ForAWS(credsSecretName, "us-east-1"), + testcp.WithBaseDomain("test-domain"), + testcp.WithImageSet(imageSetName), + ) + + tests := []struct { + name string + pool *hivev1.ClusterPool + existing []runtime.Object + expectedTotalClusters int + expectedManifests map[string][]byte + expectError bool + }{ + { + name: "clusterpool with manifests secret", + pool: poolBuilder.Build( + testcp.WithSize(1), + testcp.WithManifestsSecretRef("test-manifests-secret"), + ), + existing: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-manifests-secret", + Namespace: testNamespace, + }, + Data: map[string][]byte{ + "manifest1.yaml": []byte("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test"), + }, + }, + }, + expectedManifests: map[string][]byte{ + "manifest1.yaml": []byte("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test"), + }, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := testfake.NewFakeClientBuilder().WithRuntimeObjects(tt.existing...).Build() + r := &ReconcileClusterPool{ + Client: c, + } + + // Test the getManifests function directly + result, err := r.getManifests(tt.pool, logger) + + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expectedManifests, result) + } + }) + } +} diff --git a/pkg/test/clusterpool/clusterpool.go b/pkg/test/clusterpool/clusterpool.go index 47f3f2270dc..65c3366a9b1 100644 --- a/pkg/test/clusterpool/clusterpool.go +++ b/pkg/test/clusterpool/clusterpool.go @@ -222,3 +222,11 @@ func WithCustomizationRef(cdcName string) Option { } } } + +func WithManifestsSecretRef(secretName string) Option { + return func(clusterPool *hivev1.ClusterPool) { + clusterPool.Spec.ManifestsSecretRef = &corev1.LocalObjectReference{ + Name: secretName, + } + } +} diff --git a/pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.go b/pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.go index 94a96c97377..e41c747bd17 100644 --- a/pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.go +++ b/pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook.go @@ -160,6 +160,11 @@ func (a *ClusterPoolValidatingAdmissionHook) validateCreate(admissionSpec *admis // Add the new data to the contextLogger contextLogger.Data["object.Name"] = newObject.Name + // Log if manifests are being validated + if newObject.Spec.ManifestsSecretRef != nil { + contextLogger.WithField("manifestsSecretRef", newObject.Spec.ManifestsSecretRef.Name).Info("validating cluster pool with manifests secret reference") + } + // TODO: Put Create Validation Here (or in openAPIV3Schema validation section of crd) if len(newObject.Name) > validation.DNS1123LabelMaxLength { @@ -234,6 +239,11 @@ func (a *ClusterPoolValidatingAdmissionHook) validateUpdate(admissionSpec *admis // Add the new data to the contextLogger contextLogger.Data["oldObject.Name"] = oldObject.Name + // Log if manifests are being validated + if newObject.Spec.ManifestsSecretRef != nil { + contextLogger.WithField("manifestsSecretRef", newObject.Spec.ManifestsSecretRef.Name).Info("validating cluster pool update with manifests secret reference") + } + allErrs := field.ErrorList{} specPath := field.NewPath("spec") diff --git a/pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook_test.go b/pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook_test.go index dba178aa950..eefcaf6796d 100644 --- a/pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook_test.go +++ b/pkg/validating-webhooks/hive/v1/clusterpool_validating_admission_hook_test.go @@ -222,6 +222,27 @@ func TestClusterPoolValidate(t *testing.T) { operation: admissionv1beta1.Delete, expectedAllowed: true, }, + { + name: "valid create with manifests secret", + newObject: func() *hivev1.ClusterPool { + cp := validAWSClusterPool() + cp.Spec.ManifestsSecretRef = &corev1.LocalObjectReference{Name: "test-manifests-secret"} + return cp + }(), + operation: admissionv1beta1.Create, + expectedAllowed: true, + }, + { + name: "valid update with manifests secret", + oldObject: validAWSClusterPool(), + newObject: func() *hivev1.ClusterPool { + cp := validAWSClusterPool() + cp.Spec.ManifestsSecretRef = &corev1.LocalObjectReference{Name: "test-manifests-secret"} + return cp + }(), + operation: admissionv1beta1.Update, + expectedAllowed: true, + }, } for _, tc := range cases { diff --git a/vendor/github.com/openshift/hive/apis/hive/v1/clusterpool_types.go b/vendor/github.com/openshift/hive/apis/hive/v1/clusterpool_types.go index eca37072a75..d5ad0eae36d 100644 --- a/vendor/github.com/openshift/hive/apis/hive/v1/clusterpool_types.go +++ b/vendor/github.com/openshift/hive/apis/hive/v1/clusterpool_types.go @@ -109,6 +109,11 @@ type ClusterPoolSpec struct { // ClusterDeployment generated by the ClusterPool. // +optional CustomizationRef *corev1.LocalObjectReference `json:"customizationRef,omitempty"` + + // ManifestsSecretRef is a reference to user-provided manifests to add to or replace manifests + // that are generated by the installer for all clusters created by this pool. + // +optional + ManifestsSecretRef *corev1.LocalObjectReference `json:"manifestsSecretRef,omitempty"` } type HibernationConfig struct { diff --git a/vendor/github.com/openshift/hive/apis/hive/v1/zz_generated.deepcopy.go b/vendor/github.com/openshift/hive/apis/hive/v1/zz_generated.deepcopy.go index 52b9ac2a80e..79f92b13a85 100644 --- a/vendor/github.com/openshift/hive/apis/hive/v1/zz_generated.deepcopy.go +++ b/vendor/github.com/openshift/hive/apis/hive/v1/zz_generated.deepcopy.go @@ -1596,6 +1596,11 @@ func (in *ClusterPoolSpec) DeepCopyInto(out *ClusterPoolSpec) { *out = new(corev1.LocalObjectReference) **out = **in } + if in.ManifestsSecretRef != nil { + in, out := &in.ManifestsSecretRef, &out.ManifestsSecretRef + *out = new(corev1.LocalObjectReference) + **out = **in + } return }