OCPBUGS-14994: Don't add clientca-configmap finalizer if deleting #948

Miciah · 2023-06-15T02:45:05Z

Don't add the clientca-configmap finalizer to an IngressController if it is marked for deletion.

Before this change, 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.

pkg/operator/controller/clientca-configmap/controller.go (Reconcile): Only attempt to add the finalizer if the IngressController is not marked for deletion.

openshift-ci-robot · 2023-06-15T02:45:14Z

@Miciah: This pull request references Jira Issue OCPBUGS-14994, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Don't add the clientca-configmap finalizer to an IngressController if it is marked for deletion.

Before this change, 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.

pkg/operator/controller/clientca-configmap/controller.go (Reconcile): Only attempt to add the finalizer if the IngressController is not marked for deletion.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

candita · 2023-06-21T21:03:36Z

/assign

candita · 2023-06-21T21:37:52Z

pkg/operator/controller/clientca-configmap/controller.go

@@ -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) {


Could you add a unit test for this change?

https://github.com/openshift/cluster-ingress-operator/compare/d6ec5434cf35d6046264ceb5d652a5cc22cae4f3..b3ea6755c73f9e0a0a4980ae2deb838e072fda95 adds a unit test.

candita · 2023-06-26T21:57:46Z

/retest-required

openshift-bot · 2023-09-25T09:00:46Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

candita · 2023-09-25T20:53:57Z

/remove-lifecycle stale

candita · 2023-09-25T20:55:31Z

/test e2e-gpc-ovn
/test e2e-azure-operator
/test e2e-aws-ovn-single-node

candita · 2023-10-18T15:26:45Z

/assign @frobware

candita · 2023-10-18T21:03:54Z

pkg/operator/controller/clientca-configmap/controller_test.go

+			expectDelete: []client.Object{},
+		},
+		{
+			name: "source configmap absent, target configmap absent, finalizer absent",


Isn't the source configmap present in existingObjects and expectUpdate? And the finalizer is present in expectUpdate too. Am I reading it wrong?

In this test case, the IngressController exists and specifies the "ca-bundle" configmap, but the configmap doesn't exist, so the controller can add the finalizer but cannot copy the configmap.

If it's not too much trouble, it would be helpful to specify in the test name what the outcome of the test is supposed to be. So here, because there is no cm in the existingObjects, there is no cm in the expectUpdate? And, it is ok for the configmap name to be there in the ic even if there is no cm object? And we only add a finalizer if there is a configmap name, even if there is no matching cm object?

If it's not too much trouble, it would be helpful to specify in the test name what the outcome of the test is supposed to be.

Done in https://github.com/openshift/cluster-ingress-operator/compare/3a3fb20502db4bb46db3de1ffef89faa5f84cc6a..e9ceaf5969c681a0ba773d5f97bc661703dfbf17.

So here, because there is no cm in the existingObjects, there is no cm in the expectUpdate?

Correct. There is no source configmap, so the controller cannot make a copy of it.

And, it is ok for the configmap name to be there in the ic even if there is no cm object?

It can happen, so the controller needs to handle the case. The router pods cannot start until the configmap exists though because the router pods have a volume mount for the configmap.

And we only add a finalizer if there is a configmap name, even if there is no matching cm object?

Correct.

candita · 2023-10-18T21:42:26Z

pkg/operator/controller/clientca-configmap/controller_test.go

+				Namespace:         "openshift-ingress-operator",
+				Name:              "test",
+			},
+			Spec: operatorv1.IngressControllerSpec{


Why is the configmap being examined in these tests?

The purpose of this controller is to copy the user-provided configmap, which is specified in the IngressController, from the "openshift-config" namespace into the "openshift-ingress" namespace, so the test is written to give the controller an IngressController that specifies a configmap and verify that the controller copies the configmap if it exists.

Did I understand your question correctly and answer it? Should I add a code comment with the above explanation?

I was expecting a test that only checked for refraining of adding the finalizer when deleting.

The controller was missing test coverage entirely, so I added a comprehensive test, including test cases for the specific issue in OCPBUGS-14994. Would it be better if I split it into two commits: one to add the test and one to add the specific test case that is related to OCPBUGS-14994?

No, this is fine.

frobware

LGTM, just a comment regarding the necessity of the OperatorNamespace in Test_Reconcile.

pkg/operator/controller/clientca-configmap/controller_test.go

frobware · 2023-10-19T10:17:05Z

/retest

frobware · 2023-10-19T16:32:45Z

/lgtm

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.

* pkg/operator/controller/clientca-configmap/controller.go (Config): * pkg/operator/controller/clientca-configmap/controller_test.go (Test_Reconcile): * pkg/operator/operator.go (New): Remove unused OperatorNamespace field.

openshift-ci · 2023-10-25T07:33:05Z

@Miciah: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

frobware · 2023-10-25T10:31:20Z

Good doc additions on both the test function and individual test cases.

/lgtm

candita · 2023-10-30T17:26:26Z

/approve

openshift-ci · 2023-10-30T17:30:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [candita]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-10-30T17:33:44Z

@Miciah: Jira Issue OCPBUGS-14994: All pull requests linked via external trackers have merged:

openshift/cluster-ingress-operator#948

Jira Issue OCPBUGS-14994 has been moved to the MODIFIED state.

In response to this:

Don't add the clientca-configmap finalizer to an IngressController if it is marked for deletion.

Before this change, 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.

pkg/operator/controller/clientca-configmap/controller.go (Reconcile): Only attempt to add the finalizer if the IngressController is not marked for deletion.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-merge-robot · 2023-10-31T09:40:58Z

Fix included in accepted release 4.15.0-0.nightly-2023-10-31-054858

openshift-ci bot requested review from lihongan, candita and rfredette June 15, 2023 02:45

openshift-ci bot assigned candita Jun 21, 2023

candita reviewed Jun 21, 2023

View reviewed changes

Miciah force-pushed the OCPBUGS-14994-do-not-add-clientca-configmap-finalizer-if-deleting branch from d6ec543 to b3ea675 Compare June 22, 2023 02:50

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 25, 2023

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 25, 2023

openshift-ci bot assigned frobware Oct 18, 2023

candita reviewed Oct 18, 2023

View reviewed changes

frobware reviewed Oct 19, 2023

View reviewed changes

pkg/operator/controller/clientca-configmap/controller_test.go Outdated Show resolved Hide resolved

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2023

Miciah added 2 commits October 25, 2023 00:33

clientca-configmap: Remove OperatorNamespace field

e9ceaf5

* pkg/operator/controller/clientca-configmap/controller.go (Config): * pkg/operator/controller/clientca-configmap/controller_test.go (Test_Reconcile): * pkg/operator/operator.go (New): Remove unused OperatorNamespace field.

Miciah force-pushed the OCPBUGS-14994-do-not-add-clientca-configmap-finalizer-if-deleting branch from 3a3fb20 to e9ceaf5 Compare October 25, 2023 04:50

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2023

openshift-ci bot merged commit 23ee5be into openshift:master Oct 30, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-14994: Don't add clientca-configmap finalizer if deleting #948

OCPBUGS-14994: Don't add clientca-configmap finalizer if deleting #948

Miciah commented Jun 15, 2023

openshift-ci-robot commented Jun 15, 2023

candita commented Jun 21, 2023

candita Jun 21, 2023

Miciah Jun 22, 2023

candita commented Jun 26, 2023

openshift-bot commented Sep 25, 2023

candita commented Sep 25, 2023

candita commented Sep 25, 2023

candita commented Oct 18, 2023

candita Oct 18, 2023

Miciah Oct 18, 2023

candita Oct 19, 2023

Miciah Oct 25, 2023

candita Oct 18, 2023

Miciah Oct 18, 2023

candita Oct 19, 2023

Miciah Oct 25, 2023

candita Oct 30, 2023

frobware left a comment

frobware commented Oct 19, 2023

frobware commented Oct 19, 2023

openshift-ci bot commented Oct 25, 2023

frobware commented Oct 25, 2023

candita commented Oct 30, 2023

openshift-ci bot commented Oct 30, 2023

openshift-ci-robot commented Oct 30, 2023

openshift-merge-robot commented Oct 31, 2023

OCPBUGS-14994: Don't add clientca-configmap finalizer if deleting #948

OCPBUGS-14994: Don't add clientca-configmap finalizer if deleting #948

Conversation

Miciah commented Jun 15, 2023

openshift-ci-robot commented Jun 15, 2023

candita commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita commented Jun 26, 2023

openshift-bot commented Sep 25, 2023

candita commented Sep 25, 2023

candita commented Sep 25, 2023

candita commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frobware left a comment

Choose a reason for hiding this comment

frobware commented Oct 19, 2023

frobware commented Oct 19, 2023

openshift-ci bot commented Oct 25, 2023

frobware commented Oct 25, 2023

candita commented Oct 30, 2023

openshift-ci bot commented Oct 30, 2023

openshift-ci-robot commented Oct 30, 2023

openshift-merge-robot commented Oct 31, 2023