Merge pull request operator-framework#364 from alecmerdler/ALM-623

Resolve Install Plan before Requiring Approval

Documentation/design/architecture.md

@@ -1,8 +1,8 @@
 # Architecture
 
-OLM is composed of two operators: the OLM operator and the Catalog operator.
+OLM is composed of two Operators: the OLM Operator and the Catalog Operator.
 
-Each of these operators are responsible for managing the CRDs that are the basis for the OLM framework:
+Each of these Operators are responsible for managing the CRDs that are the basis for the OLM framework:
 
 | Resource                 | Short Name | Owner   | Description                                                                                |
 |--------------------------|------------|---------|--------------------------------------------------------------------------------------------|
@@ -11,7 +11,7 @@ Each of these operators are responsible for managing the CRDs that are the basis
 | CatalogSource-v1         | CS         | Catalog | a repository of CSVs, CRDs, and packages that define an application                        |
 | Subscription-v1          | Sub        | Catalog | used to keep CSVs up to date by tracking a channel in a package                            |
 
-Each of these operators are also responsible for creating resources:
+Each of these Operators are also responsible for creating resources:
 
 | Operator | Creatable Resources        |
 |----------|----------------------------|
@@ -37,17 +37,17 @@ ClusterServiceVersion:
    - Type
    - Owned - managed by this service
    - Required - must exist in the cluster for this service to run
-   - Resources - a list of k8s resources that the operator interacts with
+   - Resources - a list of k8s resources that the Operator interacts with
    - Descriptors - annotate CRD spec and status fields to provide semantic information
 
 
 ## OLM Operator
 
-The OLM operator is responsible to deploying applications defined by ClusterServiceVersion-v1 resources once the required resources specified in the ClusterServiceVersion-v1 are present in the cluster.
-The OLM operator is not concerned with the creation of the required resources; users can choose to manually create these resources using `kubectl` or users can choose to create these resources using the Catalog operator.
+The OLM Operator is responsible to deploying applications defined by ClusterServiceVersion-v1 resources once the required resources specified in the ClusterServiceVersion-v1 are present in the cluster.
+The OLM Operator is not concerned with the creation of the required resources; users can choose to manually create these resources using `kubectl` or users can choose to create these resources using the Catalog Operator.
 This separation of concern enables users incremental buy-in in terms of how much of the OLM framework they choose to leverage for their application.
 
-While the OLM operator is often configured to watch all namespaces, it can also be operated alongside other OLM operators so long as they all manage separate namespaces.
+While the OLM Operator is often configured to watch all namespaces, it can also be operated alongside other OLM Operators so long as they all manage separate namespaces.
 
 ### ClusterServiceVersion-v1 Control Loop
 
@@ -66,9 +66,9 @@ Replacing --> Deleting
 
 | Phase      | Description                                                                                                            |
 |------------|------------------------------------------------------------------------------------------------------------------------|
-| None       | initial phase, once seen by the operator, it is immediately transitioned to `Pending`                                  |
+| None       | initial phase, once seen by the Operator, it is immediately transitioned to `Pending`                                  |
 | Pending    | requirements in the CSV are not met, once they are this phase transitions to `Installing`                              |
-| InstallReady | all requirements in the CSV are present, the operator will begin executing the install strategy                      |
+| InstallReady | all requirements in the CSV are present, the Operator will begin executing the install strategy                      |
 | Installing | the install strategy is being executed and resources are being created, but not all components are reporting as ready  |
 | Succeeded  | the execution of the Install Strategy was successful; if requirements disappear, this may transition back to `Pending` |
 | Failed     | upon failed execution of the Install Strategy, the CSV transitions to this terminal phase                              |
@@ -77,29 +77,33 @@ Replacing --> Deleting
 
 ### Namespace Control Loop
 
-In addition to watching the creation of ClusterServiceVersion-v1s in a set of namespaces, the OLM operator also watches those namespaces themselves.
-If a namespace that the OLM operator is configured to watch is created, the OLM operator will annotate that namespace with the `alm-manager` key.
+In addition to watching the creation of ClusterServiceVersion-v1s in a set of namespaces, the OLM Operator also watches those namespaces themselves.
+If a namespace that the OLM Operator is configured to watch is created, the OLM Operator will annotate that namespace with the `alm-manager` key.
 This enables dashboards and users of `kubectl` to filter namespaces based on what OLM is managing.
 
 ## Catalog Operator
 
