Skip to content

Commit

Permalink
Don't add clientca-configmap finalizer if deleting
Browse files Browse the repository at this point in the history 
Don't add the clientca-configmap finalizer to an IngressController if it is
marked for deletion.

Before this commit, the clientca-configmap controller would attempt to
add (or re-add) its finalizer to an IngressController that had been marked
for deletion.  This attempt resulted in the following error from the API:

    IngressController.operator.openshift.io "test-client-ca-configmap" is invalid: metadata.finalizers: Forbidden: no new finalizers can be added if the object is being deleted, found new finalizers []string{"ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"}

The controller would retry adding the finalizer repeatedly, causing the
same error to be repeatedly logged until the deletion completed.

This commit fixes OCPBUGS-14994.

https://issues.redhat.com/browse/OCPBUGS-14994

* pkg/operator/controller/clientca-configmap/controller.go (Reconcile):
Only attempt to add the finalizer if the IngressController is not marked
for deletion.
* pkg/operator/controller/clientca-configmap/controller_test.go: New file.
(Test_Reconcile): New test.  Verify that Reconcile adds and removes the
finalizer and creates, updates, or deletes the target configmap as
appropriate.
(fakeClientRecorder): New type, used in Test_Reconcile.
(Create, Delete, Update): New methods for fakeClientRecorder to implement
the controller-runtime client.Client interface.
  • Loading branch information
@Miciah
Miciah committed Jun 22, 2023
1 parent 0e500e6 commit b3ea675
Show file tree
Hide file tree
Showing 2 changed files with 305 additions and 1 deletion.
2 changes: 1 addition & 1 deletion pkg/operator/controller/clientca-configmap/controller.go
View file
Expand Up @@ -191,7 +191,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
}

