Skip to content
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.

Sign up for GitHub

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

apiserver: better-log handling routines when changes occur to extensi… #9719

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

apiserver: better-log handling routines when changes occur to extensi… #9719

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5542d83
apiserver: better-log handling routines when changes occur to extensi…
aaaaaaaalex Jan 15, 2025
2c93a61
show differing data keys between versions of configmap
aaaaaaaalex Jan 15, 2025
1890e4d
dont log values of configmap
aaaaaaaalex Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apiserver/cmd/apiserver/server/run_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func RunServer(opts *CalicoServerOptions, server *apiserver.ProjectCalicoServer)
logrus.Error("Unable to watch the extension auth ConfigMap: ", err)
}
if changed {
logrus.Debug("Detected change in extension-apiserver-authentication ConfigMap, exiting so apiserver can be restarted")
logrus.Info("Detected change in extension-apiserver-authentication ConfigMap, exiting so apiserver can be restarted")
cancel()
}
}()
Expand Down
4 changes: 4 additions & 0 deletions apiserver/cmd/apiserver/server/watch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"os"
"time"

"github.com/sirupsen/logrus"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -65,12 +66,14 @@ func WatchExtensionAuth(ctx context.Context) (bool, error) {
Handler: cache.ResourceEventHandlerFuncs{
AddFunc: func(_ interface{}) {
if synced {
logrus.Info("Detected creation of extension-apiserver-authentication ConfigMap")
changed = true
cancel()
}
},
DeleteFunc: func(_ interface{}) {
if synced {
logrus.Info("Detected deletion of extension-apiserver-authentication ConfigMap")
changed = true
cancel()
}
Expand All @@ -81,6 +84,7 @@ func WatchExtensionAuth(ctx context.Context) (bool, error) {
n := new.(*corev1.ConfigMap)
// Only detect as changed if the version has changed
Copy link
Member

@fasaxc fasaxc Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done now

if o.ResourceVersion != n.ResourceVersion {
logrus.WithFields(logrus.Fields{"old.ResourceVersion": o.ResourceVersion, "new.ResourceVersion": n.ResourceVersion}).Info("Detected update to extension-apiserver-authentication ConfigMap")
changed = true
cancel()
}
Expand Down
Loading