fix adb webhook and reconcile issue

Author: ting-lan-wang

Makefile

@@ -61,7 +61,7 @@ test: manifests generate fmt vet envtest ## Run unit tests.
 
 E2ETEST ?= ./test/e2e/
 e2e: manifests generate fmt vet envtest ## Run e2e tests.
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" ginkgo -v --timeout=2h30m --fail-fast $(E2ETEST)
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test $(E2ETEST) -test.timeout 0 -test.v --ginkgo.fail-fast
 
 ##@ Build
 

apis/database/v1alpha1/autonomousdatabase_webhook.go

@@ -164,7 +164,9 @@ func (r *AutonomousDatabase) ValidateUpdate(old runtime.Object) error {
 	// cannot change lifecycleState with other fields together (except the oci config)
 	var lifecycleChanged, otherFieldsChanged bool
 
-	lifecycleChanged = oldADB.Spec.Details.LifecycleState != "" && oldADB.Spec.Details.LifecycleState != r.Spec.Details.LifecycleState
+	lifecycleChanged = oldADB.Spec.Details.LifecycleState != "" &&
+		r.Spec.Details.LifecycleState != "" &&
+		oldADB.Spec.Details.LifecycleState != r.Spec.Details.LifecycleState
 	var copiedADB *AutonomousDatabaseSpec = r.Spec.DeepCopy()
 	copiedADB.Details.LifecycleState = oldADB.Spec.Details.LifecycleState
 	copiedADB.OCIConfig = oldADB.Spec.OCIConfig

controllers/database/autonomousdatabase_controller.go

@@ -157,10 +157,15 @@ func (r *AutonomousDatabaseReconciler) eventFilterPredicate() predicate.Predicat
 			if adbOk {
 				oldADB := e.ObjectOld.(*dbv1alpha1.AutonomousDatabase)
 
-				if !reflect.DeepEqual(oldADB.Status, desiredADB.Status) ||
-					(controllerutil.ContainsFinalizer(oldADB, dbv1alpha1.LastSuccessfulSpec) != controllerutil.ContainsFinalizer(desiredADB, dbv1alpha1.LastSuccessfulSpec)) ||
+				statusChanged := !reflect.DeepEqual(oldADB.Status, desiredADB.Status)
+
+				oldLastSucSpec := oldADB.GetAnnotations()[dbv1alpha1.LastSuccessfulSpec]
+				desiredLastSucSpec := desiredADB.GetAnnotations()[dbv1alpha1.LastSuccessfulSpec]
+				lastSucSpecChanged := oldLastSucSpec != desiredLastSucSpec
+
+				if statusChanged || lastSucSpecChanged ||
 					(controllerutil.ContainsFinalizer(oldADB, dbv1alpha1.ADBFinalizer) != controllerutil.ContainsFinalizer(desiredADB, dbv1alpha1.ADBFinalizer)) {
-					// Don't enqueue if the status, lastSucSpec, or the finalizler changes
+					// Don't enqueue if the status, lastSucSpec, or the finalizler changes the first time
 					return false
 				}
 
@@ -458,8 +463,6 @@ func (r *AutonomousDatabaseReconciler) validateOperation(
 	} else {
 		l.Info("No operation specified; sync the resource")
 
-		testOldADB := adb.DeepCopy()
-
 		// The user doesn't change the spec and the controller should pull the spec from the OCI.
 		specChanged, err := r.getADB(logger, adb)
 		if err != nil {
@@ -470,12 +473,12 @@ func (r *AutonomousDatabaseReconciler) validateOperation(
 			l.Info("The local spec doesn't match the oci's spec; update the CR")
 
 			// Erase the status.lifecycleState temporarily to avoid the webhook error.
-			oldADB := adb.DeepCopy()
+			tmpADB := adb.DeepCopy()
 			adb.Status.LifecycleState = ""
-			r.KubeClient.Status().Update(context.TODO(), adb)
-			adb.Spec = oldADB.Spec
-
-			adb.DeepCopy().RemoveUnchangedDetails(testOldADB.Spec)
+			if err := r.KubeClient.Status().Update(context.TODO(), adb); err != nil {
+				return false, emptyResult, err
+			}
+			adb.Spec = tmpADB.Spec
 
 			if err := r.updateCR(adb); err != nil {
 				return false, emptyResult, err
@@ -580,7 +583,9 @@ func (r *AutonomousDatabaseReconciler) validateFinalizer(logger logr.Logger, adb
 // updateCR updates the lastSucSpec and the CR
 func (r *AutonomousDatabaseReconciler) updateCR(adb *dbv1alpha1.AutonomousDatabase) error {
 	// Update the lastSucSpec
-	if err := adb.UpdateLastSuccessfulSpec(); err != nil {
+	// Should patch the lastSuccessfulSpec first, otherwise, the update event will be
+	// filtered out by predicate since the lastSuccessfulSpec is changed.
+	if err := r.patchLastSuccessfulSpec(adb); err != nil {
 		return err
 	}
 
@@ -933,22 +938,23 @@ func (r *AutonomousDatabaseReconciler) validateDesiredLifecycleState(
 }
 
 // The logic of updating the network access configurations is as follows:
-// 1. Shared databases:
-// 	 If the network access type changes
-//   a. to PUBLIC:
+//
+//  1. Shared databases:
+//     If the network access type changes
+//     a. to PUBLIC:
 //     was RESTRICTED: re-enable IsMTLSConnectionRequired if its not. Then set WhitelistedIps to an array with a single empty string entry.
 //     was PRIVATE: re-enable IsMTLSConnectionRequired if its not. Then set PrivateEndpointLabel to an emtpy string.
-//   b. to RESTRICTED:
+//     b. to RESTRICTED:
 //     was PUBLIC: set WhitelistedIps to desired IPs/CIDR blocks/VCN OCID. Configure the IsMTLSConnectionRequired settings if it is set to disabled.
 //     was PRIVATE: re-enable IsMTLSConnectionRequired if its not. Set the type to PUBLIC first, and then configure the WhitelistedIps. Finally resume the IsMTLSConnectionRequired settings if it was, or is configured as disabled.
-//   c. to PRIVATE:
+//     c. to PRIVATE:
 //     was PUBLIC: set subnetOCID and nsgOCIDs. Configure the IsMTLSConnectionRequired settings if it is set.
 //     was RESTRICTED: set subnetOCID and nsgOCIDs. Configure the IsMTLSConnectionRequired settings if it is set.
 //
-// 	 Otherwise, if the network access type remains the same, apply the network configuration, and then set the IsMTLSConnectionRequired.
+//     Otherwise, if the network access type remains the same, apply the network configuration, and then set the IsMTLSConnectionRequired.
 //
-// 2. Dedicated databases:
-//   Apply the configs directly
+//  2. Dedicated databases:
+//     Apply the configs directly
 func (r *AutonomousDatabaseReconciler) validateGeneralNetworkAccess(
 	logger logr.Logger,
 	adb *dbv1alpha1.AutonomousDatabase,