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.

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

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

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

Conversation

foot
Copy link
Contributor

@foot foot commented Jan 25, 2023

What changed?

  • We log out the namespaces we've guessed a user has access
  • Include user a bit more in the logs too
  • Remove "attempt to read token from auth header" as its not that useful

Why was this change made?

Make it easier to debug why things may be missing on the apps/sources etc pages

How was this change implemented?

Adding more logging

How did you validate the change?

Manually

Examples

{"level":"debug","ts":1674665219.1867902,"logger":"gitops","caller":"clustersmngr/factory.go:386","msg":"Updated namespaces cache","namespaces":{"default/vcluster":["default","kube-system","kube-public","kube-node-lease","flux-system"],"management":[]}}
{"level":"error","ts":1674665219.1871428,"logger":"gitops","caller":"clustersmngr/factory.go:595","msg":"error updating namespaces from user client","user":"id=\"alice\" groups=[foo] tokenLength=922","error":"1 error occurred:\n\t* Failed to list resource on cluster=\"management\" namespace=\"\" err=\"namespaces is forbidden: User \\\"alice\\\" cannot list resource \\\"namespaces\\\" in API group \\\"\\\" at the cluster scope\"\n\n"}
{"level":"debug","ts":1674665219.2782335,"logger":"gitops","caller":"clustersmngr/factory.go:603","msg":"Updated namespace access cache for user","userNamespaces":{"default/vcluster":["flux-system"],"management":[]},"user":"id=\"alice\" groups=[foo] tokenLength=922","ttl":"30s"}

- We log out the namespaces we've guessed a user has access to
  so if thats quite wrong we can go straight to investigating RBAC
- Include user a bit more in the logs too
- Remove "attempt to read token from auth header" as its not that useful
@foot foot marked this pull request as ready for review January 25, 2023 16:46
- Again remove the JWTAuthorizationHeaderPrincipalGetter debug
  statement, its not very useful
@foot foot requested a review from a team January 30, 2023 11:48
@foot foot requested a review from a team January 30, 2023 11:50
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

Love the additional styling guidelines! ✨ Just a small question.

core/clustersmngr/factory.go Show resolved Hide resolved
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.

pkg/server/auth/auth_test.go Outdated Show resolved Hide resolved
pkg/server/middleware/middleware.go Outdated Show resolved Hide resolved
@lasomethingsomething
Copy link
Contributor

@foot Is this still in progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants