From 03eb5e35c1f537a1331b223bc4ea9126ededc460 Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Tue, 16 Oct 2018 09:39:56 -0400 Subject: [PATCH 1/2] Refactor to level driven design Refactor the handler to be level driven. --- cmd/cluster-ingress-operator/main.go | 4 +- pkg/stub/handler.go | 132 ++++++++++++++++++++++----- 2 files changed, 112 insertions(+), 24 deletions(-) diff --git a/cmd/cluster-ingress-operator/main.go b/cmd/cluster-ingress-operator/main.go index e5ce93f6db..725c68630f 100644 --- a/cmd/cluster-ingress-operator/main.go +++ b/cmd/cluster-ingress-operator/main.go @@ -10,6 +10,8 @@ import ( k8sutil "github.com/operator-framework/operator-sdk/pkg/util/k8sutil" sdkVersion "github.com/operator-framework/operator-sdk/version" + "github.com/openshift/cluster-ingress-operator/pkg/manifests" + "github.com/sirupsen/logrus" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" ) @@ -31,7 +33,7 @@ func main() { if err != nil { logrus.Fatalf("Failed to get watch namespace: %v", err) } - handler := stub.NewHandler() + handler := stub.NewHandler(namespace, manifests.NewFactory()) if err := handler.EnsureDefaultClusterIngress(); err != nil { logrus.Fatalf("Ensuring default cluster ingress: %v", err) } diff --git a/pkg/stub/handler.go b/pkg/stub/handler.go index 4ec32443cc..e2bc589379 100644 --- a/pkg/stub/handler.go +++ b/pkg/stub/handler.go @@ -14,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilerrors "k8s.io/apimachinery/pkg/util/errors" ) const ( @@ -24,27 +25,26 @@ const ( clusterConfigResource = "cluster-config-v1" ) -func NewHandler() *Handler { +func NewHandler(namespace string, manifestFactory *manifests.Factory) *Handler { return &Handler{ - manifestFactory: manifests.NewFactory(), + namespace: namespace, + manifestFactory: manifestFactory, } } type Handler struct { + namespace string manifestFactory *manifests.Factory } func (h *Handler) Handle(ctx context.Context, event sdk.Event) error { + // TODO: This should be adding an item to a rate limited work queue, but for + // now correctness is more important than performance. switch o := event.Object.(type) { case *ingressv1alpha1.ClusterIngress: - if event.Deleted { - logrus.Infof("Deleting ClusterIngress object: %s", o.Name) - return h.deleteIngress(o) - } else { - return h.syncIngressUpdate(o) - } + logrus.Infof("reconciling for update to clusteringress %q", o.Name) } - return nil + return h.reconcile() } // EnsureDefaultClusterIngress ensures that a default ClusterIngress exists. @@ -74,7 +74,74 @@ func (h *Handler) EnsureDefaultClusterIngress() error { return fmt.Errorf("creating default cluster ingress %s/%s: %v", ci.Namespace, ci.Name, err) } -func (h *Handler) syncIngressUpdate(ci *ingressv1alpha1.ClusterIngress) error { +// Reconcile performs a full reconciliation loop for ingress, including +// generalized setup and handling of all clusteringress resources in the +// operator namespace. +func (h *Handler) reconcile() error { + // Ensure we have all the necessary scaffolding on which to place router + // instances. + err := h.ensureRouterNamespace() + if err != nil { + return err + } + + // Find all clusteringresses. + ingresses := &ingressv1alpha1.ClusterIngressList{ + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterIngress", + APIVersion: "ingress.openshift.io/v1alpha1", + }, + } + err = sdk.List(h.namespace, ingresses, sdk.WithListOptions(&metav1.ListOptions{})) + if err != nil { + return fmt.Errorf("failed to list clusteringresses: %v", err) + } + + // Reconcile all the ingresses. + errors := []error{} + for _, ingress := range ingresses.Items { + // Handle deleted ingress. + // TODO: Assert/ensure that the ingress has a finalizer so we can reliably detect + // deletion. + if ingress.DeletionTimestamp != nil { + // Destroy any router associated with the clusteringress. + err := h.ensureRouterDeleted(&ingress) + if err != nil { + errors = append(errors, fmt.Errorf("couldn't delete clusteringress %q: %v", ingress.Name, err)) + continue + } + // Clean up the finalizer to allow the clusteringress to be deleted. + ingress.Finalizers = RemoveString(ingress.Finalizers, "ingress.openshift.io/default-cluster-ingress") + err = sdk.Update(&ingress) + if err != nil { + errors = append(errors, fmt.Errorf("couldn't remove finalizer from clusteringress %q: %v", ingress.Name, err)) + } + continue + } + + // Handle active ingress. + err := h.ensureRouterForIngress(&ingress) + if err != nil { + errors = append(errors, fmt.Errorf("couldn't ensure clusteringress %q: %v", ingress.Name, err)) + } + } + return utilerrors.NewAggregate(errors) +} + +// ensureRouterNamespace ensures all the necessary scaffolding exists for +// routers generally, including a namespace and all RBAC setup. +func (h *Handler) ensureRouterNamespace() error { + cr, err := h.manifestFactory.RouterClusterRole() + if err != nil { + return fmt.Errorf("couldn't build router cluster role: %v", err) + } + err = sdk.Create(cr) + if err == nil { + logrus.Infof("created router cluster role %q", cr.Name) + } else if !errors.IsAlreadyExists(err) { + return fmt.Errorf("couldn't create router cluster role: %v", err) + } + ns, err := h.manifestFactory.RouterNamespace() if err != nil { return fmt.Errorf("couldn't build router namespace: %v", err) @@ -97,17 +164,6 @@ func (h *Handler) syncIngressUpdate(ci *ingressv1alpha1.ClusterIngress) error { return fmt.Errorf("couldn't create router service account %s/%s: %v", sa.Namespace, sa.Name, err) } - cr, err := h.manifestFactory.RouterClusterRole() - if err != nil { - return fmt.Errorf("couldn't build router cluster role: %v", err) - } - err = sdk.Create(cr) - if err == nil { - logrus.Infof("created router cluster role %q", cr.Name) - } else if !errors.IsAlreadyExists(err) { - return fmt.Errorf("couldn't create router cluster role: %v", err) - } - crb, err := h.manifestFactory.RouterClusterRoleBinding() if err != nil { return fmt.Errorf("couldn't build router cluster role binding: %v", err) @@ -119,6 +175,12 @@ func (h *Handler) syncIngressUpdate(ci *ingressv1alpha1.ClusterIngress) error { return fmt.Errorf("couldn't create router cluster role binding: %v", err) } + return nil +} + +// ensureRouterForIngress ensures all necessary router resources exist for a +// given clusteringress. +func (h *Handler) ensureRouterForIngress(ci *ingressv1alpha1.ClusterIngress) error { ds, err := h.manifestFactory.RouterDaemonSet(ci) if err != nil { return fmt.Errorf("couldn't build daemonset: %v", err) @@ -163,10 +225,34 @@ func (h *Handler) syncIngressUpdate(ci *ingressv1alpha1.ClusterIngress) error { return nil } -func (h *Handler) deleteIngress(ci *ingressv1alpha1.ClusterIngress) error { +// ensureRouterDeleted ensures that any router resources associated with the +// clusteringress are deleted. +func (h *Handler) ensureRouterDeleted(ci *ingressv1alpha1.ClusterIngress) error { ds, err := h.manifestFactory.RouterDaemonSet(ci) if err != nil { return fmt.Errorf("couldn't build DaemonSet object for deletion: %v", err) } - return sdk.Delete(ds) + err = sdk.Delete(ds) + if !errors.IsNotFound(err) { + return err + } + return nil +} + +// RemoveString returns a newly created []string that contains all items from slice that +// are not equal to s. +func RemoveString(slice []string, s string) []string { + newSlice := make([]string, 0) + for _, item := range slice { + if item == s { + continue + } + newSlice = append(newSlice, item) + } + if len(newSlice) == 0 { + // Sanitize for unit tests so we don't need to distinguish empty array + // and nil. + newSlice = nil + } + return newSlice } From a447eaabd281dda14d4c1b68dc81409467b2dde9 Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Tue, 16 Oct 2018 13:59:07 -0400 Subject: [PATCH 2/2] Improve reentrancy for deletes --- pkg/stub/handler.go | 35 +++++++++++++---------------------- pkg/util/slice/slice.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 22 deletions(-) create mode 100644 pkg/util/slice/slice.go diff --git a/pkg/stub/handler.go b/pkg/stub/handler.go index e2bc589379..5e0fbdb3a0 100644 --- a/pkg/stub/handler.go +++ b/pkg/stub/handler.go @@ -8,6 +8,7 @@ import ( ingressv1alpha1 "github.com/openshift/cluster-ingress-operator/pkg/apis/ingress/v1alpha1" "github.com/openshift/cluster-ingress-operator/pkg/manifests" + "github.com/openshift/cluster-ingress-operator/pkg/util/slice" "github.com/operator-framework/operator-sdk/pkg/k8sclient" "github.com/operator-framework/operator-sdk/pkg/sdk" @@ -23,6 +24,12 @@ const ( // clusterConfigResource is the resource containing the installer config. clusterConfigResource = "cluster-config-v1" + + // ClusterIngressFinalizer is applied to all ClusterIngress resources before + // they are considered for processing; this ensures the operator has a chance + // to handle all states. + // TODO: Make this generic and not tied to the "default" ingress. + ClusterIngressFinalizer = "ingress.openshift.io/default-cluster-ingress" ) func NewHandler(namespace string, manifestFactory *manifests.Factory) *Handler { @@ -111,10 +118,12 @@ func (h *Handler) reconcile() error { continue } // Clean up the finalizer to allow the clusteringress to be deleted. - ingress.Finalizers = RemoveString(ingress.Finalizers, "ingress.openshift.io/default-cluster-ingress") - err = sdk.Update(&ingress) - if err != nil { - errors = append(errors, fmt.Errorf("couldn't remove finalizer from clusteringress %q: %v", ingress.Name, err)) + if slice.ContainsString(ingress.Finalizers, ClusterIngressFinalizer) { + ingress.Finalizers = slice.RemoveString(ingress.Finalizers, ClusterIngressFinalizer) + err = sdk.Update(&ingress) + if err != nil { + errors = append(errors, fmt.Errorf("couldn't remove finalizer from clusteringress %q: %v", ingress.Name, err)) + } } continue } @@ -238,21 +247,3 @@ func (h *Handler) ensureRouterDeleted(ci *ingressv1alpha1.ClusterIngress) error } return nil } - -// RemoveString returns a newly created []string that contains all items from slice that -// are not equal to s. -func RemoveString(slice []string, s string) []string { - newSlice := make([]string, 0) - for _, item := range slice { - if item == s { - continue - } - newSlice = append(newSlice, item) - } - if len(newSlice) == 0 { - // Sanitize for unit tests so we don't need to distinguish empty array - // and nil. - newSlice = nil - } - return newSlice -} diff --git a/pkg/util/slice/slice.go b/pkg/util/slice/slice.go new file mode 100644 index 0000000000..9b254428bd --- /dev/null +++ b/pkg/util/slice/slice.go @@ -0,0 +1,29 @@ +package slice + +// RemoveString returns a newly created []string that contains all items from slice that +// are not equal to s. +func RemoveString(slice []string, s string) []string { + newSlice := make([]string, 0) + for _, item := range slice { + if item == s { + continue + } + newSlice = append(newSlice, item) + } + if len(newSlice) == 0 { + // Sanitize for unit tests so we don't need to distinguish empty array + // and nil. + newSlice = nil + } + return newSlice +} + +// ContainsString checks if a given slice of strings contains the provided string. +func ContainsString(slice []string, s string) bool { + for _, item := range slice { + if item == s { + return true + } + } + return false +}