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

PROD-54189 Fix regex configuration via environment variables #113

Closed
wants to merge 2 commits into from

Conversation

ChristosLabrou
Copy link
Contributor

When an expected Regex would be passed as a string (which happens when setting regexes via environment variables) then riviere would throw in specific points and not log requests.

@nikostoulas
Copy link
Contributor

@ChristosLabrou why is this happening? I see that orka does the wiring of env vars to regexp. Riviere isn't the place to do this. Is sth missing from here: https://github.com/Workable/orka/blob/master/src/initializers/riviere.ts?

Copy link
Contributor

@nikostoulas nikostoulas left a comment

Choose a reason for hiding this comment

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

Let's make any needed changes to orka. Riviere is expecting a js object as configuration so passing string where riviere expects a regex makes little sense

@ChristosLabrou
Copy link
Contributor Author

@ChristosLabrou why is this happening? I see that orka does the wiring of env vars to regexp. Riviere isn't the place to do this. Is sth missing from here: https://github.com/Workable/orka/blob/master/src/initializers/riviere.ts?

This was my initial attempt but didn't work when testing. There is an override for outbound which would override any casting of outbound.blacklistedPathRegex to RegExp instance and convert it back to string. Riviere already has this behavior for headerRegex even with Orka converting to RegExp instance too

@nikostoulas
Copy link
Contributor

nikostoulas commented Feb 10, 2025

@ChristosLabrou why is this happening? I see that orka does the wiring of env vars to regexp. Riviere isn't the place to do this. Is sth missing from here: https://github.com/Workable/orka/blob/master/src/initializers/riviere.ts?

This was my initial attempt but didn't work when testing. There is an override for outbound which would override any casting of outbound.blacklistedPathRegex to RegExp instance and convert it back to string.

That orka is passing configuration wrongly is orka's issue to fix.

Riviere already has this behavior for [headerRegex]
(

if (new RegExp(headersRegex).test(header)) {
) even with Orka converting to RegExp instance too

Possibly leftover from older implementations. Doesn't justify repeating it. (E.g note that in an initial implementation default option was a string here: c9531a1)

@klesgidis
Copy link
Contributor

It is not just orka's issue as far as I understand. When you override the regex from helm release, whatever transformation you do in orka, here you will have a string

cc: @nikostoulas

@nikostoulas
Copy link
Contributor

It is not just orka's issue as far as I understand. When you override the regex from helm release, whatever transformation you do in orka, here you will have a string

cc: @nikostoulas

@klesgidis No. Riviere is initialized with code not from env. So it's the code's responsibility to pass a regex. This code for our apps lies in orka.

@klesgidis
Copy link
Contributor

oh you're right

@ChristosLabrou
Copy link
Contributor Author

Closing this. Will open PR in Orka instead as discussed

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

Successfully merging this pull request may close these issues.

3 participants