Fix EST upgrade script, and add profile.configuration.managed switch to be able to disable adding profiles under CM - #5388
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTwo upgrade scripts now conditionally gate profile management operations on a ChangesProfile Management Configuration and Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 conditional checks to skip profile installation and updates when profile configuration is managed (profile.configuration.managed). It also refactors 04-UpdateMLDSAProfiles.py into smaller helper methods and adds file existence checks in 01-EnableEST.py. The review feedback highlights two critical issues: first, subsystem.config.get may return string values (like "false") which are truthy in Python, so they must be parsed explicitly to booleans; second, using return instead of continue inside the profile update loop in 01-EnableEST.py will prematurely abort the update process for subsequent profiles.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/server/upgrade/11.9.0/04-UpdateMLDSAProfiles.py (1)
96-113: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExisting profile files should still reconcile
profile.listandprofile.<id>.class_id.The
continueat Line 97 skips config reconciliation when the file already exists. On reruns/partial upgrades, this can leave profile metadata incomplete even though files are present.Proposed fix
- if os.path.exists(path): - continue - logger.info('Adding profile %s', file_name) - - instance.copyfile( - '/usr/share/pki/ca/profiles/ca/{}'.format(file_name), - path, - exist_ok=True, - force=False) + if not os.path.exists(path): + logger.info('Adding profile %s', file_name) + instance.copyfile( + '/usr/share/pki/ca/profiles/ca/{}'.format(file_name), + path, + exist_ok=True, + force=False) + else: + logger.info('Profile file %s already exists; reconciling config only', file_name) if profile not in profile_list: # Add new profiles to profile.list logger.info('Adding %s to profile.list', profile) profile_list.append(profile)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/server/upgrade/11.9.0/04-UpdateMLDSAProfiles.py` around lines 96 - 113, The code currently skips reconciliation when the profile file exists due to the "if os.path.exists(path): continue" path check; remove the early continue (or restructure so existence only skips the copyfile call) and ensure the subsequent logic that appends to profile_list and calls subsystem.set_config('profile.{}.class_id'.format(profile), 'caEnrollImpl') always runs regardless of file existence; keep the copy operation guarded by exist_ok/force but move or duplicate the profile-list and class_id reconciliation (references: path, instance.copyfile, profile_list, profile, subsystem.set_config) so reruns/partial upgrades update profile.list and profile.<id>.class_id even when files already exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/server/upgrade/11.9.0/01-EnableEST.py`:
- Around line 31-32: The code reads
subsystem.config.get('profile.configuration.managed', False) into manageprofiles
and branches on it directly, which treats string values like "false" as truthy;
update both scripts (01-EnableEST.py and 04-UpdateMLDSAProfiles.py) to normalize
that config value first by replacing the direct usage with a small
normalization: if the retrieved value is a bool keep it, if it's a string
interpret common truthy values (e.g. "true","1","yes") as True and everything
else as False, then assign the normalized boolean back to manageprofiles before
any if not manageprofiles checks so downstream logic uses a real boolean.
- Around line 109-111: In update_internal_profiles(), do not return when
encountering an already-updated internal profile; instead skip that profile and
continue processing the rest — replace the early "return" (inside the loop where
it checks if 'group="Enterprise EST Administrators"' in config['authz.acl'])
with logic to log the already-updated state and continue to the next profile so
all internal profiles are checked/updated (keep the logger.info message but
remove the return/exit behavior).
---
Outside diff comments:
In `@base/server/upgrade/11.9.0/04-UpdateMLDSAProfiles.py`:
- Around line 96-113: The code currently skips reconciliation when the profile
file exists due to the "if os.path.exists(path): continue" path check; remove
the early continue (or restructure so existence only skips the copyfile call)
and ensure the subsequent logic that appends to profile_list and calls
subsystem.set_config('profile.{}.class_id'.format(profile), 'caEnrollImpl')
always runs regardless of file existence; keep the copy operation guarded by
exist_ok/force but move or duplicate the profile-list and class_id
reconciliation (references: path, instance.copyfile, profile_list, profile,
subsystem.set_config) so reruns/partial upgrades update profile.list and
profile.<id>.class_id even when files already exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25fbf59a-8bce-4f98-b7a3-400e4f7d206d
📒 Files selected for processing (2)
base/server/upgrade/11.9.0/01-EnableEST.pybase/server/upgrade/11.9.0/04-UpdateMLDSAProfiles.py
|
Check if profile exists before trying to update to prevent it from causeing CA to fail to start.
edewata
left a comment
edewata
left a comment
There was a problem hiding this comment.
LGTM. Sorry for the delay. I'll merge this. Thanks!
3 participants
Add ability to turn off adding profiles to a CA with CM Managed Profiles
Check the CS.cfg for a new item profile.configuration.managed and default to false. If Profiles are under CM Management "true" skip adding files to the ca/profiles/ca for the instances but perform all other actions such as updating the registry for new entries, and new defaults for CS.cfg etc.
manageprofiles = subsystem.config.get('profile.configuration.managed', False)
01 EST Script does not check if a profile exists before attempting to update resulting in CA start failure if a profile does not exist.
Summary by CodeRabbit
Bug Fixes
Refactor