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

filters/auth: add jwtMetrics #3020

Merged
merged 1 commit into from
Apr 18, 2024
Merged

Conversation

AlexanderYastrebov
Copy link
Member

@AlexanderYastrebov AlexanderYastrebov commented Apr 15, 2024

Add jwtMetrics filter that parses JWT token and increments a set of counters,
see documentation for details.

@AlexanderYastrebov AlexanderYastrebov added the minor no risk changes, for example new filters label Apr 15, 2024
@AlexanderYastrebov AlexanderYastrebov added the wip work in progress label Apr 15, 2024
@AlexanderYastrebov AlexanderYastrebov force-pushed the filters/auth/jwtMetrics branch 12 times, most recently from 53208f9 to 327394e Compare April 17, 2024 12:43
@AlexanderYastrebov AlexanderYastrebov marked this pull request as ready for review April 17, 2024 12:46
@AlexanderYastrebov AlexanderYastrebov force-pushed the filters/auth/jwtMetrics branch 2 times, most recently from 6a235e0 to a2c8e8a Compare April 17, 2024 13:02
@AlexanderYastrebov AlexanderYastrebov removed the wip work in progress label Apr 17, 2024
metrics.IncCounter(metricsPrefix + "missing-issuer")
} else if !slices.Contains(f.Issuers, issuer) {
metrics.IncCounter(metricsPrefix + "invalid-issuer")
}
Copy link
Member

Choose a reason for hiding this comment

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

You do not count valid tokens, why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its not a goal to reveal valid tokens, besides we don't validate so we could only know that it parses as JWT token.

Copy link
Member

Choose a reason for hiding this comment

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

As a user I would wonder why we do not have a counter for valid tokens

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to count configured claims (e.g. token realms) but this may be implemented later.

Copy link
Member Author

Choose a reason for hiding this comment

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

At this stage jwtMetrics is essentially invalidJwtMetrics because it only counts wrong tokens. I do not want to put "invalid" into the name and we may extend it in the future to count other claims.

Copy link
Member

Choose a reason for hiding this comment

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

ok

Add jwtMetrics filter that parses JWT token and increments a set of counters,
see documentation for details.

Signed-off-by: Alexander Yastrebov <[email protected]>
@szuecs
Copy link
Member

szuecs commented Apr 18, 2024

👍

1 similar comment
@AlexanderYastrebov
Copy link
Member Author

👍

@AlexanderYastrebov AlexanderYastrebov merged commit e852b14 into master Apr 18, 2024
14 checks passed
@AlexanderYastrebov AlexanderYastrebov deleted the filters/auth/jwtMetrics branch April 18, 2024 17:07
AlexanderYastrebov added a commit that referenced this pull request Apr 19, 2024
Extend configuration of `jwtMetrics` (#3020) to support opt-out -
disable when any of the configured route annotations (#3022) is present.

This can be used to collect data about missing/invalid JWT tokens per hostname
in multitenant ingress setup.

Add `jwtMetrics` filter to all routes using `-default-filters-append` flag and
allow users to annotate routes that do not require JWT token.

Signed-off-by: Alexander Yastrebov <[email protected]>
AlexanderYastrebov added a commit that referenced this pull request Apr 19, 2024
Extend configuration of `jwtMetrics` (#3020) to support opt-out -
disable metrics collection when any of the configured
route annotations (#3022) is present.

This can be used to collect data about missing/invalid JWT tokens per hostname
in multitenant ingress setup.

Add `jwtMetrics` filter to all routes using `-default-filters-append` flag and
allow users to annotate routes that do not require JWT token.

Signed-off-by: Alexander Yastrebov <[email protected]>
AlexanderYastrebov added a commit that referenced this pull request Apr 22, 2024
Extend configuration of `jwtMetrics` (#3020) to support opt-out -
disable metrics collection when any of the configured
route annotations (#3022) is present.

This can be used to collect data about missing/invalid JWT tokens per hostname
in multitenant ingress setup.

Add `jwtMetrics` filter to all routes using `-default-filters-append` flag and
allow users to annotate routes that do not require JWT token.

Signed-off-by: Alexander Yastrebov <[email protected]>
AlexanderYastrebov added a commit that referenced this pull request Apr 22, 2024
Extend configuration of `jwtMetrics` (#3020) to support opt-out -
disable metrics collection when any of the configured
route annotations (#3022) is present.

This can be used to collect data about missing/invalid JWT tokens per hostname
in multitenant ingress setup.

Add `jwtMetrics` filter to all routes using `-default-filters-append` flag and
allow users to annotate routes that do not require JWT token.

Signed-off-by: Alexander Yastrebov <[email protected]>
AlexanderYastrebov added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Apr 22, 2024
[Changes](zalando/skipper@v0.21.54...v0.21.61)

* [build(deps): bump amazonlinux from `ef9435f` to `5478f82` in /fuzz](zalando/skipper#3031)
* [build(deps): bump actions/upload-artifact from 4.3.1 to 4.3.2](zalando/skipper#3033)
* [build(deps): bump actions/checkout from 4.1.2 to 4.1.3](zalando/skipper#3032)
* [build(deps): bump github.com/miekg/dns from 1.1.58 to 1.1.59](zalando/skipper#3030)
* [proxy: support configurable metrics](zalando/skipper#3027)
* [filters/auth: add login redirect stub support](zalando/skipper#3028)
* [filters: move annotate into own package](zalando/skipper#3023)
* [filters/auth: add jwtMetrics](zalando/skipper#3020)
* [filters/builtin: add annotate filter](zalando/skipper#3022)

Signed-off-by: Alexander Yastrebov <[email protected]>
AlexanderYastrebov added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Apr 22, 2024
[Changes](zalando/skipper@v0.21.54...v0.21.61)

* `build(deps): bump amazonlinux from `ef9435f` to `5478f82` in /fuzz` zalando/skipper#3031
* `build(deps): bump actions/upload-artifact from 4.3.1 to 4.3.2` zalando/skipper#3033
* `build(deps): bump actions/checkout from 4.1.2 to 4.1.3` zalando/skipper#3032
* `build(deps): bump github.com/miekg/dns from 1.1.58 to 1.1.59` zalando/skipper#3030
* `proxy: support configurable metrics` zalando/skipper#3027
* `filters/auth: add login redirect stub support` zalando/skipper#3028
* `filters: move annotate into own package` zalando/skipper#3023
* `filters/auth: add jwtMetrics` zalando/skipper#3020
* `filters/builtin: add annotate filter` zalando/skipper#3022

Signed-off-by: Alexander Yastrebov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor no risk changes, for example new filters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants