[NAE-2246] Enable Redis TLS & Configure Redis Sentinel #377
Conversation
- Updated parent POM version from `7.0.0-RC8.1` to `7.0.0-RC9` across all module POM files.
- Introduced properties for Redis Sentinel and SSL configurations to improve connectivity options and security. - Updated session and security management to use enhanced Redis configuration structure. - Refactored Redis property handling and connection factory creation for better maintainability and support of new features.
- add ssl configuration for redis client
tuplle
added
improvement
WalkthroughNest session-related Redis properties under a new Changes
Sequence DiagramsequenceDiagram
participant Caller
participant SessionConfig
participant DataProps
participant JedisFactory
Caller->>SessionConfig: request JedisConnectionFactory
SessionConfig->>DataProps: getRedisProperties()
DataProps-->>SessionConfig: RedisProperties (includes .session)
alt sentinel.master configured
SessionConfig->>SessionConfig: redisSentinelConfiguration()
Note right of SessionConfig `#D6EAF8`: parse sentinel nodes\nuse sentinel master + credentials
SessionConfig->>JedisFactory: build with RedisSentinelConfiguration + JedisClientConfiguration
else standalone
SessionConfig->>SessionConfig: standaloneRedisConfiguration()
Note right of SessionConfig `#FDEBD0`: use host/port + optional credentials
SessionConfig->>JedisFactory: build with RedisStandaloneConfiguration + JedisClientConfiguration
end
alt ssl enabled
SessionConfig->>JedisFactory: enable SSL in JedisClientConfiguration
end
JedisFactory-->>SessionConfig: configured JedisConnectionFactory
SessionConfig-->>Caller: return factory
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/AbstractSecurityConfiguration.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/manager/service/SessionManagerService.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (11)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java (3)
34-40: LGTM! Conditional logic is sound.The conditional selection between Sentinel and standalone configurations based on master name is appropriate and handles both null and empty string cases correctly.
81-86: LGTM! SSL configuration is clean and correct.The conditional SSL configuration logic is straightforward and appropriate.
8-11: LGTM! New imports are necessary and used.The added imports support the new Sentinel configuration functionality and are all utilized in the code.
Also applies to: 18-18
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java (5)
18-18: LGTM! Import is necessary for the new Redis constants.The
RedisNodeimport is used forDEFAULT_PORTandDEFAULT_SENTINEL_PORTconstants in the Redis configuration.
109-111: LGTM! Namespace access correctly updated to nested session property.The namespace is now properly accessed via the nested
sessionproperty, aligning with the broader Redis properties restructuring.
664-664: LGTM! Good use of constants and new SSL flag.Using
RedisNode.DEFAULT_PORTinstead of a magic number improves maintainability. The addition of thesslflag enables secure Redis connections.Also applies to: 684-684
751-770: LGTM! Session properties class is well-structured.The
EngineRedisSessionPropertiesclass correctly extendsRedisSessionPropertiesand properly uses@EqualsAndHashCode(callSuper = true)to handle inheritance. The added fields for session limiting and filtering are clearly defined.
651-651: Review comment is incorrect—the configuration structure does not match the claimed breaking change.The review comment incorrectly claims the prefix changed from
netgrif.engine.sessiontonetgrif.engine.data.redis. However, verification of the codebase shows:
- No "netgrif.engine.session" prefix exists – Search found zero references to this prefix anywhere in the Java codebase.
- Actual structure –
RedisPropertiesuses prefixnetgrif.engine.data.rediswith session configuration as a nested property (accessed viaredis.getSession()).- Correct configuration – Users configure
netgrif.engine.data.redis.hostandnetgrif.engine.data.redis.session.*properties, notnetgrif.engine.session.host.The migration examples provided in the original comment are misleading and do not reflect actual configuration changes.
Likely an incorrect or invalid review comment.
application-engine/src/main/java/com/netgrif/application/engine/manager/service/SessionManagerService.java (1)
31-31: LGTM! Namespace access correctly updated for nested session property.The Redis username key construction now correctly accesses the namespace via
redisProperties.getSession().getNamespace(), aligning with the restructured Redis configuration.application-engine/src/main/java/com/netgrif/application/engine/configuration/AbstractSecurityConfiguration.java (2)
223-228: LGTM! Session configuration correctly updated to use nested properties.The session limit configuration now correctly accesses
isEnabledLimitSession()andgetMaxSession()via the nested session property, aligning with the restructured Redis configuration.
232-235: LGTM! Filter configuration correctly updated to use nested properties.The filter configuration now correctly accesses
isEnabledFilter()via the nested session property, aligning with the restructured Redis configuration.
...ava/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java
Outdated
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java
Show resolved
Hide resolved
tuplle
changed the title
- Introduced `DEFAULT_SENTINEL_NODE` constant for default node configuration. - Added a helper method `hasCredentials` to improve credential checks. - Refactored sentinel node parsing to handle edge cases with better logging.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.java (1)
Configuration(11-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
...ava/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
Outdated
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java
Outdated
Show resolved
Hide resolved
- Corrected Redis session namespace property to align with enhanced configuration standards. - Ensured consistency across static and non-static session configurations.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java (2)
application-engine/src/main/java/com/netgrif/application/engine/manager/service/SessionManagerService.java (1)
Slf4j(20-71)application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.java (1)
Configuration(11-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: task-list-completed
🔇 Additional comments (6)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java (6)
5-5: LGTM!The new imports and
@Slf4jannotation are properly added to support Redis Sentinel configuration, SSL enablement, and logging of node parsing failures.Also applies to: 9-10, 12-12, 19-19, 21-21
36-42: LGTM!The routing logic correctly discriminates between Sentinel and standalone modes based on the presence of a configured master name. The implementation is null-safe and provides clear separation between the two configuration paths.
44-54: LGTM!The standalone Redis configuration is well-structured with appropriate defaults (localhost, DEFAULT_PORT), optional credential handling via the
hasCredentials()helper, and SSL support through the sharedjedisClientConfiguration()method.
56-80: LGTM!The Sentinel configuration properly handles master and sentinel node setup with appropriate fallback for node parsing failures. The implementation correctly supports separate credentials for master and sentinel nodes, which is important for production deployments. The try-catch approach with logging on line 63 provides good visibility for troubleshooting configuration issues.
82-87: LGTM!The SSL configuration method is clean and correctly shared between standalone and Sentinel configurations, ensuring consistent SSL behavior across both deployment modes.
89-92: LGTM!The credential validation helper eliminates code duplication and uses
isBlank()for more robust validation (catches null, empty, and whitespace-only strings). This addresses the refactoring suggested in the past review comments.
...-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java
Show resolved
Hide resolved
...ain/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.java
Outdated
Show resolved
Hide resolved
machacjozef
changed the base branch from
release/7.0.0-rev9
to
release/7.0.0-rev8
# Conflicts: # application-engine/pom.xml # nae-object-library/pom.xml # nae-spring-core-adapter/pom.xml # nae-user-ce/pom.xml # nae-user-common/pom.xml # pom.xml
- Updated Redis session namespace initialization to handle default blank or null cases more effectively. - Aligned application properties and session configuration with the updated namespace standards. - Ensured consistency between YAML configuration and Java annotations.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(6 hunks)application-engine/src/main/resources/application.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/resources/application.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.java (1)
12-12: Namespace placeholder alignment looks good.The annotation now pulls from the same
netgrif.engine.data.redis.session.namespaceproperty as the runtime configuration, so both paths stay in sync.
| String namespace = redis.getSession().getNamespace(); | ||
| if (namespace == null || namespace.isBlank() || "spring:session".equals(namespace)) { | ||
| redis.getSession().setNamespace("spring:session:" + databaseName); | ||
| } |
There was a problem hiding this comment.
Avoid double spring:session prefix in fallback.
Now that EnableRedisIndexedHttpSession already expands the namespace as spring:session:${...}, this fallback still prepends spring:session: when the bound value is the parent default (spring:session). In configs that omit netgrif.engine.data.redis.session.namespace, you’ll end up with spring:session:spring:session:<dbName>, so the static session config and the runtime repository will read/write different keys. Please leave the fallback as the bare suffix (e.g., set it to databaseName) and let the annotation (and any other callers) add the spring:session: prefix exactly once.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
around lines 109 to 112, the fallback currently prepends "spring:session:"
producing duplicates when the bound value equals the parent default
"spring:session"; change the fallback to set only the bare databaseName (i.e.,
set namespace to databaseName) so the framework/annotation can apply the
"spring:session:" prefix exactly once, leaving the conditional logic
(null/blank/"spring:session") intact.
Description
Implements connectio for Redi Sentinel deployments and enables SSL configuration for Redis communication. It redefines the Engine properties for Redis for additional Redis Sentinel configuration.
Implements NAE-2246
Dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
Configuration where tested on two Redis deployments on local and DEV kubernetes cluster, with and without Redis Sentinel. For deployment, bitnami helm chart of Redis were used.
Test Configuration
Checklist:
Summary by CodeRabbit