Abort server startup on invalid SolutionTemplate configuration#3159
Abort server startup on invalid SolutionTemplate configuration#3159havetisyan merged 4 commits intoAthenZ:masterfrom
Conversation
…Template Signed-off-by: Go Yakami <gyakami@lycorp.co.jp>
…tion templates Signed-off-by: Go Yakami <gyakami@lycorp.co.jp>
…embers and trusted roles ZMSImplTest: add test for loading solution templates with null roles solution_templates.json: add templates for testing role members addition and trust settings Signed-off-by: Go Yakami <gyakami@lycorp.co.jp>
Summary of ChangesHello @gyakami, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and reliability of the server by introducing critical validation and warning mechanisms for SolutionTemplate configurations. It prevents the server from initializing with SolutionTemplates that define roles with conflicting 'Trust' and 'RoleMembers' attributes, thereby ensuring data integrity and system stability. Furthermore, it provides runtime warnings when SolutionTemplates are applied in a manner that would create these conflicting states on existing roles, empowering administrators to proactively identify and rectify potential misconfigurations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces important validation to prevent conflicting configurations in SolutionTemplates, specifically ensuring a role does not have both trust and roleMembers defined. It also adds warnings for applying templates that could lead to such conflicts. The changes are well-implemented and include corresponding tests. My review includes suggestions to improve code readability and maintainability by refactoring nested conditional statements and optimizing map iterations. I've also noted an opportunity to reduce code duplication in the new test methods.
| for (String templateName : serverSolutionTemplates.getTemplates().keySet()) { | ||
| Template template = serverSolutionTemplates.get(templateName); | ||
| if (template.getRoles() == null) { | ||
| continue; | ||
| } | ||
| for (Role role : template.getRoles()) { | ||
| if (!StringUtil.isEmpty(role.getTrust()) && role.getRoleMembers() != null && !role.getRoleMembers().isEmpty()) { | ||
| LOG.error("Solution Template {} role {} has both trust and members defined. Exiting...", | ||
| templateName, role.getName()); | ||
| throw new RuntimeException("Solution Template " + templateName + " role " + role.getName() + " has both trust and members defined"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
For better performance and code style, it's recommended to iterate over the map's entrySet instead of its keySet. This avoids the extra get() call for each key in the loop.
| for (String templateName : serverSolutionTemplates.getTemplates().keySet()) { | |
| Template template = serverSolutionTemplates.get(templateName); | |
| if (template.getRoles() == null) { | |
| continue; | |
| } | |
| for (Role role : template.getRoles()) { | |
| if (!StringUtil.isEmpty(role.getTrust()) && role.getRoleMembers() != null && !role.getRoleMembers().isEmpty()) { | |
| LOG.error("Solution Template {} role {} has both trust and members defined. Exiting...", | |
| templateName, role.getName()); | |
| throw new RuntimeException("Solution Template " + templateName + " role " + role.getName() + " has both trust and members defined"); | |
| } | |
| } | |
| } | |
| for (Map.Entry<String, Template> entry : serverSolutionTemplates.getTemplates().entrySet()) { | |
| final String templateName = entry.getKey(); | |
| final Template template = entry.getValue(); | |
| if (template.getRoles() == null) { | |
| continue; | |
| } | |
| for (Role role : template.getRoles()) { | |
| if (!StringUtil.isEmpty(role.getTrust()) && role.getRoleMembers() != null && !role.getRoleMembers().isEmpty()) { | |
| LOG.error("Solution Template {} role {} has both trust and members defined. Exiting...", | |
| templateName, role.getName()); | |
| throw new RuntimeException("Solution Template " + templateName + " role " + role.getName() + " has both trust and members defined"); | |
| } | |
| } | |
| } |
| public void testAddSolutionTemplateSettingTrustOnRoleWithMembers() throws ServerResourceException { | ||
| String domainName = "warn-trust-on-role-with-members"; | ||
| TopLevelDomain dom1 = createTopLevelDomainObject(domainName, "Test Domain1", "testOrg", adminUser); | ||
| zms.postTopLevelDomain(mockDomRsrcCtx, auditRef, null, dom1); | ||
|
|
||
| // Create role-with-members-test role with members first | ||
| List<RoleMember> members = new ArrayList<>(); | ||
| members.add(new RoleMember().setMemberName("user.joe")); | ||
| Role role = createRoleObject(domainName, "role-with-members-test", null, members); | ||
| zms.putRole(mockDomRsrcCtx, domainName, "role-with-members-test", auditRef, false, null, role); | ||
|
|
||
| // Setup log appender to capture warning messages | ||
| boolean[] warningLogged = new boolean[1]; | ||
| Logger dbServiceLogger = (Logger) LoggerFactory.getLogger(DBService.class); | ||
| AppenderBase testAppender = new AppenderBase() { | ||
| @Override | ||
| protected void append(Object o) { | ||
| if (o instanceof LoggingEvent) { | ||
| String message = ((LoggingEvent) o).getMessage(); | ||
| if (message.contains("SolutionTemplate is setting trust on role") && | ||
| message.contains("which already has members")) { | ||
| warningLogged[0] = true; | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| testAppender.start(); | ||
| dbServiceLogger.addAppender(testAppender); | ||
|
|
||
| // Apply template which tries to set trust on this role | ||
| List<String> templates = new ArrayList<>(); | ||
| templates.add("templateWithTrustRoleTest"); | ||
| DomainTemplate domainTemplate = new DomainTemplate().setTemplateNames(templates); | ||
| zms.dbService.executePutDomainTemplate(mockDomRsrcCtx, domainName, domainTemplate, auditRef, "testCaller"); | ||
|
|
||
| testAppender.stop(); | ||
| dbServiceLogger.detachAppender(testAppender); | ||
|
|
||
| // Verify the warning was logged | ||
| assertTrue(warningLogged[0], "Expected warning log about setting trust on role with members"); | ||
|
|
||
| zms.deleteTopLevelDomain(mockDomRsrcCtx, domainName, auditRef, null); | ||
| } |
There was a problem hiding this comment.
This test method contains significant boilerplate code for setting up a log appender to capture warnings. This same logic is duplicated in testAddSolutionTemplateAddingMembersToTrustedRole. To improve maintainability and reduce code duplication, consider refactoring this logging setup and teardown into a private helper method within this test class.
There was a problem hiding this comment.
Since the log output is a temporary measure, I'd like to leave this as is.
Signed-off-by: Go Yakami <gyakami@lycorp.co.jp>
2 participants
Description
Modified the server startup process to abort if a SolutionTemplate is found with both Trust and RoleMembers configured.
Additionally, a warning message is now logged in the following scenarios:
Contribution Checklist:
Attach Screenshots (Optional)