RI-6658: fallback to HELLO command when INFO is disabled via ACL #4377

KrumTy · 2025-02-17T16:38:09Z

Description

Issue: revoking access to INFO via ACL causes areas of the app not to work
Solution: fallback to HELLO command where possible, otherwise refactor logic

Notes

this PR addresses issues related to db connectivity when access to INFO is disabled
type-wise, the only mandatory field from the info response was 'version', something that's also present in the hello response
connecting to db only requires info command's response to check the redis' version (for recommendation and client info collect)

Updates

this PR evolved a bit as per @ArtemHoruzhenko 's suggestion
the logic for retrieving the info data has been unified inside the base client (see getInfo in redis.client.ts)
it basically checks for info, if no permissions are available - checks for data from hello;
in case hello is not available (redis v < 6.2.0) or something fails, defaults to a basic static object
the original PR was aimed at solving just RI-6849 but it so happens that it resolves multiple issues

redisinsight/api/src/utils/converter.ts

redisinsight/api/src/modules/database/providers/database-info.provider.ts

redisinsight/api/src/utils/converter.ts

ArtemHoruzhenko

I'm not sure that using DatabaseInfoProvider will be easy in all app modules. I assume we might have a lot of circular dependencies then. Let's move this logic to RedisClient so it will be available everywhere. wdyt?

redisinsight/api/src/utils/converter.ts

redisinsight/api/src/modules/database/providers/database-info.provider.ts

redisinsight/api/src/common/constants/general.ts

redisinsight/api/src/constants/error-messages.ts

redisinsight/api/src/modules/redis/client/redis.client.ts

ArtemHoruzhenko · 2025-02-21T11:58:38Z

redisinsight/api/src/modules/cluster-monitor/cluster-monitor.service.ts

-        ['info', 'server'],
-        { replyEncoding: 'utf8' },
-      ) as string);
+      const info = await client.getInfo(true, 'server');


Let's avoid getting various info sections avross the app.

Why though? Preserving the behavior is one thing, but I figured querying just specific sections should be faster. E.g. if we have an interval, querying the data and only carrying about one section it would make sense to only query that section. I might be wrong, let me know.

I don't think this slightly impact performance. Just don't like idea of merging objects in getInfo funciton.
If we want to stay with this logic we should enhance getInfo command to reply only particular section as it works now (most probably without storing it)

refactored; I've removed the stored info object

ArtemHoruzhenko · 2025-02-21T11:59:47Z

redisinsight/api/src/modules/database/providers/database-info.provider.ts

-        ['info', 'server'],
-        { replyEncoding: 'utf8' },
-      ) as string);
+      const reply = await client.getInfo();


ArtemHoruzhenko · 2025-02-21T12:01:38Z

redisinsight/api/src/modules/recommendation/providers/recommendation.provider.ts

-          { replyEncoding: 'utf8' },
-        ) as string,
-      );
+      const info = await redisClient.getInfo(true, 'clients');


ArtemHoruzhenko · 2025-02-21T12:01:46Z

redisinsight/api/src/modules/recommendation/providers/recommendation.provider.ts

-          { replyEncoding: 'utf8' },
-        ) as string,
-      );
+      const info = await redisClient.getInfo(true, 'server');


ArtemHoruzhenko · 2025-02-21T12:02:29Z

redisinsight/api/src/modules/redis/client/redis.client.ts

+   * @param infoSection - e.g. server, clients, memory, etc.
   */
-  public async getInfo(force = false): Promise<object> {
+  public async getInfo(force = true, infoSection?: string) {


let's remove infoSection

ArtemHoruzhenko · 2025-02-21T12:03:03Z

redisinsight/api/src/modules/redis/client/redis.client.ts

+        this.info = {
+          ...this.info,
+          ...infoData,
+        }


let's replace this with entirely new object we've got

ArtemHoruzhenko · 2025-02-21T12:03:43Z

redisinsight/api/src/modules/redis/utils/keys.util.ts

-        replyEncoding: 'utf8',
-      }) as string,
-    );
+    const info = await client.getInfo(true, 'keyspace');


keyspace here

