Skip to content

Commit 6640d97

Browse files
PanSpagetkaPanSpagetka
committed
Address review comments
1 parent 3f9bd3a commit 6640d97

File tree

5 files changed

+120
-36
lines changed

5 files changed

+120
-36
lines changed

‎controlplane/rosa/api/v1beta2/rosacontrolplane_webhook.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -185,16 +185,21 @@ func (r *ROSAControlPlane) validateExternalAuthProviders() *field.Error {
185185

186186
func (r *ROSAControlPlane) validateRosaRoleConfig() *field.Error {
187187
hasRosaRoleConfigRef := r.Spec.RosaRoleConfigRef != nil
188-
hasDirectRoleFields := r.Spec.OIDCID != "" || r.Spec.InstallerRoleARN != "" || r.Spec.SupportRoleARN != "" || r.Spec.WorkerRoleARN != "" ||
188+
hasAnyDirectRoleFields := r.Spec.OIDCID != "" || r.Spec.InstallerRoleARN != "" || r.Spec.SupportRoleARN != "" || r.Spec.WorkerRoleARN != "" ||
189189
r.Spec.RolesRef.IngressARN != "" || r.Spec.RolesRef.ImageRegistryARN != "" || r.Spec.RolesRef.StorageARN != "" ||
190190
r.Spec.RolesRef.NetworkARN != "" || r.Spec.RolesRef.KubeCloudControllerARN != "" || r.Spec.RolesRef.NodePoolManagementARN != "" ||
191191
r.Spec.RolesRef.ControlPlaneOperatorARN != "" || r.Spec.RolesRef.KMSProviderARN != ""
192192

193-
if hasRosaRoleConfigRef && hasDirectRoleFields {
193+
hasAllDirectRoleFields := r.Spec.OIDCID != "" && r.Spec.InstallerRoleARN != "" && r.Spec.SupportRoleARN != "" && r.Spec.WorkerRoleARN != "" &&
194+
r.Spec.RolesRef.IngressARN != "" && r.Spec.RolesRef.ImageRegistryARN != "" && r.Spec.RolesRef.StorageARN != "" &&
195+
r.Spec.RolesRef.NetworkARN != "" && r.Spec.RolesRef.KubeCloudControllerARN != "" && r.Spec.RolesRef.NodePoolManagementARN != "" &&
196+
r.Spec.RolesRef.ControlPlaneOperatorARN != "" && r.Spec.RolesRef.KMSProviderARN != ""
197+
198+
if hasRosaRoleConfigRef && hasAnyDirectRoleFields {
194199
return field.Invalid(field.NewPath("spec.rosaRoleConfigRef"), r.Spec.RosaRoleConfigRef, "rosaRoleConfigRef and direct role fields (oidcID, installerRoleARN, supportRoleARN, workerRoleARN, rolesRef) are mutually exclusive")
195200
}
196201

197-
if !hasRosaRoleConfigRef && !hasDirectRoleFields {
202+
if !hasRosaRoleConfigRef && !hasAllDirectRoleFields {
198203
return field.Invalid(field.NewPath("spec"), r.Spec, "either rosaRoleConfigRef or direct role fields (oidcID, installerRoleARN, supportRoleARN, workerRoleARN, rolesRef) must be specified")
199204
}
200205

‎controlplane/rosa/controllers/rosacontrolplane_controller.go‎

Lines changed: 73 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -228,10 +228,11 @@ func (r *ROSAControlPlaneReconciler) reconcileNormal(ctx context.Context, rosaSc
228228
return ctrl.Result{}, fmt.Errorf("failed to transform caller identity to creator: %w", err)
229229
}
230230

231+
rosaRoleConfig := &expinfrav1.ROSARoleConfig{}
231232
// Get role configuration from either RosaRoleConfig or direct fields
232233
if rosaScope.ControlPlane.Spec.RosaRoleConfigRef != nil {
233234
// Get configuration from RosaRoleConfig
234-
rosaRoleConfig := &expinfrav1.ROSARoleConfig{}
235+
235236
key := client.ObjectKey{
236237
Name: rosaScope.ControlPlane.Spec.RosaRoleConfigRef.Name,
237238
Namespace: rosaScope.ControlPlane.Namespace,
@@ -260,21 +261,25 @@ func (r *ROSAControlPlaneReconciler) reconcileNormal(ctx context.Context, rosaSc
260261
}
261262

262263
conditions.MarkTrue(rosaScope.ControlPlane, rosacontrolplanev1.ROSARoleConfigReadyCondition)
263-
264-
// Update spec fields from RosaRoleConfig
265-
rosaScope.ControlPlane.Spec.OIDCID = rosaRoleConfig.Status.OIDCID
266-
rosaScope.ControlPlane.Spec.InstallerRoleARN = rosaRoleConfig.Status.AccountRolesRef.InstallerRoleARN
267-
rosaScope.ControlPlane.Spec.SupportRoleARN = rosaRoleConfig.Status.AccountRolesRef.SupportRoleARN
268-
rosaScope.ControlPlane.Spec.WorkerRoleARN = rosaRoleConfig.Status.AccountRolesRef.WorkerRoleARN
269-
rosaScope.ControlPlane.Spec.RolesRef = rosaRoleConfig.Status.OperatorRolesRef
270-
rosaScope.ControlPlane.Spec.EnableExternalAuthProviders = len(rosaRoleConfig.Spec.OIDCConfig.ExternalAuthProviders) > 0
264+
} else {
265+
rosaRoleConfig.Status.OIDCID = rosaScope.ControlPlane.Spec.OIDCID
266+
rosaRoleConfig.Status.AccountRolesRef.InstallerRoleARN = rosaScope.ControlPlane.Spec.InstallerRoleARN
267+
rosaRoleConfig.Status.AccountRolesRef.SupportRoleARN = rosaScope.ControlPlane.Spec.SupportRoleARN
268+
rosaRoleConfig.Status.AccountRolesRef.WorkerRoleARN = rosaScope.ControlPlane.Spec.WorkerRoleARN
269+
rosaRoleConfig.Status.OperatorRolesRef = rosaScope.ControlPlane.Spec.RolesRef
270+
rosaRoleConfig.Spec.OIDCConfig.ExternalAuthProviders = rosaScope.ControlPlane.Spec.ExternalAuthProviders
271271
}
272272

273273
validationMessage, err := validateControlPlaneSpec(ocmClient, rosaScope)
274274
if err != nil {
275275
return ctrl.Result{}, fmt.Errorf("failed to validate ROSAControlPlane.spec: %w", err)
276276
}
277277

278+
err = validateRoleConfigSpec(rosaRoleConfig)
279+
if err != nil {
280+
return ctrl.Result{}, fmt.Errorf("failed to validate ROSAControlPlane.spec: %w", err)
281+
}
282+
278283
conditions.MarkTrue(rosaScope.ControlPlane, rosacontrolplanev1.ROSAControlPlaneValidCondition)
279284
if validationMessage != "" {
280285
conditions.MarkFalse(rosaScope.ControlPlane,
@@ -357,7 +362,7 @@ func (r *ROSAControlPlaneReconciler) reconcileNormal(ctx context.Context, rosaSc
357362
return ctrl.Result{RequeueAfter: time.Second * 60}, nil
358363
}
359364

360-
ocmClusterSpec, err := buildOCMClusterSpec(rosaScope.ControlPlane.Spec, creator)
365+
ocmClusterSpec, err := buildOCMClusterSpec(rosaScope.ControlPlane.Spec, rosaRoleConfig, creator)
361366
if err != nil {
362367
return ctrl.Result{}, err
363368
}
@@ -951,7 +956,7 @@ func validateControlPlaneSpec(ocmClient rosa.OCMClient, rosaScope *scope.ROSACon
951956
return "", nil
952957
}
953958

954-
func buildOCMClusterSpec(controlPlaneSpec rosacontrolplanev1.RosaControlPlaneSpec, creator *rosaaws.Creator) (ocm.Spec, error) {
959+
func buildOCMClusterSpec(controlPlaneSpec rosacontrolplanev1.RosaControlPlaneSpec, roleConfig *expinfrav1.ROSARoleConfig, creator *rosaaws.Creator) (ocm.Spec, error) {
955960
billingAccount := controlPlaneSpec.BillingAccount
956961
if billingAccount == "" {
957962
billingAccount = creator.AccountID
@@ -975,11 +980,11 @@ func buildOCMClusterSpec(controlPlaneSpec rosacontrolplanev1.RosaControlPlaneSpe
975980

976981
SubnetIds: controlPlaneSpec.Subnets,
977982
IsSTS: true,
978-
RoleARN: controlPlaneSpec.InstallerRoleARN,
979-
SupportRoleARN: controlPlaneSpec.SupportRoleARN,
980-
WorkerRoleARN: controlPlaneSpec.WorkerRoleARN,
981-
OperatorIAMRoles: operatorIAMRoles(controlPlaneSpec.RolesRef),
982-
OidcConfigId: controlPlaneSpec.OIDCID,
983+
RoleARN: roleConfig.Status.AccountRolesRef.InstallerRoleARN,
984+
SupportRoleARN: roleConfig.Status.AccountRolesRef.SupportRoleARN,
985+
WorkerRoleARN: roleConfig.Status.AccountRolesRef.WorkerRoleARN,
986+
OperatorIAMRoles: operatorIAMRoles(roleConfig.Status.OperatorRolesRef),
987+
OidcConfigId: roleConfig.Status.OIDCID,
983988
Mode: "auto",
984989
Hypershift: ocm.Hypershift{
985990
Enabled: true,
@@ -1183,3 +1188,55 @@ func convertStsV2(identity *sts.GetCallerIdentityOutput) *stsv2.GetCallerIdentit
11831188
UserId: identity.UserId,
11841189
}
11851190
}
1191+
1192+
func validateRoleConfigSpec(roleConfig *expinfrav1.ROSARoleConfig) error {
1193+
if roleConfig.Status.OIDCID == "" {
1194+
return fmt.Errorf("OIDCID is required")
1195+
}
1196+
1197+
if roleConfig.Status.AccountRolesRef.InstallerRoleARN == "" {
1198+
return fmt.Errorf("InstallerRoleARN is required")
1199+
}
1200+
1201+
if roleConfig.Status.AccountRolesRef.SupportRoleARN == "" {
1202+
return fmt.Errorf("SupportRoleARN is required")
1203+
}
1204+
1205+
if roleConfig.Status.AccountRolesRef.WorkerRoleARN == "" {
1206+
return fmt.Errorf("WorkerRoleARN is required")
1207+
}
1208+
1209+
if roleConfig.Status.OperatorRolesRef.IngressARN == "" {
1210+
return fmt.Errorf("IngressARN is required")
1211+
}
1212+
1213+
if roleConfig.Status.OperatorRolesRef.ImageRegistryARN == "" {
1214+
return fmt.Errorf("ImageRegistryARN is required")
1215+
}
1216+
1217+
if roleConfig.Status.OperatorRolesRef.StorageARN == "" {
1218+
return fmt.Errorf("StorageARN is required")
1219+
}
1220+
1221+
if roleConfig.Status.OperatorRolesRef.NetworkARN == "" {
1222+
return fmt.Errorf("NetworkARN is required")
1223+
}
1224+
1225+
if roleConfig.Status.OperatorRolesRef.KubeCloudControllerARN == "" {
1226+
return fmt.Errorf("KubeCloudControllerARN is required")
1227+
}
1228+
1229+
if roleConfig.Status.OperatorRolesRef.KMSProviderARN == "" {
1230+
return fmt.Errorf("KMSProviderARN is required")
1231+
}
1232+
1233+
if roleConfig.Status.OperatorRolesRef.ControlPlaneOperatorARN == "" {
1234+
return fmt.Errorf("ControlPlaneOperatorARN is required")
1235+
}
1236+
1237+
if roleConfig.Status.OperatorRolesRef.NodePoolManagementARN == "" {
1238+
return fmt.Errorf("NodePoolManagementARN is required")
1239+
}
1240+
1241+
return nil
1242+
}

‎exp/api/v1beta2/rosaroleconfig_types.go‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,20 +60,27 @@ type ROSARoleConfig struct {
6060
// AccountRoleConfig defines account-wide IAM roles before creating your ROSA cluster.
6161
type AccountRoleConfig struct {
6262
// User-defined prefix for all generated AWS resources
63-
// +kubebuilder:validation:MaxLength:=4
63+
// +kubebuilder:validation:MaxLength:=40
6464
// +kubebuilder:validation:Required
65+
// +immutable
6566
Prefix string `json:"prefix"`
6667
// The ARN of the policy that is used to set the permissions boundary for the account roles.
6768
// +optional
69+
// +immutable
70+
6871
PermissionsBoundaryARN string `json:"permissionsBoundaryARN,omitempty"`
6972
// The arn path for the account/operator roles as well as their policies.
7073
// +optional
74+
// +immutable
75+
7176
Path string `json:"path,omitempty"`
7277
// Version of OpenShift that will be used to setup policy tag, for example "4.11"
7378
// +kubebuilder:validation:Required
79+
// +immutable
7480
Version string `json:"version"`
7581
// SharedVPCConfig is used to set up shared VPC.
7682
// +optional
83+
// +immutable
7784
SharedVPCConfig SharedVPCConfig `json:"sharedVPCConfig,omitempty"`
7885
}
7986

@@ -82,12 +89,15 @@ type OperatorRoleConfig struct {
8289
// User-defined prefix for generated AWS operator policies.
8390
// +kubebuilder:validation:MaxLength:=4
8491
// +kubebuilder:validation:Required
92+
// +immutable
8593
Prefix string `json:"prefix"`
8694
// The ARN of the policy that is used to set the permissions boundary for the operator roles.
8795
// +optional
96+
// +immutable
8897
PermissionsBoundaryARN string `json:"permissionsBoundaryARN,omitempty"`
8998
// SharedVPCConfig is used to set up shared VPC.
9099
// +optional
100+
// +immutable
91101
SharedVPCConfig SharedVPCConfig `json:"sharedVPCConfig,omitempty"`
92102
}
93103

@@ -104,6 +114,7 @@ type SharedVPCConfig struct {
104114
type OIDCConfig struct {
105115
// ManagedOIDC indicates whether it is a Red Hat managed or unmanaged (Customer hosted) OIDC Configuration. Default is true.
106116
// +kubebuilder:default=true
117+
// +immutable
107118
ManagedOIDC bool `json:"managedOIDC"`
108119
// ExternalAuthProviders are external OIDC identity providers that can issue tokens for this cluster.
109120
// Can only be set if "enableExternalAuthProviders" is set to "True".
@@ -113,10 +124,17 @@ type OIDCConfig struct {
113124
// +listType=map
114125
// +listMapKey=name
115126
// +kubebuilder:validation:MaxItems=1
127+
// +immutable
116128
ExternalAuthProviders []rosacontrolplanev1.ExternalAuthProvider `json:"externalAuthProviders,omitempty"`
117-
IdentityRef *infrav1.AWSIdentityReference `json:"identityRef,omitempty"`
118-
Region string `json:"region,omitempty"`
119-
Prefix string `json:"prefix"`
129+
// IdentityRef is the AWS identity reference for the OIDC config.
130+
// +immutable
131+
IdentityRef *infrav1.AWSIdentityReference `json:"identityRef,omitempty"`
132+
// Region is the AWS region for the OIDC config.
133+
// +immutable
134+
Region string `json:"region,omitempty"`
135+
// Prefix is the prefix for the OIDC config.
136+
// +immutable
137+
Prefix string `json:"prefix"`
120138
}
121139

122140
// ROSARoleConfigStatus defines the observed state of ROSARoleConfig

‎exp/controllers/rosaroleconfig_controller.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222
"fmt"
2323
"net/url"
2424
"strings"
25-
"time"
2625

2726
"github.com/aws/aws-sdk-go/service/sts/stsiface"
2827
"github.com/go-logr/logr"
@@ -141,26 +140,26 @@ func (r *ROSARoleConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reque
141140
err = r.createOIDCConfig(roleConfig, scope, ocmClient)
142141
if err != nil {
143142
conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Failed to create OIDC Config: %v", err)
144-
return ctrl.Result{RequeueAfter: time.Second * 60}, fmt.Errorf("failed to OICD Config: %w", err)
143+
return ctrl.Result{}, fmt.Errorf("failed to OICD Config: %w", err)
145144
}
146145
}
147146

148147
err = r.createAccountRoles(ctx, roleConfig, scope, ocmClient)
149148
if err != nil {
150149
conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Failed to create Account Roles: %v", err)
151-
return ctrl.Result{RequeueAfter: time.Second * 60}, fmt.Errorf("failed to Create AccountRoles: %w", err)
150+
return ctrl.Result{}, fmt.Errorf("failed to Create AccountRoles: %w", err)
152151
}
153152

154153
err = r.createOperatorRoles(ctx, roleConfig, scope, ocmClient)
155154
if err != nil {
156155
conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Failed to create Operator Roles: %v", err)
157-
return ctrl.Result{RequeueAfter: time.Second * 60}, fmt.Errorf("failed to Create OperatorRoles: %w", err)
156+
return ctrl.Result{}, fmt.Errorf("failed to Create OperatorRoles: %w", err)
158157
}
159158

160159
err = r.createOIDCProvider(scope, ocmClient)
161160
if err != nil {
162161
conditions.MarkFalse(scope.RosaRoleConfig, expinfrav1.RosaRoleConfigReadyCondition, expinfrav1.RosaRoleConfigReconciliationFailedReason, clusterv1.ConditionSeverityError, "Failed to create OIDC provider: %v", err)
163-
return ctrl.Result{RequeueAfter: time.Second * 60}, fmt.Errorf("failed to Create OIDC provider: %w", err)
162+
return ctrl.Result{}, fmt.Errorf("failed to Create OIDC provider: %w", err)
164163
}
165164

166165
if r.rosaRolesConfigReady(scope) {

‎main.go‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -285,15 +285,20 @@ func main() {
285285
setupLog.Error(err, "unable to create webhook", "webhook", "ROSAMachinePool")
286286
os.Exit(1)
287287
}
288-
}
289288

290-
if err = (&expcontrollers.ROSARoleConfigReconciler{
291-
Client: mgr.GetClient(),
292-
Log: ctrl.Log.WithName("controllers").WithName("ROSARoleConfig"),
293-
Scheme: mgr.GetScheme(),
294-
}).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: awsClusterConcurrency, RecoverPanic: ptr.To[bool](true)}); err != nil {
295-
setupLog.Error(err, "unable to create controller", "controller", "ROSARoleConfig")
296-
os.Exit(1)
289+
if err = (&expcontrollers.ROSARoleConfigReconciler{
290+
Client: mgr.GetClient(),
291+
Log: ctrl.Log.WithName("controllers").WithName("ROSARoleConfig"),
292+
Scheme: mgr.GetScheme(),
293+
}).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: awsClusterConcurrency, RecoverPanic: ptr.To[bool](true)}); err != nil {
294+
setupLog.Error(err, "unable to create controller", "controller", "ROSARoleConfig")
295+
os.Exit(1)
296+
}
297+
298+
if err := (&expinfrav1.ROSARoleConfig{}).SetupWebhookWithManager(mgr); err != nil {
299+
setupLog.Error(err, "unable to create webhook", "webhook", "ROSARoleConfig")
300+
os.Exit(1)
301+
}
297302
}
298303
// +kubebuilder:scaffold:builder
299304

0 commit comments

Comments
 (0)