apiserver: better-log handling routines when changes occur to extensi… #9719
Conversation
aaaaaaaalex
added
release-note-required
…on-apiserver-authentication ConfigMap
aaaaaaaalex
force-pushed
the
apiserver-configmap-restart-log-level
branch
from
6d40be5 to
5542d83
Compare
| @@ -81,6 +84,7 @@ func WatchExtensionAuth(ctx context.Context) (bool, error) { | |||
| n := new.(*corev1.ConfigMap) | |||
| // Only detect as changed if the version has changed | |||
There was a problem hiding this comment.
Do we definitely care about every revision change here? Seems like we could easily check the data for changes instead:
if len(n.Data) != len(o.Data) { changed = true ... }
for k, v := range n.Data {
if ov, ok := o.Data[k] !ok || ov != v {
logrus.WithField("key", k).Info("Config map key changed/added") // Guessing we don't want to log the value (might be sensitive?)
changed = true ... }
}
// Same for n.BinaryData ...
(It's common for an operator to tweak annotations on the object every few seconds, but we should look at the data we care about and ignore anything else.)
There was a problem hiding this comment.
The intention of this change is to see if our cache is stale and re-iterating over the same revisions post-restart, rather than investigating the change itself. Though, I suppose if we confirm that our cache is not stale, inspecting the changes themselves is the next step, so I'll put both in, thanks!
There was a problem hiding this comment.
This is done now
aaaaaaaalex
force-pushed
the
apiserver-configmap-restart-log-level
branch
from
387f3e7 to
4b34d5f
Compare
aaaaaaaalex
force-pushed
the
apiserver-configmap-restart-log-level
branch
from
4b34d5f to
2c93a61
Compare
| "newResourceVersion": n.ResourceVersion, | ||
| "changedDataKey": changedKey, | ||
| "oldKeyValue": oldVal, | ||
| "newKeyValue": newVal, |
There was a problem hiding this comment.
We mustn't log sensitive values, and I think these come from a secret which by default makes them sensitive (unless carefully reasoned otherwise).
There was a problem hiding this comment.
Sorted now, thanks
…on-apiserver-authentication ConfigMap
Description
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.