const finalizer = "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"
if len(ic.Spec.ClientTLS.ClientCA.Name) != 0 && !slice.ContainsString(ic.Finalizers, finalizer) {
if len(ic.Spec.ClientTLS.ClientCA.Name) != 0 && ic.DeletionTimestamp == nil && !slice.ContainsString(ic.Finalizers, finalizer) {
// Ensure the ingresscontroller has a finalizer so we get a
// chance to delete the configmap when the ingresscontroller is
// deleted.
Expand Down
304 changes: 304 additions & 0 deletions pkg/operator/controller/clientca-configmap/controller_test.go
View file
@@ -0,0 +1,304 @@
package clientcaconfigmap

import (
"context"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/stretchr/testify/assert"

"github.com/openshift/cluster-ingress-operator/pkg/manifests"

configv1 "github.com/openshift/api/config/v1"
operatorv1 "github.com/openshift/api/operator/v1"

corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"

"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

// Test_Reconcile verifies that the controller's Reconcile method behaves
// correctly.
func Test_Reconcile(t *testing.T) {
exampleTime, err := time.Parse(time.DateTime, "2006-01-02 15:04:05")
if err != nil {
t.Fatal(err)
}
ic := func(deleted bool, configmapName string, additionalFinalizers ...string) *operatorv1.IngressController {
var ts *metav1.Time
if deleted {
ts = &metav1.Time{Time: exampleTime}
}
return &operatorv1.IngressController{
ObjectMeta: metav1.ObjectMeta{
DeletionTimestamp: ts,
Finalizers: append([]string{manifests.IngressControllerFinalizer}, additionalFinalizers...),
Namespace: "openshift-ingress-operator",
Name: "test",
},
Spec: operatorv1.IngressControllerSpec{
ClientTLS: operatorv1.ClientTLS{
ClientCA: configv1.ConfigMapNameReference{
Name: configmapName,
},
},
},
}
}
cm := func(namespace, name string, data map[string]string) *corev1.ConfigMap {
return &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: name,
},
Data: data,
}
}
var (
expectedData = map[string]string{
"ca-bundle.pem": "certificate",
}
unexpectedData = map[string]string{
"garbage": "garbage",
}
)
tests := []struct {
name string
existingObjects []runtime.Object
expectCreate []client.Object
expectUpdate []client.Object
expectDelete []client.Object
}{
{
name: "ingresscontroller absent",
existingObjects: []runtime.Object{},
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
},
{
name: "no configmap specified",
existingObjects: []runtime.Object{
ic(false, ""),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
},
{
name: "source configmap absent, target configmap absent, finalizer absent",
existingObjects: []runtime.Object{
ic(false, "ca-bundle"),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{
ic(false, "ca-bundle", "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"),
},
expectDelete: []client.Object{},
},
{
name: "source configmap absent, target configmap absent, finalizer present",
existingObjects: []runtime.Object{
ic(false, "ca-bundle", "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
},
{
name: "source configmap absent, target configmap present, finalizer absent",
existingObjects: []runtime.Object{
ic(false, "ca-bundle"),
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{
ic(false, "ca-bundle", "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"),
},
expectDelete: []client.Object{
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
},
{
name: "source configmap absent, target configmap present, finalizer present",
existingObjects: []runtime.Object{
ic(false, "ca-bundle", "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"),
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
},
{
name: "source configmap present, target configmap absent, finalizer absent",
existingObjects: []runtime.Object{
ic(false, "ca-bundle"),
cm("openshift-config", "ca-bundle", expectedData),
},
expectCreate: []client.Object{
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
expectUpdate: []client.Object{
ic(false, "ca-bundle", "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"),
},
expectDelete: []client.Object{},
},
{
name: "source configmap present, target configmap absent, finalizer present",
existingObjects: []runtime.Object{
ic(false, "ca-bundle", "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"),
cm("openshift-config", "ca-bundle", expectedData),
},
expectCreate: []client.Object{
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
},
{
name: "source configmap present, target configmap present, finalizer absent",
existingObjects: []runtime.Object{
ic(false, "ca-bundle"),
cm("openshift-config", "ca-bundle", expectedData),
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{
ic(false, "ca-bundle", "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"),
},
expectDelete: []client.Object{},
},
{
name: "source configmap present, target configmap present but inconsistent, finalizer present",
existingObjects: []runtime.Object{
ic(false, "ca-bundle", "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"),
cm("openshift-config", "ca-bundle", expectedData),
cm("openshift-ingress", "router-client-ca-test", unexpectedData),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
expectDelete: []client.Object{},
},
{
name: "source configmap present, target configmap absent, finalizer absent, ingresscontroller deleted",
existingObjects: []runtime.Object{
ic(true, "ca-bundle"),
cm("openshift-config", "ca-bundle", expectedData),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
},
{
name: "source configmap present, target configmap present, finalizer absent, ingresscontroller deleted",
existingObjects: []runtime.Object{
ic(true, "ca-bundle"),
cm("openshift-config", "ca-bundle", expectedData),
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{
cm("openshift-ingress", "router-client-ca-test", expectedData),
},
},
{
name: "source configmap present, target configmap present but inconsistent, finalizer present, ingresscontroller deleted",
existingObjects: []runtime.Object{
ic(true, "ca-bundle", "ingresscontroller.operator.openshift.io/finalizer-clientca-configmap"),
cm("openshift-config", "ca-bundle", expectedData),
cm("openshift-ingress", "router-client-ca-test", unexpectedData),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{
ic(true, "ca-bundle"),
},
expectDelete: []client.Object{
cm("openshift-ingress", "router-client-ca-test", unexpectedData),
},
},
}

scheme := runtime.NewScheme()
operatorv1.Install(scheme)
corev1.AddToScheme(scheme)

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
fakeClient := fake.NewClientBuilder().
WithScheme(scheme).
WithRuntimeObjects(tc.existingObjects...).
Build()
cl := &fakeClientRecorder{fakeClient, t, []client.Object{}, []client.Object{}, []client.Object{}}
reconciler := &reconciler{
client: cl,
config: Config{
OperatorNamespace: "openshift-ingress-operator",
SourceNamespace: "openshift-config",
TargetNamespace: "openshift-ingress",
},
}
req := reconcile.Request{
NamespacedName: types.NamespacedName{
Namespace: "openshift-ingress-operator",
Name: "test",
},
}
res, err := reconciler.Reconcile(context.Background(), req)
assert.NoError(t, err)
assert.Equal(t, reconcile.Result{}, res)
cmpOpts := []cmp.Option{
cmpopts.EquateEmpty(),
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "Annotations", "ResourceVersion"),
cmpopts.IgnoreFields(metav1.TypeMeta{}, "Kind", "APIVersion"),
cmpopts.IgnoreFields(apiextensionsv1.CustomResourceDefinition{}, "Spec"),
}
if diff := cmp.Diff(tc.expectCreate, cl.added, cmpOpts...); diff != "" {
t.Fatalf("found diff between expected and actual creates: %s", diff)
}
if diff := cmp.Diff(tc.expectUpdate, cl.updated, cmpOpts...); diff != "" {
t.Fatalf("found diff between expected and actual updates: %s", diff)
}
if diff := cmp.Diff(tc.expectDelete, cl.deleted, cmpOpts...); diff != "" {
t.Fatalf("found diff between expected and actual deletes: %s", diff)
}
})
}
}

// fakeClientRecorder is a fake client that records adds, updates, and deletes.
type fakeClientRecorder struct {
client.Client
*testing.T

added []client.Object
updated []client.Object
deleted []client.Object
}

func (c *fakeClientRecorder) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
c.added = append(c.added, obj)
return c.Client.Create(ctx, obj, opts...)
}

func (c *fakeClientRecorder) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
c.deleted = append(c.deleted, obj)
return c.Client.Delete(ctx, obj, opts...)
}

func (c *fakeClientRecorder) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
c.updated = append(c.updated, obj)
return c.Client.Update(ctx, obj, opts...)
}

0 comments on commit b3ea675

Please sign in to comment.