🤖 Sync from open-cluster-management-io/config-policy-controller: #242, #239

.vscode/launch.json

@@ -29,6 +29,23 @@
  "KUBECONFIG": "${workspaceFolder}/kubeconfig_managed_e2e",
  }
  },
+ // Set FDescribe or FIt on the test to debug. Then set the desired breakpoint.
+ {
+ "name": "Launch Hosted Test Function",
+ "type": "go",
+ "request": "launch",
+ "mode": "auto",
+ "program": "${workspaceFolder}/test/e2e/e2e_suite_test.go",
+ "args": [
+ "-ginkgo.debug",
+ "-ginkgo.v",
+ "-is_hosted=true"
+ ],
+ "env": {
+ "KUBECONFIG": "${workspaceFolder}/kubeconfig_managed_e2e",
+ "TARGET_KUBECONFIG_PATH": "${workspaceFolder}/kubeconfig_managed2_e2e",
+ }
+ },
  // Set the correct path to the governance-policy-framework repo directory in the env section.
  {
  "name": "Launch Package (Framework E2E) (instructions in launch.json)",
@@ -45,4 +62,4 @@
  }
  }
  ]
-}
+}

Makefile

@@ -106,7 +106,7 @@ create-ns:
 # Run against the current locally configured Kubernetes cluster
 .PHONY: run
 run:
- WATCH_NAMESPACE=$(WATCH_NAMESPACE) go run ./main.go controller --leader-elect=false
+ WATCH_NAMESPACE=$(WATCH_NAMESPACE) go run ./main.go controller --leader-elect=false --enable-operator-policy=true
 
 ############################################################
 # clean section
@@ -193,7 +193,7 @@ kind-create-cluster:
 .PHONY: kind-additional-cluster
 kind-additional-cluster: MANAGED_CLUSTER_SUFFIX = 2
 kind-additional-cluster: CLUSTER_NAME = $(MANAGED_CLUSTER_NAME)
-kind-additional-cluster: kind-create-cluster kind-controller-kubeconfig
+kind-additional-cluster: kind-create-cluster kind-controller-kubeconfig install-crds-hosted
 
 .PHONY: kind-delete-cluster
 kind-delete-cluster:
@@ -204,11 +204,10 @@ kind-delete-cluster:
 kind-tests: kind-delete-cluster kind-bootstrap-cluster-dev kind-deploy-controller-dev e2e-test
 
 OLM_VERSION := v0.26.0
+OLM_INSTALLER = $(LOCAL_BIN)/install.sh
 
 .PHONY: install-crds
-install-crds:
- @echo installing olm
- curl -L https://github.com/operator-framework/operator-lifecycle-manager/releases/download/$(OLM_VERSION)/install.sh -o $(LOCAL_BIN)/install.sh
+install-crds: $(OLM_INSTALLER)
  chmod +x $(LOCAL_BIN)/install.sh
  $(LOCAL_BIN)/install.sh $(OLM_VERSION)
  @echo installing crds
@@ -221,6 +220,15 @@ install-crds:
  kubectl apply -f deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml
  kubectl apply -f deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml
 
+$(OLM_INSTALLER):
+ @echo getting OLM installer
+ curl -L https://github.com/operator-framework/operator-lifecycle-manager/releases/download/$(OLM_VERSION)/install.sh -o $(LOCAL_BIN)/install.sh
+
+install-crds-hosted: export KUBECONFIG=./kubeconfig_managed$(MANAGED_CLUSTER_SUFFIX)_e2e
+install-crds-hosted: $(OLM_INSTALLER)
+ chmod +x $(LOCAL_BIN)/install.sh
+ $(LOCAL_BIN)/install.sh $(OLM_VERSION)
+
 .PHONY: install-resources
 install-resources:
  # creating namespaces
@@ -230,17 +238,22 @@ install-resources:
  kubectl apply -k deploy/rbac
  kubectl apply -f deploy/manager/service-account.yaml -n $(KIND_NAMESPACE)
 
+IS_HOSTED ?= false
+E2E_PROCS = 20
+
 .PHONY: e2e-test
 e2e-test: e2e-dependencies
- $(GINKGO) -v --procs=20 $(E2E_TEST_ARGS) test/e2e
+ $(GINKGO) -v --procs=$(E2E_PROCS) $(E2E_TEST_ARGS) test/e2e -- -is_hosted=$(IS_HOSTED)
 
 .PHONY: e2e-test-coverage
 e2e-test-coverage: E2E_TEST_ARGS = --json-report=report_e2e.json --label-filter='!hosted-mode && !running-in-cluster' --output-dir=.
 e2e-test-coverage: e2e-run-instrumented e2e-test e2e-stop-instrumented
 
 .PHONY: e2e-test-hosted-mode-coverage
