OCPBUGS-33958: secret re-creation scenario for externalCertificate with active informer #614

chiragkyal · 2024-07-15T07:39:23Z

This PR:

Handles secret re-creation scenarios for externalCertificate where a previously deleted secret is added again.
Introduces a deletedSecrets map in RouteSecretManager to track routes for which the associated secret was deleted. This helps differentiate between new secret creation and recreation of a previously deleted secret.
Refactors secret handling logic in generateSecretHandler to address these changes.
- AddFunc handles the secret recreations. Upon detecting a recreated secret, it revalidates the route, updates its TLS configuration, and triggers a Modified event for the next plugins.
- DeleteFunc does not call UnregisterRoute(), instead tracks deleted secrets.
Improves the logic for handling Modified events for routes with externalCertificate to avoid blindly unregistering and re-registrations with the secret manager.
- This change is also required to avoid always calling UnregisterRoute(), which leads to the stopping of the informer.
- Depends on OCPBUGS-33958: retrieve registered secret name from secretManager library-go#1740
Makes stopping of the informer self-contained to the RouteSecretManager plugin.
- RemoveRoute (templateRouter) should not call UnregisterRoute() to avoid removal of informer when RouteSecretManager calls the next plugin chain with watch.Deleted event.
- Informer will be stopped only during the following cases
  - Route is deleted
  - Route is updated and externalCertificate field is removed
  - Route is updated and externalCertificate field is changed to a new secret name. [Old secret name is cached inside SecretManager's registeredHandlers map.
    - OCPBUGS-33958: retrieve registered secret name from secretManager library-go#1740
Adds unit tests to cover the new logic and scenarios.

New Helper Functions:
validate() to encapsulate the externalCertificate validation logic.
populateRouteTLSFromSecret() to update a route's in-memory TLS configuration from the referenced secret.
unregister() to remove the route's registration with the secret manager and clean up references in deletedSecrets.

Part of : https://issues.redhat.com//browse/OCPBUGS-33958

openshift-ci · 2024-07-15T07:39:27Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2024-07-15T07:39:28Z

@chiragkyal: This pull request references Jira Issue OCPBUGS-33958, which is invalid:

expected the bug to target the "4.17.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

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

In response to this:

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-07-25T09:09:01Z

/jira refresh

openshift-ci-robot · 2024-07-25T09:09:06Z

@chiragkyal: This pull request references Jira Issue OCPBUGS-33958, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

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.

openshift-ci-robot · 2024-08-28T09:03:23Z

@chiragkyal: This pull request references Jira Issue OCPBUGS-33958, which is invalid:

expected the bug to target either version "4.18." or "openshift-4.18.", but it targets "4.17.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

This PR:

Handles secret re-creation scenarios for externalCertificate where a previously deleted secret is added again.

Introduces a deletedSecrets map in RouteSecretManager to track routes for which the associated secret was deleted. This helps differentiate between new secret creation and recreation of a previously deleted secret.

Refactors secret handling logic in generateSecretHandler to address these changes.

AddFunc handles the secret recreations. Upon detecting a recreated secret, it revalidates the route, updates its TLS configuration, and triggers a Modified event for the next plugins.

DeleteFunc does not call UnregisterRoute(), instead tracks deleted secrets.

Improves the logic for handling Modified events for routes with externalCertificate to avoid blindly unregistering and re-registrations with the secret manager.

This change is also required to avoid always calling UnregisterRoute(), which leads to the stopping of the informer.

Depends on OCPBUGS-33958: retrieve registered secret name from secretManager library-go#1740

Makes stopping of the informer self-contained to the RouteSecretManager plugin.

RemoveRoute (templateRouter) should not call UnregisterRoute() to avoid removal of informer when RouteSecretManager calls the next plugin chain with watch.Deleted event.

Informer will be stopped only during the following cases

Route is deleted

Route is updated and externalCertificate field is removed

Route is updated and externalCertificate field is changed to a new secret name. [Old secret name is cached inside SecretManager's registeredHandlers map.

OCPBUGS-33958: retrieve registered secret name from secretManager library-go#1740

Adds unit tests to cover the new logic and scenarios.

New Helper Functions:

validate() to encapsulate the externalCertificate validation logic.

populateRouteTLSFromSecret() to update a route's in-memory TLS configuration from the referenced secret.

unregister() to remove the route's registration with the secret manager and clean up references in deletedSecrets.

Part of : https://issues.redhat.com//browse/OCPBUGS-33958

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.

Miciah · 2024-08-28T15:19:49Z

/assign

candita · 2024-08-28T17:51:10Z

/assign @alebedev87

chiragkyal · 2024-08-29T13:38:22Z

/jira refresh

openshift-ci-robot · 2024-08-29T13:38:27Z

@chiragkyal: This pull request references Jira Issue OCPBUGS-33958, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

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.

openshift-ci · 2024-09-16T08:34:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from alebedev87. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

to vendor openshift/library-go#1740 go get github.com/openshift/library-go@0119d1ed45b58f8e7b180370b4cfe933846f9a91 Signed-off-by: chiragkyal <[email protected]>

* RemoveRoute should not delete the informer (deletion will be handled by the plugin) * utilize old secret name for modified event Signed-off-by: chiragkyal <[email protected]>

Signed-off-by: chiragkyal <[email protected]>

alebedev87

Some questions and nit picks.

pkg/router/controller/route_secret_manager.go

pkg/router/controller/route_secret_manager_test.go

alebedev87 · 2024-10-03T12:59:17Z

pkg/router/controller/route_secret_manager.go

+				// since secret re-creation will not trigger the apiserver route admission,
+				// we need to rely on the router controller for this validation.
+				if err := p.validate(route); err != nil {
+					return


If we fail here, will the fact that the deletedSecrets has been cleaned already cause any problem?

If we fail here, then there are two choices

Delete that secret which will re-populate deletedSecrets ; and then re-create it

Update that secret

Both choices will only work after fixing the issue which had caused the validation to fail.

Of we fail here, will the fact that the deletedSecrets has been cleaned already cause any problem?

So, it should not cause any problem.

Signed-off-by: chiragkyal <[email protected]>

chiragkyal · 2024-10-08T07:01:59Z

pkg/router/controller/route_secret_manager.go

+	// TODO always creating a new `secretHandler` ensures that there are no stale route specs
 	// in the next plugin chain, especially when the referenced secret is updated or deleted.
 	// This prevents sending outdated routes to subsequent plugins, preserving expected functionality.
-	// TODO: Refer https://github.com/openshift/router/pull/565#discussion_r1596441128 for possible ways to improve the logic.
+	// TODO : we might need to add RouteLister()


I think we need to use RouteLister() inside all ResourceEventHandlerFuncs of generateSecretHandler() to avoid sending stale route object in the next plugin chain.

As discussed during the sync call, the problem statement is valid - the route state is captured at the time of the handler registration which may lead to the update of the route to the older state. The solution which gets the route state from the controller-runtime cache may be problematic as it breaks out of the single source of truth for routes - route controller. Alternative solutions:

when the secret manager detects a secret recreation the corresponding route should be re-synced by the route controller forcing it to go through the plugin chain (preferred).

when a route is registered into the secret manager its whole state can be passed as an additional parameter forming a dedicated cache of routes with external certificates. When the secret manager detects a secret recreation it can get the latest route state from this cache.

when the secret manager detects a secret recreation the corresponding route should be re-synced by the route controller forcing it to go through the plugin chain (preferred).

If we can send some event (maybe forcefully) to the Shared RouteInformer from the secret handlerFuncs without updating the actual route object, then that would be great.

when a route is registered into the secret manager its whole state can be passed as an additional parameter forming a dedicated cache of routes with external certificates. When the secret manager detects a secret recreation it can get the latest route state from this cache.

A route is registered only once with the secret manager. We're removing the logic of always doing re-registration of route for every plugin chain call in this PR to have the informer active. So, if we want to keep a dedicated cache of routes, it has to be part of RouteSecretManager rather than SecretManager (exists in library-go)

chiragkyal · 2024-10-08T07:02:55Z

/retest-required

chiragkyal · 2024-10-23T11:29:54Z

@alebedev87 Apart from #614 (comment) discussion, is this PR lgtm from your side?

Signed-off-by: chiragkyal <[email protected]>

openshift-ci · 2024-11-12T21:57:05Z

@chiragkyal: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-serial	`c5e9c7f`	link	true	`/test e2e-aws-serial`

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-sigs/prow repository. I understand the commands that are listed here.

Miciah

I took at look at the latest changes so I could leave some quick feedback and questions about recent changes. I still need to go back and re-review the older changes.

Miciah · 2024-11-14T15:01:57Z

pkg/router/controller/route_secret_manager.go

+				//    entire plugin chain again.  Without re-validating the external certificate
+				//    and re-syncing the secret here, the route could incorrectly transition
+				//    back to an "active" state and start serving the default certificate
+				//    even though its spec still references an external certificate.
+				//


Why would the router use the default certificate for the route? Why wouldn't it continue to use the route's old external certificate?

This point is central to my concern about the secret manager's setting Admitted=False. I'm fine with setting Admitted=False in these two cases:

The secret was deleted.

A new secret was created after the old one had been deleted (which means that the route should already have Admitted=False).

In other words, the secret manager can set Admitted=False in any sequence of events that includes deleting the secret that the route references. However, I still don't understand the necessity of setting Admitted=False in the case where the secret was updated. We do need to force the controller to revalidate the route, but is it really necessary to mark the route as rejected in the meantime?

Does setting Admitted=False when the secret is updated imply that rotating certificates necessarily causes downtime when using an external certificate? ...Hm, maybe not, as you only call p.plugin.HandleRoute(watch.Deleted, route) in the delete case, and not in the update case; does that mean we report that the route is rejected but continue serving it unless revalidation re-rejects it?

In comparison, if the route has an inline certificate, the route can be updated with a new certificate, and the router continues using the old certificate until it observes the update; once the router does observe the update, it switches to the new certificate without rejecting the route and without any interruption to service. I don't understand why the experience should or must be different when using an external certificate.

Miciah · 2024-11-14T15:02:03Z

pkg/router/controller/route_secret_manager.go

+				//    the same, the router needs to be aware of the events when the content of
+				//    the secret is updated or the secret is recreated, to re-validate and
+				//    fetch the new TLS data. Note: These events won't trigger the apiserver
+				//    route admission, hence we need to rely on the router controller for this validation.


You mean we rely on the controller to trigger re-validation, right? Or are you saying that this scenario requires the controller to perform some validation steps that would otherwise be handled by the admission plugin?

Suggested change

// route admission, hence we need to rely on the router controller for this validation.

// route admission, hence we need to rely on the router controller to trigger this validation.

Miciah · 2024-11-14T15:02:05Z

pkg/router/controller/route_secret_manager.go

+// - AddFunc:
+//   - Invoked when a new secret is added.
+//   - Handles secret recreation: If a secret is recreated (created after being
+//     deleted), it fetches the latest route object associated with it using the


What happens if multiple routes reference the same secret?

Miciah · 2024-11-14T15:02:09Z

pkg/router/controller/route_secret_manager.go

+
+				// The route should *remain* rejected until it's re-evaluated
+				// by all the plugins (including this plugin). Once passes, the route will become active again.
+				p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretReCreated", msg)


Suggested change

p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretReCreated", msg)

p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretRecreated", msg)

Miciah · 2024-11-14T15:02:12Z

pkg/router/controller/route_secret_manager.go

 			}
 		},

 		UpdateFunc: func(old interface{}, new interface{}) {
 			secretOld := old.(*kapi.Secret)
 			secretNew := new.(*kapi.Secret)
-			log.V(4).Info("secret updated for route", "namespace", route.Namespace, "secret", secretNew.Name, "old-version", secretOld.ResourceVersion, "new-version", secretNew.ResourceVersion, "route", route.Name)
+			key := generateKey(namespace, routeName)
+			msg := fmt.Sprintf("secret %q updated for route %q : old-version %q new-version %q", secretNew.Name, key, secretOld.ResourceVersion, secretNew.ResourceVersion)


Suggested change

msg := fmt.Sprintf("secret %q updated for route %q : old-version %q new-version %q", secretNew.Name, key, secretOld.ResourceVersion, secretNew.ResourceVersion)

msg := fmt.Sprintf("secret %q updated for route %q (old resourceVersion=%v, new resourceVersion=%v)", secretNew.Name, key, secretOld.ResourceVersion, secretNew.ResourceVersion)

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2024

openshift-ci-robot added the jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. label Jul 15, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 15, 2024

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Jul 15, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2024

chiragkyal force-pushed the active-informer branch 6 times, most recently from 41a1750 to f51a3f2 Compare July 23, 2024 07:48

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 25, 2024

openshift-ci bot requested a review from lihongan July 25, 2024 09:09

chiragkyal mentioned this pull request Jul 31, 2024

OCPBUGS-33958: retrieve registered secret name from secretManager openshift/library-go#1740

Merged

chiragkyal force-pushed the active-informer branch 4 times, most recently from 70121a3 to f655bd2 Compare August 28, 2024 07:48

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2024

chiragkyal marked this pull request as ready for review August 28, 2024 07:50

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 28, 2024

openshift-ci bot requested review from frobware and Miciah August 28, 2024 07:50

openshift-ci-robot removed the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Aug 28, 2024

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Aug 28, 2024

openshift-ci bot assigned Miciah Aug 28, 2024

openshift-ci bot assigned alebedev87 Aug 28, 2024

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Aug 29, 2024

openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Aug 29, 2024

chiragkyal added 4 commits September 16, 2024 14:04

Bump library-go

1604bc1

to vendor openshift/library-go#1740 go get github.com/openshift/library-go@0119d1ed45b58f8e7b180370b4cfe933846f9a91 Signed-off-by: chiragkyal <[email protected]>

secret re-creation scenario with active informer

2f90b41

* RemoveRoute should not delete the informer (deletion will be handled by the plugin) * utilize old secret name for modified event Signed-off-by: chiragkyal <[email protected]>

refactor: move validatation and GetSecret to separate functions

122d05b

Signed-off-by: chiragkyal <[email protected]>

update unit tests

441b12f

Signed-off-by: chiragkyal <[email protected]>

chiragkyal force-pushed the active-informer branch from 8c31094 to 441b12f Compare September 16, 2024 08:35

alebedev87 reviewed Oct 3, 2024

View reviewed changes

address review comments

b177f0f

Signed-off-by: chiragkyal <[email protected]>

chiragkyal commented Oct 8, 2024

View reviewed changes

Use RouteLister and update route's status during secret events

c5e9c7f

Signed-off-by: chiragkyal <[email protected]>

Miciah reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-33958: secret re-creation scenario for externalCertificate with active informer #614

OCPBUGS-33958: secret re-creation scenario for externalCertificate with active informer #614

chiragkyal commented Jul 15, 2024 •

edited

Loading

openshift-ci bot commented Jul 15, 2024

openshift-ci-robot commented Jul 15, 2024

chiragkyal commented Jul 25, 2024

openshift-ci-robot commented Jul 25, 2024

openshift-ci-robot commented Aug 28, 2024

Miciah commented Aug 28, 2024

candita commented Aug 28, 2024

chiragkyal commented Aug 29, 2024

openshift-ci-robot commented Aug 29, 2024

openshift-ci bot commented Sep 16, 2024

alebedev87 left a comment •

edited

Loading

alebedev87 Oct 3, 2024 •

edited

Loading

chiragkyal Oct 7, 2024

chiragkyal Oct 8, 2024

alebedev87 Oct 17, 2024 •

edited

Loading

chiragkyal Oct 17, 2024

chiragkyal commented Oct 8, 2024

chiragkyal commented Oct 23, 2024

openshift-ci bot commented Nov 12, 2024

Miciah left a comment

Miciah Nov 14, 2024

Miciah Nov 14, 2024

Miciah Nov 14, 2024

Miciah Nov 14, 2024

Miciah Nov 14, 2024

	// route admission, hence we need to rely on the router controller for this validation.
	// route admission, hence we need to rely on the router controller to trigger this validation.

	p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretReCreated", msg)
	p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretRecreated", msg)

	msg := fmt.Sprintf("secret %q updated for route %q : old-version %q new-version %q", secretNew.Name, key, secretOld.ResourceVersion, secretNew.ResourceVersion)
	msg := fmt.Sprintf("secret %q updated for route %q (old resourceVersion=%v, new resourceVersion=%v)", secretNew.Name, key, secretOld.ResourceVersion, secretNew.ResourceVersion)

OCPBUGS-33958: secret re-creation scenario for externalCertificate with active informer #614

Are you sure you want to change the base?

OCPBUGS-33958: secret re-creation scenario for externalCertificate with active informer #614

Conversation

chiragkyal commented Jul 15, 2024 • edited Loading

openshift-ci bot commented Jul 15, 2024

openshift-ci-robot commented Jul 15, 2024

chiragkyal commented Jul 25, 2024

openshift-ci-robot commented Jul 25, 2024

openshift-ci-robot commented Aug 28, 2024

Miciah commented Aug 28, 2024

candita commented Aug 28, 2024

chiragkyal commented Aug 29, 2024

openshift-ci-robot commented Aug 29, 2024

openshift-ci bot commented Sep 16, 2024

alebedev87 left a comment • edited Loading

Choose a reason for hiding this comment

alebedev87 Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alebedev87 Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragkyal commented Oct 8, 2024

chiragkyal commented Oct 23, 2024

openshift-ci bot commented Nov 12, 2024

Miciah left a 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

chiragkyal commented Jul 15, 2024 •

edited

Loading

alebedev87 left a comment •

edited

Loading

alebedev87 Oct 3, 2024 •

edited

Loading

alebedev87 Oct 17, 2024 •

edited

Loading