-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CFE-866: Add secret manager for routes to consume secret_monitor #1626
CFE-866: Add secret manager for routes to consume secret_monitor #1626
Conversation
@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.15.0" version, but no target version was set. 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 kubernetes/test-infra repository. |
Skipping CI for Draft Pull Request. |
Continued from #1518 |
/retest |
2f5683e
to
632b0c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solid start
/assign @alebedev87 |
pkg/route/secret/manager.go
Outdated
// each route (namespace/routeName) should be registered only once with any secret. | ||
// Note: inside a namespace multiple different routes can be registered(watch) with a common secret | ||
key := generateKey(namespace, routeName) | ||
if _, exists := m.registeredHandlers[key]; exists { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the secret name changes, what should happen? Probably a remove and then an add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, unregister old secret and register new secret.
pkg/route/secret/manager.go
Outdated
return apierrors.NewInternalError(fmt.Errorf("route already registered with key %s", key)) | ||
} | ||
|
||
handlerRegistration, err := m.monitor.AddSecretEventHandler(namespace, secretName, m.secretHandler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth considering how your consuming layers will expect the "route changed secret" to be represented to them.
pkg/route/secret/manager.go
Outdated
} | ||
|
||
// wait for informer store sync, to load secret | ||
if err := wait.PollImmediate(10*time.Millisecond, time.Second, func() (done bool, err error) { return handlerRegistration.HasSynced(), nil }); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you waited for the HasSync
in the step above, this wait should no longer be necessary, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That HasSync
will be called only once after starting the informer. Correct me if I'm wrong, but I think we need to call HasSync
for every GetSecret
invocation to update the cache with the latest state of the resource.
/test all |
As discussed, we'll split this PR into 2 PRs. |
1dbb52b
to
6fc1b0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second iteration, mostly the nit picks.
pkg/route/secret/secret_monitor.go
Outdated
if err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the error is checked after the exist
variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Informer's cache (GetByKey()
) is returning error
even with exists
, so we should handle error
instead of ignoring it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't make the question clear. I meant that why not checking the error before we check exist
. It may be misleading as exist
may be false
just because it's a default value returned in case of an error. This may result in a wrong error returned from the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that makes sense. Moved err
check before exist
. Thanks!
53fa78e
to
586b784
Compare
/hold |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. All my comments are suggestions to fix typos in comments or log or error messages and a few minor questions around error values and test code. Feel free to take or leave my suggestions, or handle them in a follow-up.
/lgtm
pkg/secret/monitor.go
Outdated
return i.informer.HasSynced() | ||
} | ||
|
||
// StartInformer starts and runs the informer util the provided context is canceled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// StartInformer starts and runs the informer util the provided context is canceled, | |
// StartInformer starts and runs the informer until the provided context is canceled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
pkg/secret/monitor.go
Outdated
defer i.lock.Unlock() | ||
|
||
if i.stopped { | ||
return nil, fmt.Errorf("can not add handler %v to already stopped informer", handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("can not add handler %v to already stopped informer", handler) | |
return nil, fmt.Errorf("cannot add handler %v to already stopped informer", handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
pkg/secret/secret_monitor.go
Outdated
// secret identifier (namespace/secret) | ||
key := NewObjectKey(namespace, secretName) | ||
|
||
// start secret informer if monitor does not exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// start secret informer if monitor does not exists | |
// Start secret informer if monitor does not exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
pkg/secret/secret_monitor.go
Outdated
return nil, fmt.Errorf("failed waiting for cache sync") | ||
} | ||
|
||
// add item key to monitors map // add watch to the list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The // add watch to the list
looks conspicuous—is this a stale comment that accidentally got joined with the // add item key to monitors map
comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was a stale comment. I've removed // add watch to the list
comment
pkg/secret/secret_monitor.go
Outdated
// add item key to monitors map // add watch to the list | ||
s.monitors[key] = m | ||
|
||
klog.Info("secret informer started", " item key ", key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, are the extra spaces intended to be there?
klog.Info("secret informer started", " item key ", key) | |
klog.Info("secret informer started", "item key", key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these are intended because it looks like klog.Info
is not putting spaces between two comma ,
separated strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was thinking these were key-value pairs, like go-logr/logr uses. Why not use a format?
klog.Info("secret informer started", " item key ", key) | |
klog.Infof("secret informer started for item with key %v", key) |
This is a minor issue and can be addressed in a follow-up if you decide to take my suggestion.
pkg/secret/monitor_test.go
Outdated
// for handling nil pointer dereference | ||
defer func() { | ||
if err := recover(); err != nil && !s.expectErr { | ||
t.Errorf("unexpected error %v", err) | ||
} | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This recover
block highlights an oddity of this test: The "nil handler is provided" test case isn't actually testing the behavior of AddEventHandler
; it's testing the behavior of the Go runtime, to verify that dereferencing a nil pointer causes a panic—right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was there to recover from panic when handle.GetHandler()
is executed inside RemoveEventHandler
, given a nil SecretEventHandlerRegistration
.
I think a much easier way would be to add if
statement inside RemoveEventHandler
to check nil SecretEventHandlerRegistration
.
I've added the following the latest commit
if handle == nil {
return fmt.Errorf("nil handler registration is provided")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it—the test is simpler, and the method is more robust. Thanks!
pkg/secret/secret_monitor_test.go
Outdated
// Delete keys if already removed | ||
for _, keyRemoved := range s.alreadyRemoved { | ||
delete(sm.monitors, keyRemoved) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this testing something that is expected to happen, or is this testing that RemoveSecretEventHandler
behaves correctly if the secret monitor's internal data structures are inexplicably corrupted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this testing that RemoveSecretEventHandler behaves correctly if the secret monitor's internal data structures are inexplicably corrupted?
For this reason(^)
It is trying to simulate the condition when the secret monitor does not contain the intended key. This is unlikely to happen, but I think we should have a test to check RemoveSecretEventHandler
catches this issue.
m, exists := s.monitors[key]
if !exists {
return fmt.Errorf("secret monitor already removed for item key %v", key)
}
pkg/secret/secret_monitor_test.go
Outdated
|
||
gotSec, gotErr := sm.GetSecret(context.TODO(), h) | ||
if (gotErr != nil) != s.expectErr { | ||
t.Fatalf("expected errors to be %t, but got %t", s.expectErr, err != nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Fatalf("expected errors to be %t, but got %t", s.expectErr, err != nil) | |
t.Fatalf("expected errors to be %t, but got %t", s.expectErr, gotErr != nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
pkg/route/secretmanager/manager.go
Outdated
return apierrors.NewInternalError(fmt.Errorf("route already registered with key %s", key)) | ||
} | ||
|
||
// Add a secret event handler for the specified namespace and secret, with the handler functions. | ||
klog.V(5).Infof("trying to add handler for key %s with secret %s", key, secretName) | ||
handlerRegistration, err := m.monitor.AddSecretEventHandler(ctx, namespace, secretName, handler) | ||
if err != nil { | ||
return apierrors.NewInternalError(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to use apierrors.NewInternalError
here? I thought apierrors.NewInternalError
was intended for errors that originated from the API server. And even if it's safe to use outside of the API server context, it isn't clear to me why the error needs to be wrapped.
That said, I don't see any router code that uses apierrors.IsInternalError
to do something special with such errors, so using apierrors.NewInternalError
shouldn't cause any problems for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I also think router code will not make use of this wrapped error by calling apierrors.IsInternalError
. So, to keep it consistent, I've drooped the apierrors
usage from this package.
pkg/route/secretmanager/manager.go
Outdated
return nil, apierrors.NewInternalError(fmt.Errorf("no handler registered with key %s", key)) | ||
} | ||
|
||
// get secret from secrt monitor's cache using registered handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// get secret from secrt monitor's cache using registered handler | |
// Get the secret from the secret monitor's cache using the registered handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Thanks! |
Signed-off-by: chiragkyal <ckyal@redhat.com>
6ff1efe
to
cef63db
Compare
@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:
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. |
/unhold |
@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. |
@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:
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. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, chiragkyal, 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:
Approvers can indicate their approval by writing |
Vendoring openshift/library-go#1626 Signed-off-by: chiragkyal <ckyal@redhat.com>
Description
This PR adds
route
layer (introduces a newsecretmanager
package) on top of #1675 to consume thesecret_monitor
.These changes primarily focus on enhancing the functionality of managing secrets associated with routes.
Important Functions:
Related: CFE-866
Implements: openshift/enhancements#1307