chore(oauth): remove modifications to OpenShift api-server (cryostati…

…o#599) * chore(oauth): remove modifications to OpenShift api-server Signed-off-by: Thuan Vo <thvo@redhat.com> * chore(tests): remove unused utils * fix(api-server): ensure existing modifications to be removed when reconciling * chore(tests): avoid hard-coded application URL in expects * fix(openshift): only update api server if an application URL is found --------- Signed-off-by: Thuan Vo <thvo@redhat.com>
tthvo · Jul 17, 2023 · 0ad9f86 · 0ad9f86
1 parent 1a02ba8
commit 0ad9f86
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 61 deletions.
diff --git a/internal/controllers/openshift.go b/internal/controllers/openshift.go
@@ -60,7 +60,8 @@ func (r *Reconciler) reconcileOpenShift(ctx context.Context, cr *model.CryostatI
 	if err != nil {
 		return err
 	}
-	return r.addCorsAllowedOriginIfNotPresent(ctx, cr)
+	// Remove CORS modifications from previous operator versions
+	return r.deleteCorsAllowedOrigins(ctx, cr)
 }
 
 func (r *Reconciler) finalizeOpenShift(ctx context.Context, cr *model.CryostatInstance) error {
@@ -130,44 +131,6 @@ func (r *Reconciler) deleteConsoleLink(ctx context.Context, link *consolev1.Cons
 	return nil
 }
 
-func (r *Reconciler) addCorsAllowedOriginIfNotPresent(ctx context.Context, cr *model.CryostatInstance) error {
-	reqLogger := r.Log.WithValues("Request.Namespace", cr.InstallNamespace, "Request.Name", cr.Name)
-
-	allowedOrigin := cr.Status.ApplicationURL
-	if len(allowedOrigin) == 0 {
-		return nil
-	}
-
-	apiServer := &configv1.APIServer{}
-	err := r.Client.Get(ctx, types.NamespacedName{Name: apiServerName}, apiServer)
-	if err != nil {
-		reqLogger.Error(err, "Failed to get APIServer config")
-		return err
-	}
-
-	allowedOriginAsRegex := regexp.QuoteMeta(allowedOrigin)
-
-	for _, origin := range apiServer.Spec.AdditionalCORSAllowedOrigins {
-		if origin == allowedOriginAsRegex {
-			return nil
-		}
-	}
-
-	apiServer.Spec.AdditionalCORSAllowedOrigins = append(
-		apiServer.Spec.AdditionalCORSAllowedOrigins,
-		allowedOriginAsRegex,
-	)
-
-	err = r.Client.Update(ctx, apiServer)
-	if err != nil {
-		reqLogger.Error(err, "Failed to update APIServer CORS allowed origins")
-		return err
-	}
-
-	reqLogger.Info("Added to APIServer CORS allowed origins")
-	return nil
-}
-
 func (r *Reconciler) deleteCorsAllowedOrigins(ctx context.Context, cr *model.CryostatInstance) error {
 	reqLogger := r.Log.WithValues("Request.Namespace", cr.InstallNamespace, "Request.Name", cr.Name)
 
@@ -191,16 +154,15 @@ func (r *Reconciler) deleteCorsAllowedOrigins(ctx context.Context, cr *model.Cry
 			apiServer.Spec.AdditionalCORSAllowedOrigins = append(
 				apiServer.Spec.AdditionalCORSAllowedOrigins[:i],
 				apiServer.Spec.AdditionalCORSAllowedOrigins[i+1:]...)
-			break
-		}
-	}
+			err = r.Client.Update(ctx, apiServer)
+			if err != nil {
+				reqLogger.Error(err, "Failed to remove Cryostat origin from APIServer CORS allowed origins")
+				return err
+			}
 
-	err = r.Client.Update(ctx, apiServer)
-	if err != nil {
-		reqLogger.Error(err, "Failed to remove Cryostat origin from APIServer CORS allowed origins")
-		return err
+			reqLogger.Info("Removed from APIServer CORS allowed origins")
+			return nil
+		}
 	}
-
-	reqLogger.Info("Removed from APIServer CORS allowed origins")
 	return nil
 }
diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go
@@ -1282,8 +1282,20 @@ func (c *controllerTest) commonTests() {
 					Expect(link.Spec.NamespaceDashboard).To(Equal(expectedLink.Spec.NamespaceDashboard))
 				})
 			})
-			It("should add application url to APIServer AdditionalCORSAllowedOrigins", func() {
-				t.expectAPIServer()
+			Context("with an existing application url in APIServer AdditionalCORSAllowedOrigins", func() {
+				BeforeEach(func() {
+					t.objs = []ctrlclient.Object{
+						t.NewApiServerWithApplicationURL(),
+						t.NewNamespace(),
+					}
+				})
+				It("should remove the application url", func() {
+					apiServer := &configv1.APIServer{}
+					err := t.Client.Get(context.Background(), types.NamespacedName{Name: "cluster"}, apiServer)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(apiServer.Spec.AdditionalCORSAllowedOrigins).ToNot(ContainElement(fmt.Sprintf("https://%s\\.example\\.com", t.Name)))
+					Expect(apiServer.Spec.AdditionalCORSAllowedOrigins).To(ContainElement("https://an-existing-user-specified\\.allowed\\.origin\\.com"))
+				})
 			})
 			It("should add the finalizer", func() {
 				t.expectCryostatFinalizerPresent()
@@ -1319,7 +1331,7 @@ func (c *controllerTest) commonTests() {
 						apiServer := &configv1.APIServer{}
 						err := t.Client.Get(context.Background(), types.NamespacedName{Name: "cluster"}, apiServer)
 						Expect(err).ToNot(HaveOccurred())
-						Expect(apiServer.Spec.AdditionalCORSAllowedOrigins).ToNot(ContainElement("https://cryostat\\.example\\.com"))
+						Expect(apiServer.Spec.AdditionalCORSAllowedOrigins).ToNot(ContainElement(fmt.Sprintf("https://%s\\.example\\.com", t.Name)))
 						Expect(apiServer.Spec.AdditionalCORSAllowedOrigins).To(ContainElement("https://an-existing-user-specified\\.allowed\\.origin\\.com"))
 					})
 					It("should delete Cryostat", func() {
@@ -1811,9 +1823,6 @@ func (c *controllerTest) commonTests() {
 						It("should not delete exisiting ConsoleLink", func() {
 							otherInput.expectConsoleLink()
 						})
-						It("should not remove CORS entry from APIServer", func() {
-							otherInput.expectAPIServer()
-						})
 					})
 				})
 			})
@@ -2966,13 +2975,6 @@ func (t *cryostatTestInput) expectConsoleLink() {
 	Expect(link.Spec).To(Equal(expectedLink.Spec))
 }
 
-func (t *cryostatTestInput) expectAPIServer() {
-	apiServer := &configv1.APIServer{}
-	err := t.Client.Get(context.Background(), types.NamespacedName{Name: "cluster"}, apiServer)
-	Expect(err).ToNot(HaveOccurred())
-	Expect(apiServer.Spec.AdditionalCORSAllowedOrigins).To(ContainElement(fmt.Sprintf("https://%s\\.example\\.com", t.Name)))
-}
-
 func (t *cryostatTestInput) expectResourcesUnaffected() {
 	for _, check := range resourceChecks() {
 		check.expectFunc(t)

diff --git a/internal/test/resources.go b/internal/test/resources.go
@@ -2636,6 +2636,20 @@ func (r *TestResources) NewApiServer() *configv1.APIServer {
 	}
 }
 
+func (r *TestResources) NewApiServerWithApplicationURL() *configv1.APIServer {
+	return &configv1.APIServer{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "cluster",
+		},
+		Spec: configv1.APIServerSpec{
+			AdditionalCORSAllowedOrigins: []string{
+				"https://an-existing-user-specified\\.allowed\\.origin\\.com",
+				fmt.Sprintf("https://%s.example.com", r.Name),
+			},
+		},
+	}
+}
+
 func newCoreContainerDefaultResource() *corev1.ResourceRequirements {
 	return &corev1.ResourceRequirements{
 		Requests: corev1.ResourceList{