-The Catalog operator is responsible for resolving and installing ClusterServiceVersion-v1s and the required resources they specify. It is also responsible for watching catalog sources for updates to packages in channels, and upgrading them (optionally automatically) to the latest available versions.
+The Catalog Operator is responsible for resolving and installing ClusterServiceVersion-v1s and the required resources they specify. It is also responsible for watching catalog sources for updates to packages in channels, and upgrading them (optionally automatically) to the latest available versions.
 A user that wishes to track a package in a channel creates a Subscription-v1 resource configuring the desired package, channel, and the catalog source from which to pull updates. When updates are found, an appropriate InstallPlan-v1 is written into the namespace on behalf of the user.
-Users can also create an InstallPlan-v1 resource directly, containing the names of the desired ClusterServiceVersion-v1s and an approval strategy and the Catalog operator will create an execution plan for the creation of all of the required resources.
-Once approved, the Catalog operator will create all of the resources in an InstallPlan-v1; this should then independently satisfy the OLM operator, which will proceed to install the ClusterServiceVersion-v1s.
+Users can also create an InstallPlan-v1 resource directly, containing the names of the desired ClusterServiceVersion-v1s and an approval strategy and the Catalog Operator will create an execution plan for the creation of all of the required resources.
+Once approved, the Catalog Operator will create all of the resources in an InstallPlan-v1; this should then independently satisfy the OLM Operator, which will proceed to install the ClusterServiceVersion-v1s.
 
 ### InstallPlan-v1 Control Loop
 
 ```
-None --> Planning --> Installing --> Complete
+None --> Planning +------>------->------> Installing --> Complete
+                  |                       ^
+                  v                       |
+                  +--> RequiresApproval --+
 ```
 
-| Phase      | Description                                                                                    |
-|------------|------------------------------------------------------------------------------------------------|
-| None       | initial phase, once seen by the operator, it is immediately transitioned to `Planning`         |
-| Planning   | dependencies between resources are being resolved, to be stored in the InstallPlan-v1 `Status` |
-| Installing | resolved resources in the InstallPlan-v1 `Status` block are being created                      |
-| Complete   | all resolved resources in the `Status` block exist                                             |
+| Phase            | Description                                                                                    |
+|------------------|------------------------------------------------------------------------------------------------|
+| None             | initial phase, once seen by the Operator, it is immediately transitioned to `Planning`         |
+| Planning         | dependencies between resources are being resolved, to be stored in the InstallPlan-v1 `Status` |
+| RequiresApproval | occurs when using manual approval, will not transition phase until `approved` field is true    |
+| Installing       | resolved resources in the InstallPlan-v1 `Status` block are being created                      |
+| Complete         | all resolved resources in the `Status` block exist                                             |
 
 ### Subscription-v1 Control Loop
 
@@ -112,7 +116,7 @@ None --> UpgradeAvailable --> UpgradePending --> AtLatestKnown -+
 
 | Phase            | Description                                                                                                   |
 |------------------|---------------------------------------------------------------------------------------------------------------|
-| None             | initial phase, once seen by the operator, it is immediately transitioned to `UpgradeAvailable`                |
+| None             | initial phase, once seen by the Operator, it is immediately transitioned to `UpgradeAvailable`                |
 | UpgradeAvailable | catalog contains a CSV which replaces the `status.installedCSV`, but no `InstallPlan-v1` has been created yet |
 | UpgradePending   | `InstallPlan-v1` has been created (referenced in `status.installplan`) to install a new CSV                   |
 | AtLatestKnown    | `status.installedCSV` matches the latest available CSV in catalog                                             |
@@ -122,7 +126,7 @@ None --> UpgradeAvailable --> UpgradePending --> AtLatestKnown -+
 
 The Catalog Registry stores CSVs and CRDs for creation in a cluster, and stores metadata about packages and channels.
 