-e2e-test-hosted-mode-coverage: E2E_TEST_ARGS = --json-report=report_e2e_hosted_mode.json --label-filter="hosted-mode && !running-in-cluster" --output-dir=.
+e2e-test-hosted-mode-coverage: E2E_TEST_ARGS = --json-report=report_e2e_hosted_mode.json --label-filter="hosted-mode || supports-hosted && !running-in-cluster" --output-dir=.
 e2e-test-hosted-mode-coverage: COVERAGE_E2E_OUT = coverage_e2e_hosted_mode.out
+e2e-test-hosted-mode-coverage: IS_HOSTED=true
+e2e-test-hosted-mode-coverage: E2E_PROCS=2
 e2e-test-hosted-mode-coverage: export TARGET_KUBECONFIG_PATH = $(PWD)/kubeconfig_managed2
 e2e-test-hosted-mode-coverage: e2e-run-instrumented e2e-test e2e-stop-instrumented
 

controllers/operatorpolicy_controller.go

@@ -103,6 +103,7 @@ type OperatorPolicyReconciler struct {
  DynamicWatcher depclient.DynamicWatcher
  InstanceName string
  DefaultNamespace string
+ TargetClient client.Client
 }
 
 // SetupWithManager sets up the controller with the Manager and will reconcile when the dynamic watcher
@@ -750,7 +751,7 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
 
  desiredOpGroup.ResourceVersion = opGroup.GetResourceVersion()
 
- err = r.Update(ctx, &opGroup)
+ err = r.TargetClient.Update(ctx, &opGroup)
  if err != nil {
  return false, nil, changed, fmt.Errorf("error updating the OperatorGroup: %w", err)
  }
@@ -771,7 +772,7 @@ func (r *OperatorPolicyReconciler) musthaveOpGroup(
 func (r *OperatorPolicyReconciler) createWithNamespace(
  ctx context.Context, policy *policyv1beta1.OperatorPolicy, object client.Object,
 ) error {
- err := r.Create(ctx, object)
+ err := r.TargetClient.Create(ctx, object)
  if err == nil {
  return nil
  }
@@ -789,13 +790,13 @@ func (r *OperatorPolicyReconciler) createWithNamespace(
  },
  }
 
- err = r.Create(ctx, &ns)
+ err = r.TargetClient.Create(ctx, &ns)
  if err != nil && !k8serrors.IsAlreadyExists(err) {
  return err
  }
 
  // Try creating the object again now that the namespace was created.
- return r.Create(ctx, object)
+ return r.TargetClient.Create(ctx, object)
 }
 
 // isNamespaceNotFound detects if the input error from r.Create failed due to the specified namespace not existing.
@@ -904,7 +905,7 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
  earlyConds = append(earlyConds, calculateComplianceCondition(policy))
  }
 
- err := r.Delete(ctx, desiredOpGroup)
+ err := r.TargetClient.Delete(ctx, desiredOpGroup)
  if err != nil {
  return earlyConds, changed, fmt.Errorf("error deleting the OperatorGroup: %w", err)
  }
@@ -1046,7 +1047,7 @@ func (r *OperatorPolicyReconciler) musthaveSubscription(
  earlyConds = append(earlyConds, calculateComplianceCondition(policy))
  }
 
- err = r.Update(ctx, foundSub)
+ err = r.TargetClient.Update(ctx, foundSub)
  if err != nil {
  return mergedSub, nil, changed, fmt.Errorf("error updating the Subscription: %w", err)
  }
@@ -1100,7 +1101,7 @@ func (r *OperatorPolicyReconciler) mustnothaveSubscription(
  earlyConds = append(earlyConds, calculateComplianceCondition(policy))
  }
 
- err := r.Delete(ctx, foundUnstructSub)
+ err := r.TargetClient.Delete(ctx, foundUnstructSub)
  if err != nil {
  return foundSub, earlyConds, changed, fmt.Errorf("error deleting the Subscription: %w", err)
  }
@@ -1175,50 +1176,27 @@ func (r *OperatorPolicyReconciler) handleInstallPlan(
  }
 
  watcher := opPolIdentifier(policy.Namespace, policy.Name)
+ selector := subLabelSelector(sub)
 
- foundInstallPlans, err := r.DynamicWatcher.List(
- watcher, installPlanGVK, sub.Namespace, labels.Everything())
+ installPlans, err := r.DynamicWatcher.List(watcher, installPlanGVK, sub.Namespace, selector)
  if err != nil {
  return false, fmt.Errorf("error listing InstallPlans: %w", err)
  }
 
