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

AP-5015 extend redis config sanitization #310

Merged
merged 4 commits into from
Aug 30, 2024

Conversation

kjamrog
Copy link
Contributor

@kjamrog kjamrog commented Aug 30, 2024

Changes

  • Adds maxRetriesPerRequest: null parameter to RedisConfig sanitization
  • Adds createSanitizedRedisClient utility method for creating bullmq compatible redis client

Checklist

  • Apply one of following labels; major, minor, patch or skip-release
  • I've updated the documentation, or no changes were necessary
  • I've updated the tests, or no changes were necessary

@kjamrog kjamrog added the minor label Aug 30, 2024
import Redis from 'ioredis'
import { sanitizeRedisConfig } from '../utils'

export const createSanitizedRedisClient = (redisConfig: RedisConfig): Redis =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some places we will still want to operate on Redis instance, not on config, while taking advantage of sanitization step. Therefore I think it's useful to have function that allows to create such instance. Let me know if you disagree

Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't have a factory per se here, just a one-liner wrapper.

what is the added value on top of the sanitizeRedisConfig method? Can't user apply sanitizeRedisConfig on their own when creating a new instance?

I'm not against having it, primarily not convinced that it deserves a separate file. can be just part of utils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I added it as a part of utils, but those are internal and not exported, so finally decided to put public one in separate file. I noticed in UPGRADING.md that with recent version sanitizeRedisConfig was explicitly excluded from public ones, not sure why.
But I can put it back to utils and export that one function if you think it's better, no strong opinion from my side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding sanitizeRedisConfig - sure, it could be exported and used as well, I was just thinking that if it's always used just for new Redis(sanitizedConfig), we can already have it wrapped. Also no strong preference :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be in favour of exporting the sanitization method, but also I'm not against having a separate method for instantiation, we can have both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to utils and exported those 2 metrics

@kjamrog kjamrog marked this pull request as ready for review August 30, 2024 14:49
@@ -7,3 +7,4 @@ export * from './processors/types'
export * from './processors/AbstractBackgroundJobProcessor'
export * from './processors/FakeBackgroundJobProcessor'
export * from './spy/types'
export { sanitizeRedisConfig, createSanitizedRedisClient } from './utils'
Copy link
Collaborator

@CarlosGamero CarlosGamero Aug 30, 2024

Choose a reason for hiding this comment

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

Stupid question, but where will we use this? just wondering if we need this exposed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One example is in fastify-extras, we have 2 queue discoverers there: one uses backgroundJobProcessorGetActiveQueueIds from background-jobs-common, another one operates directly on redis instance, and there we could still benefit from sanitized instance instead of creating one with new Redis.

Also, in Flow, we always create Redis manually from config, like: new Redis(redisConfig). I think in the follow up PR's we could replace it with createSanitizedRedisClient to ensure that required properties are always there.

Does it make sense?

Copy link
Collaborator

@CarlosGamero CarlosGamero Aug 30, 2024

Choose a reason for hiding this comment

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

For Fastify extras, I agree completely and it makes a lot of sense.

On Flow side, I am not sure if we want to sanitaise redis config, it is a requirement for BullMQ and the library is already taking care that those connections are sanitized, I think Flow should decide on those parameters base on needs

@kjamrog kjamrog merged commit b752018 into main Aug 30, 2024
5 checks passed
@kjamrog kjamrog deleted the AP-5015_extend_redis_config_sanitization branch August 30, 2024 15:23
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