[Cloud Security] fix agent count to show correct pop-up modal #181503
Conversation
Omolola-Akinleye
added
bug
|
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
botelastic
bot
added
the
Team:Fleet
|
Pinging @elastic/fleet (Team:Fleet) |
Omolola-Akinleye
added
the
release_note:skip
Omolola-Akinleye
changed the title
| setSelectedPolicyTab: (tab: SelectedPolicyTab) => void; | ||
| }) { | ||
| const { isAgentlessEnabled } = useAgentlessPolicy(); | ||
| const [selectedSetupTechnology, setSelectedSetupTechnology] = useState<SetupTechnology>( | ||
| SetupTechnology.AGENT_BASED | ||
| ); | ||
| const [agentlessPolicy, setAgentlessPolicy] = useState<AgentPolicy | undefined>(); | ||
| const [currentAgentPolicy, setCurrentAgentPolicy] = useState<AgentPolicy | undefined>(); |
There was a problem hiding this comment.
Why do we need this state? What confuses me :
- we set it in the effect that fetches the agentless policy. But this is incorrect logically, as there is no connection between this effect and setting up the actual agent policy. It doesn't do any
updateAgentPolicy - in the
handleSetupTechnologyChangethe usage ofcurrentAgentPolicyis quite confusing.
setCurrentAgentPolicy(agentlessPolicy);
updateAgentPolicy(currentAgentPolicy || agentlessPolicy);
first of all it leaves the impression that as currentAgentPolicy is set to agentlessPolicy right before calling updateAgentPolicy it will always be agentlessPolicy anyway. But mind that setState is async in nature, so the previous value of the currentAgentPolicy will be used. But as the two possible values for currentAgentPolicy are agentlessPolicy or undefined, this code is just equal to updateAgentPolicy(agentlessPolicy);
I suspect that we don't need this state at all and it should be enough to just set updateAgentPolicy(undefined); when setupTechnology is AGENT_BASED
There was a problem hiding this comment.
Unless remove the useEffect and refactor, there will be a race condition to agent policy reset to agentless policy. This essentially affect the agent count and causes the bug.
- we set it in the effect that fetches the agentless policy. But this is incorrect logically, as there is no connection between this effect and setting up the actual agent policy. It doesn't do any updateAgentPolicy
Since the Agentless Feature Flag is enabled and Agentless option is selected by default. I set the currentAgentPolicy state to agentless policy.
I added the isDirty check; however, the race condition continues so my next thought is to keep track of the current agent policy. The currentAgentPolIcy state helps track the latest agent policy when user selects setupTechnogy which can be the following agentless, undefined, and agent-based, When the handleSetupTechnologyChange gets passed a new value, currentAgentPolicy has separate state isn't affected by the useEffect race condition which keep updating agent policy to agentless.
I suspect that we don't need this state at all and it should be enough to just set updateAgentPolicy(undefined); when setupTechnology is AGENT_BASED
I tried before calling
updateAgentPolicy(undefined)and again useEffect kept resetting agent policy id toagentless. Adding thereturnfixes the issue.
| @@ -226,10 +226,14 @@ export const CreatePackagePolicySinglePage: CreatePackagePolicyParams = ({ | |||
| } | |||
| }; | |||
|
|
|||
| if (selectedPolicyTab === SelectedPolicyTab.NEW) { | |||
| setAgentCount(0); | |||
There was a problem hiding this comment.
I believe we need to return after setting the agent count to 0. Otherwise, as agentPolicyId is available when switching from EXISTING to NEW it will be set to the agent count of the selected agent policy from the EXISTING tab
There was a problem hiding this comment.
Made changes.
| setSetupTechnology(value); | ||
| setIsDirty(true); | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Are these useEffects necessary? I don't see a need for useEffect here or below.
There was a problem hiding this comment.
Hey Sean, thank you. I agree that using the useEffect should be avoided generally. I apply a fix because I believe this useEffect was placed before to check edge case. cc: @maxcold @kfirpeled @seanrathier For this bug useEffect is causing race condition for bugs. Do want to open a tech debt ticket or refactor in this PR? Refactoring can cause changes in architecture on how Agentless is setup and may take additional effort keep in mind.
There was a problem hiding this comment.
@Omolola-Akinleye If we can open. tech debt ticket that would be great and I can approve it after. Thanks for responding.
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
…c#181503) ## Summary Summarize your PR. If it involves visual changes include a screenshot or gif. This PR fixes agent count to for the following cases: * If current agent policy is New agent policy, then AWS CloudFormation when the agent count is 0 * if current agent policy is Existing agent policy with an agent , then show confirmation agent modal * If current agent policy is Agentless policy, then navigate to fleet agent policies page. `isDirty` and `currentAgentPolicy` states will used be to circumvent the race condition `useEffect` that sets SetupTechnology when we first navigate to the add CSPM integration page. * `currentAgentPolicy` state keep current agentPolicy and reset agent count when we switch from `agentless` to `agent-based` * `isDirty` helps exit out `x-pack/plugins/cloud_security_posture/public/components/fleet_extensions/setup_technology_selector/use_setup_technology.ts` `useEffect` race condition once we initialize `setupTechnology` state on page load.
Summary
Summarize your PR. If it involves visual changes include a screenshot or gif.
This PR fixes agent count to for the following cases:
isDirtyandcurrentAgentPolicystates will used be to circumvent the race conditionuseEffectthat sets SetupTechnology when we first navigate to the add CSPM integration page.currentAgentPolicystate keep current agentPolicy and reset agent count when we switch fromagentlesstoagent-basedisDirtyhelps exit outx-pack/plugins/cloud_security_posture/public/components/fleet_extensions/setup_technology_selector/use_setup_technology.tsuseEffectrace condition once we initializesetupTechnologystate on page load.