- ownedInstallPlans := make([]unstructured.Unstructured, 0, len(foundInstallPlans))
- selector := subLabelSelector(sub)
-
- for _, installPlan := range foundInstallPlans {
- // sometimes the OwnerReferences aren't correct, but the label should be
- if selector.Matches(labels.Set(installPlan.GetLabels())) {
- ownedInstallPlans = append(ownedInstallPlans, installPlan)
-
- break
- }
-
- for _, owner := range installPlan.GetOwnerReferences() {
- match := owner.Name == sub.Name &&
- owner.Kind == subscriptionGVK.Kind &&
- owner.APIVersion == subscriptionGVK.GroupVersion().String()
- if match {
- ownedInstallPlans = append(ownedInstallPlans, installPlan)
-
- break
- }
- }
- }
-
  // InstallPlans are generally kept in order to provide a history of actions on the cluster, but
  // they can be deleted without impacting the installed operator. So, not finding any should not
  // be considered a reason for NonCompliance, regardless of musthave or mustnothave.
- if len(ownedInstallPlans) == 0 {
+ if len(installPlans) == 0 {
  return updateStatus(policy, noInstallPlansCond, noInstallPlansObj(sub.Namespace)), nil
  }
 
  if policy.Spec.ComplianceType.IsMustHave() {
- changed, err := r.musthaveInstallPlan(ctx, policy, sub, ownedInstallPlans)
+ changed, err := r.musthaveInstallPlan(ctx, policy, sub, installPlans)
 
  return changed, err
  }
 
- return r.mustnothaveInstallPlan(policy, ownedInstallPlans)
+ return r.mustnothaveInstallPlan(policy, installPlans)
 }
 
 func (r *OperatorPolicyReconciler) musthaveInstallPlan(
@@ -1232,7 +1210,6 @@ func (r *OperatorPolicyReconciler) musthaveInstallPlan(
  ipsRequiringApproval := make([]unstructured.Unstructured, 0)
  anyInstalling := false
  currentPlanFailed := false
- selector := subLabelSelector(sub)
 
  // Construct the relevant relatedObjects, and collect any that might be considered for approval
  for i, installPlan := range ownedInstallPlans {
@@ -1252,11 +1229,7 @@ func (r *OperatorPolicyReconciler) musthaveInstallPlan(
  // consider some special phases
  switch phase {
  case string(operatorv1alpha1.InstallPlanPhaseRequiresApproval):
- // only consider InstallPlans with this label for approval - this label is supposed to
- // indicate the "current" InstallPlan for this subscription.
- if selector.Matches(labels.Set(installPlan.GetLabels())) {
- ipsRequiringApproval = append(ipsRequiringApproval, installPlan)
- }
+ ipsRequiringApproval = append(ipsRequiringApproval, installPlan)
  case string(operatorv1alpha1.InstallPlanPhaseInstalling):
  anyInstalling = true
  case string(operatorv1alpha1.InstallPlanFailed):
@@ -1364,7 +1337,7 @@ func (r *OperatorPolicyReconciler) musthaveInstallPlan(
  return false, fmt.Errorf("error approving InstallPlan: %w", err)
  }
 
- if err := r.Update(ctx, &approvableInstallPlans[0]); err != nil {
+ if err := r.TargetClient.Update(ctx, &approvableInstallPlans[0]); err != nil {
  return false, fmt.Errorf("error updating approved InstallPlan: %w", err)
  }
 
@@ -1502,7 +1475,7 @@ func (r *OperatorPolicyReconciler) mustnothaveCSV(
  continue
  }
 
- err := r.Delete(ctx, &csvList[i])
+ err := r.TargetClient.Delete(ctx, &csvList[i])
  if err != nil {
  changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion", csvNames...), relatedCSVs...)
 
@@ -1672,7 +1645,7 @@ func (r *OperatorPolicyReconciler) handleCRDs(
  continue
  }
 
- err := r.Delete(ctx, &crdList[i])
+ err := r.TargetClient.Delete(ctx, &crdList[i])
  if err != nil {
  changed := updateStatus(policy, foundNotWantedCond("CustomResourceDefinition"), relatedCRDs...)
 
@@ -1835,7 +1808,7 @@ func (r *OperatorPolicyReconciler) mergeObjects(
  }
 
  if updateNeeded {
- err := r.Update(ctx, existing, client.DryRunAll)
+ err := r.TargetClient.Update(ctx, existing, client.DryRunAll)
  if err != nil {
  if k8serrors.IsForbidden(err) {
  // This indicates the update would make a change, but the change is not allowed,

controllers/operatorpolicy_status.go

@@ -1042,9 +1042,8 @@ func noInstallPlansObj(namespace string) policyv1.RelatedObject {
 }
 
 // existingInstallPlanObj returns a RelatedObject for the InstallPlan, with a reason
-// like 'The InstallPlan is ____' based on the phase. Usually the object will not
-// have a compliance associated with it, but if it requires approval or is actively
-// installing, then it will be NonCompliant.
+// like 'The InstallPlan is ____' based on the phase. When the InstallPlan is in phase
+// 'Complete', the object will be Compliant, otherwise it will be NonCompliant.
 func existingInstallPlanObj(ip client.Object, phase string) policyv1.RelatedObject {
  relObj := policyv1.RelatedObject{
  Object: policyv1.ObjectResourceFromObj(ip),
@@ -1064,8 +1063,9 @@ func existingInstallPlanObj(ip client.Object, phase string) policyv1.RelatedObje
  // FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`.
  // For now, assume it is set to 'NonCompliant'
  relObj.Compliant = string(policyv1.NonCompliant)
- case string(operatorv1alpha1.InstallPlanPhaseInstalling):
- // if it's still installing, then it shouldn't be considered compliant yet.
+ case string(operatorv1alpha1.InstallPlanPhaseComplete):
+ relObj.Compliant = string(policyv1.Compliant)
+ default:
  relObj.Compliant = string(policyv1.NonCompliant)
  }
 

main.go

@@ -322,13 +322,15 @@ func main() {
  var targetK8sClient kubernetes.Interface
  var targetK8sDynamicClient dynamic.Interface
  var targetK8sConfig *rest.Config
+ var targetClient client.Client
  var nsSelMgr manager.Manager // A separate controller-manager is needed in hosted mode
 
  if opts.targetKubeConfig == "" {
  targetK8sConfig = cfg
  targetK8sClient = kubernetes.NewForConfigOrDie(targetK8sConfig)
  targetK8sDynamicClient = dynamic.NewForConfigOrDie(targetK8sConfig)
  nsSelMgr = mgr
+ targetClient = mgr.GetClient()
  } else { // "Hosted mode"
  var err error
 
@@ -343,6 +345,11 @@ func main() {
 
  targetK8sClient = kubernetes.NewForConfigOrDie(targetK8sConfig)
  targetK8sDynamicClient = dynamic.NewForConfigOrDie(targetK8sConfig)
+ targetClient, err = client.New(targetK8sConfig, client.Options{Scheme: scheme})
+ if err != nil {
+ log.Error(err, "Failed to load the target kubeconfig", "path", opts.targetKubeConfig)
+ os.Exit(1)
+ }
 
  // The managed cluster's API server is potentially not the same as the hosting cluster and it could be
  // offline already as part of the uninstall process. In this case, the manager's instantiation will fail.
@@ -428,7 +435,7 @@ func main() {
  if opts.enableOperatorPolicy {
  depReconciler, depEvents := depclient.NewControllerRuntimeSource()
 
- watcher, err := depclient.New(cfg, depReconciler,
+ watcher, err := depclient.New(targetK8sConfig, depReconciler,
  &depclient.Options{
  DisableInitialReconcile: true,
  EnableCache: true,
@@ -455,6 +462,7 @@ func main() {
  DynamicWatcher: watcher,
  InstanceName: instanceName,
  DefaultNamespace: opts.operatorPolDefaultNS,
+ TargetClient: targetClient,
  }
 
  if err = OpReconciler.SetupWithManager(mgr, depEvents); err != nil {

test/e2e/case21_alternative_kubeconfig_test.go

@@ -5,43 +5,25 @@ package e2e
 import (
  "context"
  "fmt"
- "os"
 
  . "github.com/onsi/ginkgo/v2"
  . "github.com/onsi/gomega"
  v1 "k8s.io/api/core/v1"
  "k8s.io/apimachinery/pkg/api/errors"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
- "k8s.io/client-go/kubernetes"
- "k8s.io/client-go/tools/clientcmd"
 
  "open-cluster-management.io/config-policy-controller/test/utils"
 )
 
 var _ = Describe("Test an alternative kubeconfig for policy evaluation", Ordered, Label("hosted-mode"), func() {
  const (
- envName = "TARGET_KUBECONFIG_PATH"
  namespaceName = "e2e-test-ns"
  policyName = "create-ns"
  policyYAML = "../resources/case21_alternative_kubeconfig/policy.yaml"
  parentPolicyName = "parent-create-ns"
  parentPolicyYAML = "../resources/case21_alternative_kubeconfig/parent-policy.yaml"
  )
 
- var targetK8sClient *kubernetes.Clientset
-
- BeforeAll(func() {
- By("Checking that the " + envName + " environment variable is valid")
- altKubeconfigPath := os.Getenv(envName)
- Expect(altKubeconfigPath).ToNot(Equal(""))
-
- targetK8sConfig, err := clientcmd.BuildConfigFromFlags("", altKubeconfigPath)
- Expect(err).ToNot(HaveOccurred())
-
- targetK8sClient, err = kubernetes.NewForConfig(targetK8sConfig)
- Expect(err).ToNot(HaveOccurred())
- })
-
  AfterAll(func() {
  deleteConfigPolicies([]string{policyName})