…led via ACL (#4390) * RI-6848: add info_command_is_disabled to analytics when INFO is disabled via ACL * add isInfoCommandDisabled prop to RedisClient * preserve function signature * set default value for isInfoCommandDisabled * refactor getInfo * update: always call HELLO if INFO fails

pawelangelow

Tested and it looks OK. 🚀

Side note: I know that we merged few branches to this one, so this makes the whole change list bigger and it's hard to follow all the changes (big PR well known problem). I don't have perfect solution, but I'd rather prefer to merge them to main, than to a single branch that is merged back to main. (a.k.a. dont merge 3 > 2 > 1, but 1 to main, 2 to main and 3 to main). This way it's easier for me to see what happened.

Of course, the above is debatable, just saw it here and wanted to bring it up.

pd-redis · 2025-02-26T09:17:10Z

Tested and it looks OK. 🚀

Side note: I know that we merged few branches to this one, so this makes the whole change list bigger and it's hard to follow all the changes (big PR well known problem). I don't have perfect solution, but I'd rather prefer to merge them to main, than to a single branch that is merged back to main. (a.k.a. dont merge 3 > 2 > 1, but 1 to main, 2 to main and 3 to main). This way it's easier for me to see what happened.

Of course, the above is debatable, just saw it here and wanted to bring it up.

As long as 1,2,3 are individually mergable, yes! But if they don't make sense to be tested separately ...

ArtemHoruzhenko

lgtm

RI-6849: fallback to hello command when connecting to db

937b69a

KrumTy requested review from ArtemHoruzhenko, KIvanow, pawelangelow, seth-riedel, pd-redis and dantovska February 17, 2025 16:45

pawelangelow reviewed Feb 18, 2025

View reviewed changes

redisinsight/api/src/utils/converter.ts Outdated Show resolved Hide resolved

pawelangelow reviewed Feb 18, 2025

View reviewed changes

redisinsight/api/src/modules/database/providers/database-info.provider.ts Outdated Show resolved Hide resolved

RI-6849: refactor and test convertArrayOfKeyValuePairsToObject

3dcffeb

KrumTy requested a review from pawelangelow February 18, 2025 11:14

RI-6849: expose getRedisInfo method

2ae9be0

dantovska reviewed Feb 19, 2025

View reviewed changes

redisinsight/api/src/utils/converter.ts Outdated Show resolved Hide resolved

KrumTy mentioned this pull request Feb 19, 2025

[RI-6848] [RI-6842]: handle disabled INFO command for telemetry #4382

Closed

KrumTy changed the title ~~RI-6849: fallback to hello command when connecting to db~~ [RI-6849] [RI-6846]: fallback to hello command when connecting to db Feb 19, 2025

KrumTy changed the title ~~[RI-6849] [RI-6846]: fallback to hello command when connecting to db~~ [RI-6849] [RI-6846] [RI-6847]: fallback to hello command when connecting to db Feb 19, 2025

KrumTy requested a review from dantovska February 19, 2025 12:07

KrumTy mentioned this pull request Feb 19, 2025

RI-6844: hide db overview metrics when data is not available #4383

Merged

ArtemHoruzhenko reviewed Feb 19, 2025

View reviewed changes

redisinsight/api/src/utils/converter.ts Outdated Show resolved Hide resolved

redisinsight/api/src/modules/database/providers/database-info.provider.ts Outdated Show resolved Hide resolved

KrumTy added 5 commits February 20, 2025 14:14

RI-6849: refactor getInfo method, expose from client

c5bb1e2

remove convertArrayOfKeyValuePairsToObject

5f4230f

fix failing unit test

819e2a4

update test

e1c4e1b

add default redis info

7032266

KrumTy changed the title ~~[RI-6849] [RI-6846] [RI-6847]: fallback to hello command when connecting to db~~ [RI-6849] [RI-6842] [RI-6846] [RI-6847]: fallback to hello command when connecting to db Feb 20, 2025

KrumTy added 3 commits February 20, 2025 18:26

update static value

9e842b9

update default info

af075db

update default info

021f24b

KrumTy requested a review from ArtemHoruzhenko February 21, 2025 08:29

ArtemHoruzhenko reviewed Feb 21, 2025

View reviewed changes

KrumTy added 2 commits February 21, 2025 15:06

refactor getInfo

aa17555

fix failing test

6ee35d6

KrumTy requested a review from ArtemHoruzhenko February 21, 2025 13:30

KrumTy changed the title ~~[RI-6849] [RI-6842] [RI-6846] [RI-6847]: fallback to hello command when connecting to db~~ [RI-6849] [RI-6842] [RI-6845] [RI-6846] [RI-6847]: fallback to hello command when connecting to db Feb 21, 2025

KrumTy changed the title ~~[RI-6849] [RI-6842] [RI-6845] [RI-6846] [RI-6847]: fallback to hello command when connecting to db~~ [RI-6849] [RI-6842] [RI-6843] [RI-6845] [RI-6846] [RI-6847]: fallback to hello command when connecting to db Feb 25, 2025

KrumTy added 2 commits February 25, 2025 13:32

RI-6844: hide db overview metrics when data is not available (#4383)

62dbbe7

KrumTy changed the title ~~[RI-6849] [RI-6842] [RI-6843] [RI-6845] [RI-6846] [RI-6847]: fallback to hello command when connecting to db~~ RI-6658: fallback to hello command when connecting to db Feb 25, 2025

KrumTy changed the title ~~RI-6658: fallback to hello command when connecting to db~~ RI-6658: fallback to HELLO command when INFO is disabled via ACL Feb 25, 2025

pawelangelow approved these changes Feb 26, 2025

View reviewed changes

ArtemHoruzhenko approved these changes Feb 26, 2025

View reviewed changes

KrumTy merged commit dd6358b into main Feb 26, 2025
28 checks passed

KrumTy deleted the feature/RI-6658/no-perm-info-command branch February 26, 2025 09:24

RI-6658: fallback to HELLO command when INFO is disabled via ACL #4377

RI-6658: fallback to HELLO command when INFO is disabled via ACL #4377

Uh oh!

Conversation

KrumTy commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes

Updates

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ArtemHoruzhenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrumTy Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawelangelow left a comment

Choose a reason for hiding this comment

Uh oh!

pd-redis commented Feb 26, 2025

Uh oh!

ArtemHoruzhenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KrumTy commented Feb 17, 2025 •

edited

Loading

KrumTy Feb 21, 2025 •

edited

Loading