-
Notifications
You must be signed in to change notification settings - Fork 444
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
[gcp] Migrate some gcp beat metricset to data streams #2707
Conversation
There have been an issue identified in current |
Adding tests requires To complete elastic/elastic-package#662 has been considered necessary adding a test |
8cf80ec
to
fe54af3
Compare
title: "Collect Google Cloud Platform (GCP) firewall logs (input: gcp-pubsub)" | ||
description: "Collecting firewall logs from Google Cloud Platform (GCP) instances (input: gcp-pubsub)" | ||
input_group: logs | ||
- name: vpcflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the organization of the package, but have a concern for existing users. What will happen when existing users upgrade?
I was trying to so something similar using policy_template in the CrowdStrike module, but when I tested upgrading I have hit some issues (see #2806). So I recommend doing a test where you setup gcp with an existing version, then try to upgrade to this new version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are in particular the things that break? I don't know any way to assist users with upgrades that we may use to help in this use case.
We also cannot use input level variables as those are not yet supported by Kibana (see elastic/kibana#112272)
I think a possible workaround would be an upgrade guide and a major version bump to signal the breaking change. What do you think? Could this be a valid strategy for your use case too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are in particular the things that break?
I suspect the issue lies with creating separate policy_templates
which IIUC basically creates different integrations from a user perspective. Then there is no upgrade path from the older config, but TBH I'm not really sure. That's why I would recommend doing an quick independent test with gcp to check if you hit upgrade problems. Better to find the issue earlier and we can discuss what we can do to solve it or workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the upgrade path and your fears were correct. I'm going to write down my investigation in a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upgrade issue was tracked in elastic/kibana#131251 and has been resolved, I confirmed the fix in latest 8.3 build candidate.
|
🌐 Coverage report
|
/test |
There is some internal discussion going on about how to handle the breaking change that this PR would introduce. For the moment this is not going to be merged, so I'm reverting this to Draft waiting for the discussion to reach a conclusive point. |
This configuration is not working and break test execution, as seen in https://beats-ci.elastic.co/blue/organizations/jenkins/Ingest-manager%2Fintegrations/detail/PR-2707/11/pipeline Error in logs: ``` Error: error running package system tests: could not complete test run: could not setup service: can't attach service container to the stack network: could not attach container to the stack network (stderr="Error response from daemon: No such container: elastic-package-service_gcp_1\n"): exit status 1 script returned exit code 1 ``` This test case is copied over from https://github.com/elastic/elastic-package/blob/79c22bca9ccd95c1b38861bcea5c4298057a78f8/test/packages/parallel/gcp/data_stream/compute/_dev/test/system/test-default-config.yml Removing to unblock moving forward. Tests to ensure the data stream works have been done manually.
Co-authored-by: kaiyan-sheng <kaiyan.sheng@elastic.co>
Decision has been made and this change has been considered non breaking as there is an upgrade path and the upgrade is manually triggered by a user. I've updated the Kibana constraint to 8.3.0 in edc3905 to prevent users from updating to new package version on Kibana affected by elastic/kibana#131251 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments.
packages/gcp/changelog.yml
Outdated
- version: 2.0.0 | ||
changes: | ||
- description: | | ||
Move configurations to support metrics. This change is breaking, as it move |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move configurations to support metrics. This change is breaking, as it move | |
Move configurations to support metrics. This change is breaking, as it moves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8d43275
variables again when upgrading the policies to this version. | ||
type: breaking-change | ||
link: https://github.com/elastic/integrations/pull/2707 | ||
- description: Add GCP Billing Data Stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there docs for this data stream? (if adding, it would be worth including a link to https://cloud.google.com/billing/docs/reports since there are a lot of details that help understand the documents).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation in dc10062
Thank you for pointing this out!
|
||
## Metrics | ||
|
||
This is the `compute` dataset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth fleshing these out for consistency with the other data_stream docs? They go into a little bit of detail about what they are collecting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation in dc10062
|
||
## Metrics | ||
|
||
This is the `firestore` dataset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation in dc10062
multi: false | ||
required: false | ||
show_user: false | ||
description: "GCP Alternative host" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these could be fleshed out a little so the user know when to use it, or to not use it. From the beat code, "Overrides the default Pub/Sub service address and disables TLS. For testing."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 71facbc
packages/gcp/manifest.yml
Outdated
title: filebeat gcp audit | ||
size: 1702x996 | ||
type: image/png | ||
- name: firwall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/firwall/firewall/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8d43275
Before version 8.3.0 a bug in the Fleet UI configuration conflict resolution screen would prevent a successful update of this package to the new version. The constraint is updated on Kibana >= 8.3 where the related fix landed[1] so we can guarantee a working upgrade path. [1]: elastic/kibana#132068
After internal discussion, the fix to the Fleet UI conflict resolution has not been backported to 7.17, thus making this upgrade a breaking change without a valid upgrade path (it would still be possible to not upgrade policies and only create new ones but this is uncharted territory). Based on this we decided to remove support for 7.17 altogether from gcp 2.0.0
Co-authored-by: Dan Kortschak <dan.kortschak@elastic.co>
Please note that after internal discussion due to a bug in Kibana < 8.3 not backported to 7.17 we decided to remove support for 7.17 release track in dec0dc1 I'm discussing if the fix applied to 8.3 can/will be backported to 7.17 or if there are limitations that prevent that. Update: fixes can be backported. I discussed with @tommyers-elastic (and on suggestion from @andresrc) and we decided to merge this as is it and re-introduce 7.17 compatibility in a later release of the package to unblock this and test 7.17 appropriately. cc @andresrc |
What does this PR do?
Metricsets from
gcp
beat module must be migrated togcp
integration package data streams, to support using them with the agent.This PR will contain metricset migration and tests for each of them. It will be a long running PR, as I want to add tests for each data stream, something today completely missing, but I want to have it open as a draft to allow following progresses.
Partial PRs will target
gcp-metrics
branch to add to this feature allowing to break down the scope of work in manageable chunks.Checklist
changelog.yml
file.Author's Checklist
How to test this PR locally
Related issues
Screenshots