feat:[NEXT-1951] Modified LoadBalancerUnused Rule and removed loadbal…#2392
Merged
Conversation
…ancer from legacy azure collector
Contributor
WalkthroughLoad balancer discovery is disabled by commenting out its handling in the asset collection factory and target types constant. Separately, the unused load balancer detection logic is updated to evaluate a different JSON property structure for determining utilization status. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
arunpaladin
approved these changes
Apr 28, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
jobs/azure-discovery/src/main/java/com/tmobile/pacbot/azure/inventory/file/AssetDataFactory.java (1)
144-145: Replace commented-outcase LOADBALANCERwith explicit behavior.At Line 144, commented switch logic is hard to maintain and currently falls through to a silent empty list via
default. Make the deprecation explicit (prefer fail-fast) so accidental invocations are visible.Proposed cleanup
- /*case LOADBALANCER: - return loadBalancerInventoryCollector.collect(subscription, tagMap);*/ + case LOADBALANCER: + throw new UnsupportedOperationException("LOADBALANCER collection is disabled in legacy azure collector");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jobs/azure-discovery/src/main/java/com/tmobile/pacbot/azure/inventory/file/AssetDataFactory.java` around lines 144 - 145, In AssetDataFactory replace the commented-out "case LOADBALANCER" fall-through with explicit fail-fast behavior: add a real case for LOADBALANCER in the switch that does not silently fall to default but instead throws an UnsupportedOperationException (or IllegalStateException) with a clear message referencing LOADBALANCER and advising why it’s unsupported; remove the commented code referencing loadBalancerInventoryCollector.collect so accidental calls surface as errors rather than returning an empty list from the default branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jobs/azure-discovery/src/main/java/com/tmobile/pacbot/azure/inventory/util/TargetTypesConstants.java`:
- Line 80: The removal of LOADBALANCER from TARGET_TYPES_TO_COLLECT breaks
population of the azure_loadbalancer index and silently disables rules relying
on that data; either restore LOADBALANCER to TARGET_TYPES_TO_COLLECT (re-enable
the commented "LOADBALANCER" entry) so the azure_loadbalancer ingestion path
remains active, or if you intend to deprecate load balancer discovery, update
the rule registry to disable or migrate the specific rules
remove_unused_load_balancer and
azure_loadbalancer_should_be_tagged_with_mandatory_tags and add a replacement
data source or an explicit guard so those rules error or are skipped rather than
returning false. Ensure whichever option you choose keeps the ingestion/state
consistent for azure_loadbalancer and updates any dependent logic that assumes
that index exists.
In
`@jobs/pacman-awsrules/src/main/java/com/tmobile/cloud/azurerules/LoadBalancer/RemoveUnusedLoadBalancer.java`:
- Around line 90-101: The code in RemoveUnusedLoadBalancer currently returns
false when ES hits are missing or empty (checks around
resultJson.has(PacmanRuleConstants.HITS) and hitsJsonArray.isEmpty()), which
treats data unavailability as a successful "used" result; instead, change these
branches to surface an error/indeterminate state (do not return false). Update
the logic in the method that reads resultJson so that when resultJson lacks
PacmanRuleConstants.HITS or when hitsJsonArray.isEmpty() you throw a descriptive
exception (e.g., RuleExecutionFailedException or another existing
rule-indeterminate/error exception used by your framework) with context from
resultJson, so callers can treat it as an error/indeterminate case rather than a
passed check; reference the symbols resultJson, PacmanRuleConstants.HITS,
hitsJson, and hitsJsonArray to locate and modify the checks.
- Around line 103-116: The code in RemoveUnusedLoadBalancer.java assumes JSON
nodes are objects/arrays and casts them directly (variables: source,
poolsElement, hasInstances stream), which can throw for malformed ES docs;
update parsing to first check poolsElement.isJsonArray() (already partially
done) and within the stream ensure each pool element isJsonObject before calling
getAsJsonObject, check that pool.getAsJsonObject("properties") exists and
isJsonObject (or null-safe via has("properties") &&
get("properties").isJsonObject()) before accessing properties, and verify
properties has "backendIPConfigurations" and that it isJsonArray before calling
getAsJsonArray(); if any check fails treat that pool as having no instances
(i.e., skip it) or return true safely so rule execution does not crash.
---
Nitpick comments:
In
`@jobs/azure-discovery/src/main/java/com/tmobile/pacbot/azure/inventory/file/AssetDataFactory.java`:
- Around line 144-145: In AssetDataFactory replace the commented-out "case
LOADBALANCER" fall-through with explicit fail-fast behavior: add a real case for
LOADBALANCER in the switch that does not silently fall to default but instead
throws an UnsupportedOperationException (or IllegalStateException) with a clear
message referencing LOADBALANCER and advising why it’s unsupported; remove the
commented code referencing loadBalancerInventoryCollector.collect so accidental
calls surface as errors rather than returning an empty list from the default
branch.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a9cf1ec-23fe-478f-9edf-9c161db52f0e
📒 Files selected for processing (3)
jobs/azure-discovery/src/main/java/com/tmobile/pacbot/azure/inventory/file/AssetDataFactory.javajobs/azure-discovery/src/main/java/com/tmobile/pacbot/azure/inventory/util/TargetTypesConstants.javajobs/pacman-awsrules/src/main/java/com/tmobile/cloud/azurerules/LoadBalancer/RemoveUnusedLoadBalancer.java
| DISK, // done | ||
| FUNCTION_APP, // done | ||
| LOADBALANCER, // done | ||
| // LOADBALANCER, // done |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== References to LB target type and collectors =="
rg -n -C3 '\bLOADBALANCER\b|LoadBalancerInventoryCollector|TARGET_TYPES_TO_COLLECT'
echo
echo "== References to azure_loadbalancer index or LB data outputs =="
rg -n -C3 'azure_loadbalancer|loadbalancer\.data|azure-loadbalancer\.data'
echo
echo "Expected:"
echo "- At least one active writer/collector path for load balancer data."
echo "- If none exists, this PR likely suppresses unused-LB detections."Repository: PaladinCloud/CE
Length of output: 40002
🏁 Script executed:
# Check for any other references to azure_loadbalancer beyond the disabled collector
rg -n 'azure_loadbalancer' --type-not=sql
# Check for any loader, importer, or migration for loadbalancer
rg -n -i 'loadbalancer.*import|loadbalancer.*load|loadbalancer.*migrate|loadbalancer.*sync'
# Verify the AssetDataFactory factory method to confirm no fallback paths
cat -n jobs/azure-discovery/src/main/java/com/tmobile/pacbot/azure/inventory/file/AssetDataFactory.java | sed -n '130,160p'Repository: PaladinCloud/CE
Length of output: 50371
Disabling LOADBALANCER collection creates a silent coverage failure for load balancer compliance rules.
Removing LOADBALANCER from TARGET_TYPES_TO_COLLECT (line 80) disables the only code path that populates the azure_loadbalancer index. Multiple rules (remove_unused_load_balancer, azure_loadbalancer_should_be_tagged_with_mandatory_tags) depend on this index and will silently return false when it contains no data, suppressing detections rather than erroring. No alternative ingestion mechanism exists in the codebase.
If load balancer discovery is intentionally being deprecated, ensure rules are also disabled or migrated to an alternative data source before merging this change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jobs/azure-discovery/src/main/java/com/tmobile/pacbot/azure/inventory/util/TargetTypesConstants.java`
at line 80, The removal of LOADBALANCER from TARGET_TYPES_TO_COLLECT breaks
population of the azure_loadbalancer index and silently disables rules relying
on that data; either restore LOADBALANCER to TARGET_TYPES_TO_COLLECT (re-enable
the commented "LOADBALANCER" entry) so the azure_loadbalancer ingestion path
remains active, or if you intend to deprecate load balancer discovery, update
the rule registry to disable or migrate the specific rules
remove_unused_load_balancer and
azure_loadbalancer_should_be_tagged_with_mandatory_tags and add a replacement
data source or an explicit guard so those rules error or are skipped rather than
returning false. Ensure whichever option you choose keeps the ingestion/state
consistent for azure_loadbalancer and updates any dependent logic that assumes
that index exists.
Comment on lines
+90
to
101
| if (!resultJson.has(PacmanRuleConstants.HITS)) { | ||
| return false; | ||
| } | ||
|
|
||
| String hitsString = resultJson.get(PacmanRuleConstants.HITS).toString(); | ||
| logger.debug("hit content in result json: {}", hitsString); | ||
| JsonObject hitsJson = JsonParser.parseString(hitsString).getAsJsonObject(); | ||
| JsonArray hitsJsonArray = hitsJson.getAsJsonObject().get(PacmanRuleConstants.HITS).getAsJsonArray(); | ||
|
|
||
| if (hitsJsonArray.isEmpty()) { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Do not treat missing/empty ES hits as “used”.
At Line 91 and Line 100, returning false on missing/empty hits converts data unavailability into a success path. This can hide real unused load balancers during ingestion gaps.
Safer behavior (mark as indeterminate/error)
- if (!resultJson.has(PacmanRuleConstants.HITS)) {
- return false;
- }
+ if (!resultJson.has(PacmanRuleConstants.HITS)) {
+ throw new IllegalStateException("ES response missing hits section for load balancer lookup");
+ }
@@
- if (hitsJsonArray.isEmpty()) {
- return false;
- }
+ if (hitsJsonArray.isEmpty()) {
+ throw new IllegalStateException("No load balancer document found in ES for resource");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!resultJson.has(PacmanRuleConstants.HITS)) { | |
| return false; | |
| } | |
| String hitsString = resultJson.get(PacmanRuleConstants.HITS).toString(); | |
| logger.debug("hit content in result json: {}", hitsString); | |
| JsonObject hitsJson = JsonParser.parseString(hitsString).getAsJsonObject(); | |
| JsonArray hitsJsonArray = hitsJson.getAsJsonObject().get(PacmanRuleConstants.HITS).getAsJsonArray(); | |
| if (hitsJsonArray.isEmpty()) { | |
| return false; | |
| } | |
| if (!resultJson.has(PacmanRuleConstants.HITS)) { | |
| throw new IllegalStateException("ES response missing hits section for load balancer lookup"); | |
| } | |
| String hitsString = resultJson.get(PacmanRuleConstants.HITS).toString(); | |
| logger.debug("hit content in result json: {}", hitsString); | |
| JsonObject hitsJson = JsonParser.parseString(hitsString).getAsJsonObject(); | |
| JsonArray hitsJsonArray = hitsJson.getAsJsonObject().get(PacmanRuleConstants.HITS).getAsJsonArray(); | |
| if (hitsJsonArray.isEmpty()) { | |
| throw new IllegalStateException("No load balancer document found in ES for resource"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jobs/pacman-awsrules/src/main/java/com/tmobile/cloud/azurerules/LoadBalancer/RemoveUnusedLoadBalancer.java`
around lines 90 - 101, The code in RemoveUnusedLoadBalancer currently returns
false when ES hits are missing or empty (checks around
resultJson.has(PacmanRuleConstants.HITS) and hitsJsonArray.isEmpty()), which
treats data unavailability as a successful "used" result; instead, change these
branches to surface an error/indeterminate state (do not return false). Update
the logic in the method that reads resultJson so that when resultJson lacks
PacmanRuleConstants.HITS or when hitsJsonArray.isEmpty() you throw a descriptive
exception (e.g., RuleExecutionFailedException or another existing
rule-indeterminate/error exception used by your framework) with context from
resultJson, so callers can treat it as an error/indeterminate case rather than a
passed check; reference the symbols resultJson, PacmanRuleConstants.HITS,
hitsJson, and hitsJsonArray to locate and modify the checks.
Comment on lines
+103
to
+116
| JsonObject source = (JsonObject) ((JsonObject) hitsJsonArray.get(0)) | ||
| .get(PacmanRuleConstants.SOURCE); | ||
|
|
||
| JsonElement poolsElement = source.get("backendAddressPools"); | ||
| if (poolsElement == null || !poolsElement.isJsonArray()) { | ||
| return true; | ||
| } | ||
|
|
||
| boolean hasInstances = StreamSupport.stream(poolsElement.getAsJsonArray().spliterator(), false) | ||
| .map(JsonElement::getAsJsonObject) | ||
| .map(pool -> pool.getAsJsonObject("properties")) | ||
| .filter(Objects::nonNull) | ||
| .map(props -> props.getAsJsonArray("backendIPConfigurations")) | ||
| .anyMatch(configs -> configs != null && !configs.isEmpty()); |
Contributor
There was a problem hiding this comment.
Add JSON shape guards before nested object casts.
At Line 103 and Line 112-115, direct casts assume _source, pool items, and properties are always objects. Partial or unexpected ES docs can throw at runtime and fail rule execution.
Defensive parsing update
- JsonObject source = (JsonObject) ((JsonObject) hitsJsonArray.get(0))
- .get(PacmanRuleConstants.SOURCE);
+ JsonElement sourceElement = hitsJsonArray.get(0).getAsJsonObject().get(PacmanRuleConstants.SOURCE);
+ if (sourceElement == null || !sourceElement.isJsonObject()) {
+ return false;
+ }
+ JsonObject source = sourceElement.getAsJsonObject();
@@
- boolean hasInstances = StreamSupport.stream(poolsElement.getAsJsonArray().spliterator(), false)
- .map(JsonElement::getAsJsonObject)
- .map(pool -> pool.getAsJsonObject("properties"))
- .filter(Objects::nonNull)
- .map(props -> props.getAsJsonArray("backendIPConfigurations"))
- .anyMatch(configs -> configs != null && !configs.isEmpty());
+ boolean hasInstances = StreamSupport.stream(poolsElement.getAsJsonArray().spliterator(), false)
+ .filter(JsonElement::isJsonObject)
+ .map(JsonElement::getAsJsonObject)
+ .map(pool -> pool.get("properties"))
+ .filter(Objects::nonNull)
+ .filter(JsonElement::isJsonObject)
+ .map(JsonElement::getAsJsonObject)
+ .map(props -> props.get("backendIPConfigurations"))
+ .filter(Objects::nonNull)
+ .filter(JsonElement::isJsonArray)
+ .map(JsonElement::getAsJsonArray)
+ .anyMatch(configs -> !configs.isEmpty());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jobs/pacman-awsrules/src/main/java/com/tmobile/cloud/azurerules/LoadBalancer/RemoveUnusedLoadBalancer.java`
around lines 103 - 116, The code in RemoveUnusedLoadBalancer.java assumes JSON
nodes are objects/arrays and casts them directly (variables: source,
poolsElement, hasInstances stream), which can throw for malformed ES docs;
update parsing to first check poolsElement.isJsonArray() (already partially
done) and within the stream ensure each pool element isJsonObject before calling
getAsJsonObject, check that pool.getAsJsonObject("properties") exists and
isJsonObject (or null-safe via has("properties") &&
get("properties").isJsonObject()) before accessing properties, and verify
properties has "backendIPConfigurations" and that it isJsonArray before calling
getAsJsonArray(); if any check fails treat that pool as having no instances
(i.e., skip it) or return true safely so rule execution does not crash.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ancer from legacy azure collector
Description
Please include a summary of the changes and the related issues. Please also include relevant motivation and context. List
any dependencies that are required for this change.
Problem
Solution
Fixes # (issue if any)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also
list any relevant details for your test configuration
Checklist:
Other Information:
List any documentation updates that are needed for the Wiki
Summary by CodeRabbit
Release Notes