-A package manifest is an entry in the catalog registry that associates a package identity with sets of ClusterServiceVersion-v1s. Within a package, channels which point to a particular CSV. Because CSVs explicitly reference the CSV that they replace, a package manifest provides the catalog operator needs to update a CSV to the latest version in a channel (stepping through each intermediate version).
+A package manifest is an entry in the catalog registry that associates a package identity with sets of ClusterServiceVersion-v1s. Within a package, channels which point to a particular CSV. Because CSVs explicitly reference the CSV that they replace, a package manifest provides the catalog Operator needs to update a CSV to the latest version in a channel (stepping through each intermediate version).
 
 ```
 Package {name}

pkg/api/apis/installplan/v1alpha1/types.go

@@ -54,7 +54,6 @@ type InstallPlanConditionType string
 
 const (
 	InstallPlanResolved  InstallPlanConditionType = "Resolved"
-	InstallPlanApproved  InstallPlanConditionType = "Approved"
 	InstallPlanInstalled InstallPlanConditionType = "Installed"
 )
 

pkg/controller/operators/catalog/operator.go

@@ -206,23 +206,12 @@ type installPlanTransitioner interface {
 var _ installPlanTransitioner = &Operator{}
 
 func transitionInstallPlanState(transitioner installPlanTransitioner, plan *v1alpha1.InstallPlan) error {
-	if plan.Spec.Approval == v1alpha1.ApprovalManual && plan.Spec.Approved != true {
-		log.Debugf("plan %s is not approved, skipping sync", plan.SelfLink)
-		plan.Status.Phase = v1alpha1.InstallPlanPhaseRequiresApproval
-		return nil
-	}
-
 	switch plan.Status.Phase {
 	case v1alpha1.InstallPlanPhaseNone:
 		log.Debugf("plan %s phase unrecognized, setting to Planning", plan.SelfLink)
 		plan.Status.Phase = v1alpha1.InstallPlanPhasePlanning
 		return nil
 
-	case v1alpha1.InstallPlanPhaseRequiresApproval:
-		log.Debugf("plan %s approved, setting to Planning", plan.SelfLink)
-		plan.Status.Phase = v1alpha1.InstallPlanPhasePlanning
-		return nil
-
 	case v1alpha1.InstallPlanPhasePlanning:
 		log.Debugf("plan %s phase Planning, attempting to resolve", plan.SelfLink)
 		if err := transitioner.ResolvePlan(plan); err != nil {
@@ -232,7 +221,21 @@ func transitionInstallPlanState(transitioner installPlanTransitioner, plan *v1al
 			return err
 		}
 		plan.Status.SetCondition(v1alpha1.ConditionMet(v1alpha1.InstallPlanResolved))
-		plan.Status.Phase = v1alpha1.InstallPlanPhaseInstalling
+
+		if plan.Spec.Approval == v1alpha1.ApprovalManual && plan.Spec.Approved != true {
+			plan.Status.Phase = v1alpha1.InstallPlanPhaseRequiresApproval
+		} else {
+			plan.Status.Phase = v1alpha1.InstallPlanPhaseInstalling
+		}
+		return nil
+
+	case v1alpha1.InstallPlanPhaseRequiresApproval:
+		if plan.Spec.Approved {
+			log.Debugf("plan %s approved, setting to Planning", plan.SelfLink)
+			plan.Status.Phase = v1alpha1.InstallPlanPhaseInstalling
+		} else {
+			log.Debugf("plan %s is not approved, skipping sync", plan.SelfLink)
+		}
 		return nil
 
 	case v1alpha1.InstallPlanPhaseInstalling:

pkg/controller/operators/catalog/operator_test.go

@@ -70,6 +70,8 @@ func TestTransitionInstallPlan(t *testing.T) {
 
 		{v1alpha1.InstallPlanPhasePlanning, nil, v1alpha1.ApprovalAutomatic, false, v1alpha1.InstallPlanPhaseInstalling, resolved},
 		{v1alpha1.InstallPlanPhasePlanning, nil, v1alpha1.ApprovalAutomatic, true, v1alpha1.InstallPlanPhaseInstalling, resolved},
+		{v1alpha1.InstallPlanPhasePlanning, nil, v1alpha1.ApprovalManual, false, v1alpha1.InstallPlanPhaseRequiresApproval, resolved},
+		{v1alpha1.InstallPlanPhasePlanning, nil, v1alpha1.ApprovalManual, true, v1alpha1.InstallPlanPhaseInstalling, resolved},
 		{v1alpha1.InstallPlanPhasePlanning, err, v1alpha1.ApprovalAutomatic, false, v1alpha1.InstallPlanPhaseFailed, unresolved},
 		{v1alpha1.InstallPlanPhasePlanning, err, v1alpha1.ApprovalAutomatic, true, v1alpha1.InstallPlanPhaseFailed, unresolved},
 
@@ -79,7 +81,7 @@ func TestTransitionInstallPlan(t *testing.T) {
 		{v1alpha1.InstallPlanPhaseInstalling, err, v1alpha1.ApprovalAutomatic, true, v1alpha1.InstallPlanPhaseFailed, failed},
 
 		{v1alpha1.InstallPlanPhaseRequiresApproval, nil, v1alpha1.ApprovalManual, false, v1alpha1.InstallPlanPhaseRequiresApproval, nil},
-		{v1alpha1.InstallPlanPhaseRequiresApproval, nil, v1alpha1.ApprovalManual, true, v1alpha1.InstallPlanPhasePlanning, nil},
+		{v1alpha1.InstallPlanPhaseRequiresApproval, nil, v1alpha1.ApprovalManual, true, v1alpha1.InstallPlanPhaseInstalling, nil},
 	}
 	for _, tt := range table {
 		// Create a plan in the provided initial phase.

test/e2e/alm_e2e_test.go

@@ -114,41 +114,73 @@ func TestCreateInstallPlanManualApproval(t *testing.T) {
 		},
 	}
 
-	// Create a new installplan for vault with manual approval
+	// Create a new InstallPlan for Vault with manual approval
 	cleanup, err := decorateCommonAndCreateInstallPlan(c, vaultInstallPlan)
 	require.NoError(t, err)
 	defer cleanup()
 
 	// Get InstallPlan and verify status
 	fetchedInstallPlan, err := fetchInstallPlan(t, c, vaultInstallPlan.GetName(), installPlanRequiresApprovalChecker)
 	require.NoError(t, err)
-	require.Equal(t, installplanv1alpha1.InstallPlanPhaseRequiresApproval, fetchedInstallPlan.Status.Phase)
 
-	vaultResourcesPresent := 0
-
-	// Step through the InstallPlan and check if resources have been created
-	for _, step := range fetchedInstallPlan.Status.Plan {
-		t.Logf("Verifiying that %s %s is not present", step.Resource.Kind, step.Resource.Name)
-		if step.Resource.Kind == "CustomResourceDefinition" {
-			_, err := c.GetCustomResourceDefinition(step.Resource.Name)
-
-			require.NoError(t, err)
-			vaultResourcesPresent = vaultResourcesPresent + 1
-		} else if step.Resource.Kind == "ClusterServiceVersion-v1" {
-			_, err := c.GetCustomResource(apis.GroupName, installplanv1alpha1.GroupVersion, testNamespace, step.Resource.Kind, step.Resource.Name)
-
-			require.NoError(t, err)
-			vaultResourcesPresent = vaultResourcesPresent + 1
-		} else if step.Resource.Kind == "Secret" {
-			_, err := c.KubernetesInterface().CoreV1().Secrets(testNamespace).Get(step.Resource.Name, metav1.GetOptions{})
-
-			require.NoError(t, err)
+	var verifyResources = func(installPlan *installplanv1alpha1.InstallPlan, shouldBeCreated bool) int {
+		resourcesPresent := 0
+		// Step through the InstallPlan and check if resources have been created or not
+		for _, step := range installPlan.Status.Plan {
+			t.Logf("Verifiying that %s %s is not present", step.Resource.Kind, step.Resource.Name)
+			if step.Resource.Kind == "CustomResourceDefinition" {
+				// _, err := c.GetCustomResourceDefinition(step.Resource.Name)
+
+				// FIXME: CI cluster will already have the CRDs so this will always fail
+				if shouldBeCreated {
+					// require.NoError(t, err)
+					// resourcesPresent = resourcesPresent + 1
+				} else {
+					// require.Error(t, err)
+				}
+			} else if step.Resource.Kind == "ClusterServiceVersion-v1" {
+				_, err := c.GetCustomResource(apis.GroupName, installplanv1alpha1.GroupVersion, testNamespace, step.Resource.Kind, step.Resource.Name)
+
+				if shouldBeCreated {
+					require.NoError(t, err)
+					resourcesPresent = resourcesPresent + 1
+				} else {
+					require.Error(t, err)
+				}
+			} else if step.Resource.Kind == "Secret" {
+				_, err := c.KubernetesInterface().CoreV1().Secrets(testNamespace).Get(step.Resource.Name, metav1.GetOptions{})
+
+				if shouldBeCreated {
+					require.NoError(t, err)
+					resourcesPresent = resourcesPresent + 1
+				} else {
+					require.Error(t, err)
+				}
+			}
 		}
+		return resourcesPresent
 	}
 
-	// Result: Ensure that the InstallPlan actually creates no vault resources
+	vaultResourcesPresent := verifyResources(fetchedInstallPlan, false)
+	// Result: Ensure that the InstallPlan does not actually create Vault resources
 	t.Logf("%d Vault Resources present", vaultResourcesPresent)
 	require.Zero(t, vaultResourcesPresent)
+
+	// Approve InstallPlan and update
+	fetchedInstallPlan.Spec.Approved = true
+	ipUnst, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&fetchedInstallPlan)
+	require.NoError(t, err)
+	err = c.UpdateCustomResource(&unstructured.Unstructured{Object: ipUnst})
+	require.NoError(t, err)
+
+	approvedInstallPlan, err := fetchInstallPlan(t, c, fetchedInstallPlan.GetName(), installPlanCompleteChecker)
+	require.NoError(t, err)
+
+	vaultResourcesPresent = verifyResources(approvedInstallPlan, true)
+	// Result: Ensure that the InstallPlan actually creates Vault resources
+	t.Logf("%d Vault Resources present", vaultResourcesPresent)
+	require.NotZero(t, vaultResourcesPresent)
+
 }
 
 // This captures the current state of OLM where Failed InstallPlans aren't implemented and should be removed in the future