CFE-866: feature: monitoring and handling "single" secret dynamically #1675

chiragkyal · 2024-02-06T11:26:18Z

Functionality for monitoring and handling "single" secret dynamically.

Changes in `single_item_monitor.go`:

ObjectKey representing the unique identifier for a resource.
singleItemMonitor responsible for monitoring a single resource using a SharedInformer.
Defined methods for starting and stopping informers, adding and removing event handlers, and retrieving items from the informer's cache.

Changes in `secret_monitor.go`:

Implemented functionalities to create and manage informers for specific secrets.
SecretEventHandlerRegistration for registering and unregistering event handlers for secret monitoring.
SecretMonitor for monitoring and handling specific secrets using singleItemMonitor.
Added methods for adding and removing secret event handlers, retrieving secret objects from the informer's cache.
operations for counting the number of event handlers associated with a secret.

Related: CFE-866
Implements: openshift/enhancements#1307

openshift-ci-robot · 2024-02-06T11:26:23Z

@chiragkyal: This pull request references CFE-866 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Functionality for monitoring and handling "single" secret dynamically.

Changes in single_item_monitor.go:

ObjectKey representing the unique identifier for a resource.

singleItemMonitor responsible for monitoring a single resource using a SharedInformer.

Defined methods for starting and stopping informers, adding and removing event handlers, and retrieving items from the informer's cache.

Changes in secret_monitor.go:

Implemented functionalities to create and manage informers for specific secrets.

SecretEventHandlerRegistration for registering and unregistering event handlers for secret monitoring.

SecretMonitor for monitoring and handling specific secrets using singleItemMonitor.

Added methods for adding and removing secret event handlers, retrieving secret objects from the informer's cache.

operations for counting the number of event handlers associated with a secret.

Related: CFE-866
Implements: openshift/enhancements#1307

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 openshift-eng/jira-lifecycle-plugin repository.

chiragkyal · 2024-02-06T11:26:53Z

/assign @deads2k

pkg/secret/monitor.go

deads2k · 2024-02-09T16:08:47Z

pkg/secret/monitor.go

+		case <-ctx.Done():
+			klog.Info("stopping informer due to context cancellation")
+			if !i.StopInformer() {
+				klog.Error("failed to stop informer")


Future-you can tell me if this was a good idea. I'm not sure either way.

I think we can keep this message for now, later we may drop it.

deads2k · 2024-02-09T16:09:33Z

pkg/secret/monitor.go

+
+	klog.Info("starting informer")
+	i.stopped = false
+	i.lock.Unlock()


move to defer unlock just under the lock. Both your unlocks are at the tail.

Since informer.Run is a blocking call, we need to unlock before invoking (starting any informer) it. I think moving unlock to defer will lock this function forever.(after first call)

Since informer.Run is a blocking call, we need to unlock before invoking (starting any informer) it. I think moving unlock to defer will lock this function forever.(after first call)

that line below looks bugged. I explicitly (instead of implicitly) commented this time.

After the latest changes, the StartInformer is non-blocking. So I've moved it to defer unlock

pkg/secret/secret_monitor.go

deads2k · 2024-02-09T16:19:35Z

pkg/secret/monitor.go

+}
+
+// StartInformer starts and runs the informer util the provided context is canceled,
+// or StopInformer() is called. It will block, so call via goroutine.


Start* usually doesn't block in kube. Run usually does. This looks easier to use if it doesn't block. Is there a use-case for blocking?

This function is ultimately calling informer.Run which is a blocking call, and for that reason it's also blocking.
If StartInformer() name is misleading, should we name it RunInformer()?

This function is ultimately calling informer.Run which is a blocking call, and for that reason it's also blocking.
If StartInformer() name is misleading, should we name it RunInformer()?

How about making it non-blocking?

Updated to make it non-blocking

pkg/secret/secret_monitor.go

deads2k · 2024-02-09T16:22:43Z

pkg/secret/secret_monitor.go

+
+// RemoveSecretEventHandler removes a secret event handler and stops the informer if no handlers are left.
+// If the handler is not found or if there is an issue removing it, an error is returned.
+func (s *secretMonitor) RemoveSecretEventHandler(handlerRegistration SecretEventHandlerRegistration) error {


take context

Do we need this for future usage? Unlike AddSecretEventHandler() where we need a context to stop the informer, I'm wondering what the context would do during removal.

Do we need this for future usage? Unlike AddSecretEventHandler() where we need a context to stop the informer, I'm wondering what the context would do during removal.

Cancellation and timeouts. I won't stick on it.

deads2k · 2024-02-09T16:23:46Z

fairly minor comments. Please address and have a teammate check your test coverage.

chiragkyal · 2024-02-09T18:32:27Z

/assign @alebedev87
FYI: this PR was split from #1626 to have only the secret monitoring piece.

Signed-off-by: chiragkyal <ckyal@redhat.com>

chiragkyal · 2024-04-19T08:32:48Z

/retest

alebedev87 · 2024-04-19T10:00:26Z

Reviewed as a cherry picked commit in #1626.

/lgtm
/approve

Signed-off-by: chiragkyal <ckyal@redhat.com>

Miciah · 2024-04-22T16:57:29Z

/lgtm

deads2k · 2024-04-24T19:49:49Z

pkg/secret/OWNERS

+  - chiragkyal
+approvers:
+  - deads2k
+  - soltysh


@soltysh your call

deads2k · 2024-04-24T19:56:29Z

pkg/secret/monitor.go

+	i.stopped = false
+	i.lock.Unlock()
+
+	i.informer.Run(i.stopCh)


Suggested change

i.informer.Run(i.stopCh)

go i.informer.Run(i.stopCh)

to avoid having your start block.

It's blocking by design, the function's godoc comment states this:

It will block, so call via goroutine.

Updated to make it non-blocking

Signed-off-by: chiragkyal <ckyal@redhat.com>

chiragkyal · 2024-04-25T13:32:56Z

pkg/secret/OWNERS

+approvers:
+  - deads2k
+  - soltysh
+  - Miciah


@Miciah, as discussed, added you also

openshift-ci · 2024-04-25T13:45:35Z

@chiragkyal: 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.

deads2k · 2024-04-25T15:09:01Z

/lgtm
/approve

openshift-ci · 2024-04-25T15:13:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87, chiragkyal, deads2k, Miciah

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~~ [deads2k]

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 added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 6, 2024

openshift-ci bot assigned deads2k Feb 6, 2024

openshift-ci bot requested review from deads2k and soltysh February 6, 2024 11:29

chiragkyal mentioned this pull request Feb 6, 2024

CFE-866: Add secret manager for routes to consume secret_monitor #1626

Merged