-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feature: add redis resource access for PAM #5085
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
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Greptile SummaryThis PR adds Redis resource support to the PAM system following the established architecture for other gateway-based resources like PostgreSQL and MySQL. The implementation includes backend factory pattern integration, schemas, types, frontend forms, and API route updates. The backend implementation is solid with proper hostname validation, SSL/TLS support through gateway proxying, and appropriate error handling. However, there is a maintainability concern: the Key changes:
Confidence Score: 4/5
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
25 files reviewed, 1 comment
frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/RedisAccountForm.tsx
Outdated
Show resolved
Hide resolved
aebaa29 to
7a87fdc
Compare
x032205
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great, just a few comments!
Also sent some feedback in DMs regarding these concepts:
- Adding accounts without username/password for unauthorized connections
- Formatting session logs to show commands and outputs in a neater format
frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/RedisAccountForm.tsx
Outdated
Show resolved
Hide resolved
backend/src/ee/services/pam-resource/redis/redis-resource-factory.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/RedisAccountForm.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/RedisAccountForm.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/RedisAccountForm.tsx
Outdated
Show resolved
Hide resolved
...end/src/pages/pam/PamResourcesPage/components/PamResourceForm/shared/RedisResourceFields.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/pam/PamResourcesPage/components/PamResourceForm/RedisResourceForm.tsx
Outdated
Show resolved
Hide resolved
7a87fdc to
1fe781a
Compare
f4348e8 to
d8db03d
Compare
Context
ref: https://linear.app/infisical/issue/PAM-42/add-redis-account-type-for-pam
Screenshots
Steps to verify the change
To test this feature, one needs to run a Redis server locally. First, let's see the example redis config file:
With that, we can first have a redis server running without TLS:
For the TLS version, you can first generate the certs with:
then with the compose:
the testing process is pretty much largely the same as other PAM resources.
redis -u <infisical cli proxy_url>to test it. Some corner cases, such as pub/sub cmds, monitors are a bit special in terms of how our proxy handles them.Type