Skip to content

Adds more debug logging to help figure out OIDC/RBAC issues #3308

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Adds more debug logging to help figure out OIDC/RBAC issues #3308

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d329ec2
Adds more debug logging to help figure out OIDC/RBAC issues
foot Jan 25, 2023
b82dab2
Restore log as debug, fix test
foot Jan 25, 2023
553aba1
Adds some logging around cached client creation
foot Jan 25, 2023
21c8080
Clarify git-provider-token in logging
foot Jan 26, 2023
3d333d3
Adds logging style guide and conformation for this PR
foot Jan 30, 2023
5d4f96a
Merge branch 'main' into adds-debug-logging
foot Jan 30, 2023
3c26405
Merge branch 'main' into adds-debug-logging
foot Feb 27, 2023
d607bb2
Limit the number of namespaces we log
foot Mar 1, 2023
c260026
Sample 3 namespaces, make logging keys a bit more unique
foot Mar 1, 2023
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
26 changes: 24 additions & 2 deletions core/clustersmngr/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/hashicorp/go-multierror"
"github.com/prometheus/client_golang/prometheus"
"github.com/weaveworks/weave-gitops/core/clustersmngr/cluster"
"github.com/weaveworks/weave-gitops/core/logger"
"github.com/weaveworks/weave-gitops/core/nsaccess"
"github.com/weaveworks/weave-gitops/pkg/featureflags"
"github.com/weaveworks/weave-gitops/pkg/server/auth"
Expand Down Expand Up @@ -382,6 +383,8 @@ func (cf *clustersManager) updateNamespacesWithClient(ctx context.Context, creat
}
}

cf.log.V(logger.LogLevelDebug).Info("Updated namespaces cache", "namespaces", clusterNamespacesLogValue(cf.clustersNamespaces.namespaces))

opsUpdateNamespaces.Inc()

return result.ErrorOrNil()
Expand Down Expand Up @@ -589,13 +592,17 @@ func (cf *clustersManager) userNsList(ctx context.Context, user *auth.UserPrinci
if err != nil {
// This may not completely fail the request, e.g. if some of the clusters
// are able to respond with their namespaces. So log the error and continue.
cf.log.Error(err, "failed updating namespaces from user client")
cf.log.Error(err, "error updating namespaces from user client", "user", user)
}
}

cf.UpdateUserNamespaces(ctx, user)

return cf.GetUserNamespaces(user)
userNamespaces = cf.GetUserNamespaces(user)

cf.log.V(logger.LogLevelDebug).Info("Updated namespace access cache for user", "userNamespaces", clusterNamespacesLogValue(userNamespaces), "user", user, "ttl", userNamespaceTTL.String())

return userNamespaces
}

func (cf *clustersManager) getOrCreateClient(ctx context.Context, user *auth.UserPrincipal, cluster cluster.Cluster) (client.Client, error) {
Expand Down Expand Up @@ -633,3 +640,18 @@ func (cf *clustersManager) getOrCreateClient(ctx context.Context, user *auth.Use

return client, nil
}

// Format the namespaces as a map[clusterName][]namespacesNames
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this scale?

What if you have 100+ namespaces? 1000+ ?

For clarity, 1000+ is not uncommon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. For the scenarios we've been exploring ~10-15 namespaces its been a real help. But for 1000 I guess some part of your logging system might get grumpy.

{ "myCluster": { "namespaces": ["foo", "bar"], "totalCount": "3" } # show a sensible N namespaces and report the total count

I don't know if the structured logging has nested structures in mind..

The count alone would still be useful.

// for logging as a value in a structured log.
func clusterNamespacesLogValue(clusterNamespaces map[string][]v1.Namespace) map[string][]string {
out := map[string][]string{}
for cluster, namespaces := range clusterNamespaces {
namespaceNames := []string{}
for _, n := range namespaces {
namespaceNames = append(namespaceNames, n.Name)
}
out[cluster] = namespaceNames
}

return out
}
2 changes: 1 addition & 1 deletion pkg/server/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func (p *UserPrincipal) SetToken(t string) {

// String returns the Principal ID and Groups as a string.
func (p *UserPrincipal) String() string {
return fmt.Sprintf("id=%q groups=%v", p.ID, p.Groups)
return fmt.Sprintf("id=%q groups=%v tokenLength=%v", p.ID, p.Groups, len(p.Token()))
}

// Hash returns a unique string using user id,token and groups.
Expand Down
4 changes: 1 addition & 3 deletions pkg/server/auth/jwt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ func NewJWTAuthorizationHeaderPrincipalGetter(log logr.Logger, verifier tokenVer
}

func (pg *JWTAuthorizationHeaderPrincipalGetter) Principal(r *http.Request) (*UserPrincipal, error) {
pg.log.Info("attempt to read token from auth header")

header := r.Header.Get("Authorization")
if header == "" {
return nil, nil
Expand Down Expand Up @@ -158,7 +156,7 @@ func (m MultiAuthPrincipal) Principal(r *http.Request) (*UserPrincipal, error) {
}

if p != nil {
m.Log.V(logger.LogLevelDebug).Info("Found principal", "user", p.ID, "groups", p.Groups, "tokenLength", len(p.Token()), "method", reflect.TypeOf(v))
m.Log.V(logger.LogLevelDebug).Info("Found principal", "user", p, "method", reflect.TypeOf(v))

return p, nil
}
Expand Down