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

Jump to bottom

feat: mutator tracing #1050

Merged
merged 14 commits into from
Jan 17, 2023
Merged

feat: mutator tracing #1050

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
fa91807
refactor: add tracing on hydrator
daviddelucca Jan 5, 2023
137fce8
chore: remove commented code
daviddelucca Jan 5, 2023
661bf6a
chore: test pipeline
daviddelucca Jan 5, 2023
7c7f21f
chore: fix break test
daviddelucca Jan 5, 2023
fed7446
chore: rename struct attributes
daviddelucca Jan 5, 2023
90433ab
fix: refactor tracer injection in mutator
alnr Jan 6, 2023
9ec9bd6
refactor: update oathkeeper json schema (#1051)
daviddelucca Jan 6, 2023
ab1c460
fix: revert test change
alnr Jan 9, 2023
e78b0d7
chore: bump ory/x
alnr Jan 9, 2023
6727875
Merge branch 'master' into refactor/mutator-tracing
alnr Jan 9, 2023
6f0cf84
fix: typos and reorganize code
alnr Jan 11, 2023
2ea63ad
fix: tracing config schema
alnr Jan 11, 2023
951200a
feat: import tracing config schema instead of specifying it here
alnr Jan 12, 2023
ddda29c
feat: copy in tracing schema
alnr Jan 13, 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
2 changes: 0 additions & 2 deletions driver/registry_memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ type RegistryMemory struct {
errors map[string]pe.Handler

healthEventManager *health.DefaultHealthEventManager

ruleRepositoryLock sync.Mutex
}

func (r *RegistryMemory) Init() {
Expand Down
2 changes: 1 addition & 1 deletion pipeline/authn/authenticator_oauth2_introspection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ func TestAuthenticatorOAuth2Introspection(t *testing.T) {
assertHandlerWasCalled(t)

t.Run("case=request succeeds and uses the cache", func(t *testing.T) {
config := setup(t, `{ "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
config := setup(t, `{ "required_scope": ["scope-a"], "trusted_issuers": ["foo", "bar"], "target_audience": ["audience"] }`)
alnr marked this conversation as resolved.
Show resolved Hide resolved
sess := new(AuthenticationSession)

err = a.Authenticate(r, sess, config, nil)
Expand Down
35 changes: 13 additions & 22 deletions pipeline/mutate/mutator_hydrator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,14 @@ import (
"time"

"github.com/dgraph-io/ristretto"

"github.com/ory/oathkeeper/pipeline/authn"
"github.com/ory/oathkeeper/x"

"github.com/ory/x/httpx"

"github.com/pkg/errors"
"go.opentelemetry.io/otel/trace"

"github.com/ory/oathkeeper/driver/configuration"
"github.com/ory/oathkeeper/pipeline"
"github.com/ory/oathkeeper/pipeline/authn"
"github.com/ory/oathkeeper/x"
"github.com/ory/x/httpx"
)

const (
Expand All @@ -37,12 +35,10 @@ const (
)

type MutatorHydrator struct {
c configuration.Provider
client *http.Client
d mutatorHydratorDependencies
c configuration.Provider
d mutatorHydratorDependencies

hydrateCache *ristretto.Cache
cacheTTL *time.Duration
}

type BasicAuth struct {
Expand Down Expand Up @@ -79,6 +75,7 @@ type MutatorHydratorConfig struct {

type mutatorHydratorDependencies interface {
x.RegistryLogger
Tracer() trace.Tracer
}

func NewMutatorHydrator(c configuration.Provider, d mutatorHydratorDependencies) *MutatorHydrator {
Expand All @@ -93,7 +90,6 @@ func NewMutatorHydrator(c configuration.Provider, d mutatorHydratorDependencies)
return &MutatorHydrator{
c: c,
d: d,
client: httpx.NewResilientClient().StandardClient(),
hydrateCache: cache,
}
}
Expand Down Expand Up @@ -172,33 +168,28 @@ func (a *MutatorHydrator) Mutate(r *http.Request, session *authn.AuthenticationS
}
req.Header.Set(contentTypeHeaderKey, contentTypeJSONHeaderValue)

var client *http.Client

clientOpts := []httpx.ResilientOptions{httpx.ResilientClientWithTracer(a.d.Tracer())}
if cfg.Api.Retry != nil {
maxRetryDelay := time.Second
giveUpAfter := time.Millisecond * 50
if len(cfg.Api.Retry.MaxDelay) > 0 {
maxRetryDelay := time.Second
Copy link
Member

Choose a reason for hiding this comment

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

IMO this needs to be moved one scope up, like it was before, so that the default is also set when the config is not set.

Copy link
Member

Choose a reason for hiding this comment

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

And we need a test for this, so e.g. only setting cfg.Api.Retry.MaxDelay, and expecting the giveUpAfter default to be set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After looking at this in more detail, the retry config was completely broken in 2a97e05.

I'll revisit this...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alrighty... the retry configuration is broken all over Oathkeeper. Here's my understanding of what is currently happening:

  • The give_up_after property specifies the per-retry timeout. That's surprising to me, since it sounds like it means the overall timeout, including retries.
  • The max_delay specifies the maximum backoff wait time in between retries.
  • The maximum number of retries is not configurable, and is always 4 (the default in hashicorp/retryablehttp).
  • This config migration is incorrect. It thinks give_up_after is the overall timeout (including retries).

Not sure now is the time to fix this fully, since that could also break users.

With the goal of moving this PR over the line, I propose to keep as much backward-compatibility as possible. This particular mutator previously had no overall timeout and no retries if the retry config was not specified at all. If the retry config was specified (even if {}), it peformed up to 4 retries. If the retry config is empty ({}), the timeouts also changed.

This patch keeps that behavior, while fixing bugs:

  • the fallback timeouts (retry config present but empty) were wrong: 50ms timeout and 1s delay, even though the config schema promises a more reasonable 1s timeout and 100ms delay. Those defaults now at least work as advertised.
  • faulty error messages

We should consider revamping the timeout handling all over Oathkeeper in a follow-up ticket.

Thoughts? @zepatrik @aeneasr @daviddelucca ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR is related to activate tracing on mutator. I think we could merge it and open another ticket/branch to work on revamping on timeouts.

Does make sense?

if d, err := time.ParseDuration(cfg.Api.Retry.MaxDelay); err != nil {
a.d.Logger().WithError(err).Warn("Unable to parse max_delay in the Hydrator Mutator, falling pack to default.")
zepatrik marked this conversation as resolved.
Show resolved Hide resolved
} else {
maxRetryDelay = d
}
clientOpts = append(clientOpts, httpx.ResilientClientWithMaxRetryWait(maxRetryDelay))
}
if len(cfg.Api.Retry.GiveUpAfter) > 0 {
giveUpAfter := time.Millisecond * 50
zepatrik marked this conversation as resolved.
Show resolved Hide resolved
if d, err := time.ParseDuration(cfg.Api.Retry.GiveUpAfter); err != nil {
a.d.Logger().WithError(err).Warn("Unable to parse max_delay in the Hydrator Mutator, falling pack to default.")
} else {
giveUpAfter = d
}
clientOpts = append(clientOpts, httpx.ResilientClientWithConnectionTimeout(giveUpAfter))
}

client = httpx.NewResilientClient(
httpx.ResilientClientWithMaxRetryWait(maxRetryDelay),
httpx.ResilientClientWithConnectionTimeout(giveUpAfter),
).StandardClient()
} else {
client = http.DefaultClient
}
client := httpx.NewResilientClient(clientOpts...).StandardClient()

res, err := client.Do(req.WithContext(r.Context()))
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions pipeline/mutate/mutator_hydrator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"encoding/json"
"errors"
"fmt"
"io/ioutil"
"io"
"net/http"
"net/http/httptest"
"net/url"
Expand Down Expand Up @@ -64,7 +64,7 @@ func defaultRouterSetup(actions ...func(a *authn.AuthenticationSession)) routerS
return func(t *testing.T) http.Handler {
router := httprouter.New()
router.POST("/", func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
body, err := ioutil.ReadAll(r.Body)
body, err := io.ReadAll(r.Body)
require.NoError(t, err)
var data authn.AuthenticationSession
err = json.Unmarshal(body, &data)
Expand Down
56 changes: 7 additions & 49 deletions spec/config.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1915,69 +1915,27 @@
"sampling": {
"type": "object",
"propertyNames": {
"enum": ["type", "value", "server_url"]
"enum": ["server_url", "trace_id_ratio"]
},
alnr marked this conversation as resolved.
Show resolved Hide resolved
"allOf": [
alnr marked this conversation as resolved.
Show resolved Hide resolved
{
"oneOf": [
{
"properties": {
"type": {
"description": "The type of the sampler you want to use.",
"const": "const"
},
"value": {
"type": "integer",
"description": "The value passed to the sampler type that has been configured.",
"minimum": 0,
"maximum": 1
}
}
},
{
"properties": {
"type": {
"description": "The type of the sampler you want to use.",
"const": "rateLimiting"
},
"value": {
"type": "integer",
"description": "The value passed to the sampler type that has been configured.",
"minimum": 0
}
}
},
{
"properties": {
"type": {
"description": "The type of the sampler you want to use.",
"const": "probabilistic"
},
"value": {
"type": "number",
"description": "The value passed to the sampler type that has been configured.",
"minimum": 0,
"maximum": 1
}
}
}
]
},
{
"properties": {
"server_url": {
"type": "string",
"description": "The address of jaeger-agent's HTTP sampling server",
"format": "uri"
},
"trace_id_ratio": {
"type": "number",
"description": "The address of jaeger-agent's HTTP sampling server"
}
}
}
],
"examples": [
{
"type": "const",
"value": 1,
"server_url": "http://localhost:5778/sampling"
"server_url": "http://localhost:5778/sampling",
"trace_id_ratio": 1
}
]
}
Expand Down
6 changes: 6 additions & 0 deletions x/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package x

import (
"go.opentelemetry.io/otel/trace"

"github.com/ory/x/logrusx"

"github.com/ory/herodot"
Expand All @@ -15,6 +17,10 @@ func (lp *TestLoggerProvider) Logger() *logrusx.Logger {
return logrusx.New("", "")
}

func (lp *TestLoggerProvider) Tracer() trace.Tracer {
return nil
}

type RegistryLogger interface {
Logger() *logrusx.Logger
}
Expand Down