Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cloud Security] use only available agent versions for Cloudformation and Cloud Shell parameters #166198

Conversation

maxcold
Copy link
Contributor

@maxcold maxcold commented Sep 11, 2023

Summary

fixes

instead of using Kibana version for Cloudformation and Cloud Shell params in CNVM and CSPM integrations, check if the version of an agent that matches the current Kibana version actually available as an artifact. Relevant for serverless, where Kibana version points to not-yet released versions of Elastic Agent.

Checklist

@maxcold maxcold added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Cloud Security Cloud Security team related v8.11.0 labels Sep 11, 2023
@maxcold maxcold requested a review from a team September 11, 2023 18:23
@maxcold maxcold marked this pull request as ready for review September 11, 2023 20:30
@maxcold maxcold requested a review from a team as a code owner September 11, 2023 20:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

@maxcold
Copy link
Contributor Author

maxcold commented Sep 11, 2023

A question to the Fleet team: this logic of using the Kibana version as an agent version for installation steps exists in other places for fleet UI, basically everywhere where StandaloneInstrucations and ManualInstructions are used. Should I update those places to the logic introduced in this PR as a next step? I am just not sure that I know all the use cases well enough to make a call if it's correct to just use the Kibana version there. But I think with serverless the links to the agent artifact will be incorrect, though maybe some components are not applicable to serverless

@maxcold maxcold force-pushed the 7557-cloud-security-cloudformation-params-point-to-non-existing-agent-artifact-in-serverless branch from 91d4983 to f5c39d4 Compare September 12, 2023 10:06
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Sep 12, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@maxcold maxcold requested a review from joshdover September 12, 2023 10:07
if (
availableVersions &&
availableVersions.length > 0 &&
availableVersions.indexOf(kibanaVersion) === -1
Copy link
Contributor Author

@maxcold maxcold Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshdover adding you to reviewers as you worked on https://github.com/elastic/kibana/pull/166150/files#diff-de1953cdeee69d71565b65a14751a0dd602ca5e3c8f81b4854113e6b97cc231b
Does this logic make sense? To my understanding, we need to check if the Kibana version is in available versions to support older ESS versions. Or each version of Kibana has JSON with available versions generated in a way, that it only contains the current patch version as the first element. Eg. in the future when the latest major is 8.13.0 and there is an 8.11.2 or smth released, will JSON contain versions lower than 8.11.2 or it actually will include all 8.12.x and 8.13.x?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding the JSON generated during the build will include all versions released up to that point in time, and nothing later.

On ESS/Stateful, we also add the current Kibana version which is already handled server side. I don't think this change here is needed, but I may not be understanding fully. Could you explain exactly the scenario that isn't currently working?

In any case, if a change is needed, please make it on the server side in https://github.com/elastic/kibana/blob/main/x-pack/plugins/fleet/server/services/agents/versions.ts so that it can be applied everywhere (there are some other backend pieces that consume this logic).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test case https://github.com/elastic/kibana/pull/166198/files#diff-949758c47e64dee7bf348fa03e9146d8b87a71a62f37bee09db163c46569675cR22 captures the scenario. Without the change, the returned version would be 8.9.0 while to my understanding we want an earlier version to be picked up.

If I'm not missing anything about this scenario, the change on the server side would be to filter out all the versions greater than the current Kibana version and I guess it will break other places in fleet when we need released agent versions greater than the current Kibana version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current logic only returns released agent versions in serverless, so I don't think you need any change to fix this for serverless.

Copy link
Contributor Author

@maxcold maxcold Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliaElastic the thing I struggle to understand is how we deal with the patch version to the older version of the stack. Eg. when there are 8.12.0, 8.11.0 released but the user updates to say 8.9.3 which was released. The list of released versions will contain 8.12.0 as the first element and this version then will be picked up for all the artifact links in the Fleet UI. So in 8.9.3 Kibana, the links will point to the 8.12.0 agent which has been released but may not be compatible with this old version. Do we see this as an issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't know if this is being tested, maybe from the Agent side? cc @cmacknz

Copy link
Member

@cmacknz cmacknz Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fleet server won't let agents that are newer than Fleet server's version connect, there is a version constraint enforced as part of the check in.

https://github.com/elastic/fleet-server/blob/d1d155959b0cc9ab2111c5b3dbdab27b5820245c/internal/pkg/api/userAgent.go#L32-L41

We plan to remove this constraint eventually but this is still the case today and for all previously released versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then if read @cmacknz's comment correctly we do actually need to make sure that we pick the agent version that matches the Kibana version if this version exists in the list of released agent versions. If we just keep it as it is now (picking the latest released agent version from the list no matter of the Kibana version) it will cause agents not to be able to connect to fleet servers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If kibana version is not in the list of agent versions, should the logic return the latest agent version that is <= kibana version? It's not a likely scenario, though might happen if kibana and agent uses different versions eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliaElastic good point, I changed the logic to accommodate for that and added a test case

maxcold added a commit that referenced this pull request Sep 18, 2023
…d Security integrations (#166485)

## Summary

fixes
- elastic/security-team#7557

using the new `useAgentVersion` hook to get the agent version to prefill
in the Cloudformation template

There is already a PR with the same fix
#166198 but as it also changes the
logic of `useAgentVersion` itself, it might take some time to align.
This PR only fixes the immediate issue we have in Serverless
@maxcold maxcold reopened this Sep 19, 2023
@maxcold maxcold force-pushed the 7557-cloud-security-cloudformation-params-point-to-non-existing-agent-artifact-in-serverless branch from 24c6566 to f0d74df Compare September 19, 2023 09:36
Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maxcold
Copy link
Contributor Author

maxcold commented Sep 21, 2023

I see that there is also logic on the server side in x-pack/plugins/fleet/server/services/agents/versions.ts to get the latest available agent version in getLatestAvailableVersion. I think ideally we should also apply the change I made for the client side there, but I'm not 100% sure as I'm not that familiar with the fleet codebase. If the fleet team thinks that I should also add this change to the server side, I can do it in a follow-up PR, pls let me know

…int-to-non-existing-agent-artifact-in-serverless
…int-to-non-existing-agent-artifact-in-serverless
…int-to-non-existing-agent-artifact-in-serverless
…int-to-non-existing-agent-artifact-in-serverless
…int-to-non-existing-agent-artifact-in-serverless
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #34 / serverless security UI Create Case "before each" hook for "creates a case"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 944 945 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 1.2MB 1.2MB +10.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 143.0KB 143.2KB +152.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@maxcold maxcold merged commit 62f9b56 into main Sep 26, 2023
@maxcold maxcold deleted the 7557-cloud-security-cloudformation-params-point-to-non-existing-agent-artifact-in-serverless branch September 26, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related Team:Fleet Team label for Observability Data Collection Fleet team v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants