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

Send package.version to components #4024

Merged
merged 35 commits into from
Feb 7, 2024

Conversation

AndersonQ
Copy link
Member

@AndersonQ AndersonQ commented Jan 5, 2024

What does this PR do?

The agent now includes AgentInfo alongside the connection information during component startup. It also stores the BuildHash sent by the components as part of the ComponentVersionInfo during check-in.

To facilitate testing, an info.Agent interface has been created to abstract the implementation of info.AgentInfo. This abstraction allows for the creation of mocks, enabling testing scenarios where using info.NewAgentInfo is impractical. This is because info.NewAgentInfo relies on the agent vault, which, on Mac systems, is the system's keychain. Accessing the keychain requires root permissions, which are not available during testing.

Why is it important?

The components should report the package version instead of their own.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool
  • I have added an integration test or an E2E test

How to test this PR locally

With a beats build containing elastic/beats#37553:

  • build the agent
  • change the package.version file to contain a version with extra build information, e.g.: 8.13.0+build20131123
  • enroll it to fleet
  • the events sent by the beats should have the agent.version, elastic_agent.version and metadata.version set as the version defined on the package.version file.
  • in the state.yaml from the agent diagnostics, for each component, there should be:
    • no version field in the version_info
    • a new version_info.build_hash with the beats build hash

Related issues

  • N/A

Related PRs

Questions to ask yourself

  • How are we going to support this in production?
  • How are we going to measure its adoption?
  • How are we going to debug this?
  • What are the metrics I should take care of?
  • ...

@AndersonQ AndersonQ added enhancement New feature or request Team:Elastic-Agent Label for the Agent team backport-skip labels Jan 5, 2024
@AndersonQ AndersonQ self-assigned this Jan 5, 2024
Copy link
Contributor

mergify bot commented Jan 5, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b beats-report-package-version upstream/beats-report-package-version
git merge upstream/main
git push upstream beats-report-package-version

@AndersonQ AndersonQ force-pushed the beats-report-package-version branch 5 times, most recently from 45f85c2 to 00afbc2 Compare January 10, 2024 12:52
Copy link
Member Author

Choose a reason for hiding this comment

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

Once #4039 is merged, this changes won't me o this PR anymore

@@ -1,4 +1,5 @@
{"name": "github.com/elastic/beats/v7", "licenceType": "Elastic"}
{"name": "github.com/AndersonQ/elastic-agent-client/v7", "licenceType": "Elastic"}
Copy link
Member Author

Choose a reason for hiding this comment

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

It'll be removed once elastic/elastic-agent-client#93 is merged

@AndersonQ AndersonQ requested a review from blakerouse January 10, 2024 14:10
@cmacknz
Copy link
Member

cmacknz commented Jan 10, 2024

The integration test we are going to want here will be to change the agent package version file at install time to be something different from what the agent was built with. You can edit it to something the build will never produce, like set it to 8.0.5000+build19700101 for example.

Then install the agent, ensure it can enroll in Fleet, and verify the documents ingested into Elasticsearch actually have that version in the agent.version field (and the elastic_agent.version field if it exists in the document).

You could probably just update the existing log ingestion tests to operate with a modified package version to cover this. https://github.com/elastic/elastic-agent/blob/main/testing/integration/logs_ingestion_test.go

You'd have to inspect the version fields in the returned documents in addition to what it does now, but this will be easier and execute faster than writing a completely new test.

Copy link
Contributor

mergify bot commented Jan 18, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b beats-report-package-version upstream/beats-report-package-version
git merge upstream/main
git push upstream beats-report-package-version

@AndersonQ AndersonQ force-pushed the beats-report-package-version branch from f7690b8 to 2d18ca4 Compare January 23, 2024 06:54
update agent-client

clean up

add test for package version on ConnInfo

fix license detection and notice

send AgentInfor during component startup

update agent client

update agent client

update agent client

drop replace

go mod tidy

remove notice override

add test
@AndersonQ AndersonQ force-pushed the beats-report-package-version branch from 6977cce to 452b709 Compare January 24, 2024 13:20
Copy link
Member

@pchila pchila left a comment

Choose a reason for hiding this comment

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

After discussing with @AndersonQ and the comments from @cmacknz it is now clear to me that the hash we use in component state is not a blocker.
The hacking of the version in TestComponentBuildHashInDiagnostics is a bit worrying as it will probably stop working when the version is used in the agent install path but right now is not a blocker (the test will need to be fixed or disabled if it stops working after #2579 is implemented.

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Overall this looks good. I question the need for the changelog entry though.

kind: feature

# Change summary; a 80ish characters long description of the change.
summary: Agent information is sent to components together with the connection information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really informational for our users? I don't think they care, this is more an internal change and not something that affects the usage of the Elastic Agent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree, this change is more an internal change than something the users should pay attention at

Copy link
Member

Choose a reason for hiding this comment

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

+1 we can remove the changelog entry here.

Copy link

Quality Gate passed Quality Gate passed

The SonarQube Quality Gate passed, but some issues were introduced.

4 New issues
0 Security Hotspots
66.7% 66.7% Coverage on New Code
0.0% 0.0% Duplication on New Code

See analysis details on SonarQube

@cmacknz
Copy link
Member

cmacknz commented Feb 6, 2024

Tested this manually, looks like it works. The agent.name field in each event matches the agent package version.

I did note that the build hash field is not included in the version info output from elastic-agent status --output=yaml (and json, and full)

- id: system/metrics-default
  name: system/metrics
  state: 2
  message: 'Healthy: communicating with pid ''86008'''
  units:
  - unit_id: system/metrics-default
    unit_type: 1
    state: 2
    message: Healthy
  - unit_id: system/metrics-default-system/metrics-system-56875859-e15e-48ab-ad98-29698fd331ad
    unit_type: 0
    state: 2
    message: Healthy
  version_info:
    name: beat-v2-client
    meta:
      build_time: 2024-02-06 11:59:31 +0000 UTC
      commit: ac6e2230bcfe766f1bd5695a59aa7d55116098f7

It is included in the state.yaml field in the diagostics though:

    - id: system/metrics-default
      state:
        component:
            apmconfig: null
            limits:
                gomaxprocs: 0
                source:
                    fields:
                        go_max_procs:
                            kind:
                                numbervalue: 0
        component_idx: 2
        features_idx: 2
        message: 'Healthy: communicating with pid ''86008'''
        state: 2
        units:
            input-system/metrics-default-system/metrics-system-56875859-e15e-48ab-ad98-29698fd331ad:
                message: Healthy
                state: 2
            output-system/metrics-default:
                message: Healthy
                state: 2
        version_info:
            build_hash: ac6e2230bcfe766f1bd5695a59aa7d55116098f7
            meta:
                build_time: 2024-02-06 11:59:31 +0000 UTC
                commit: ac6e2230bcfe766f1bd5695a59aa7d55116098f7
            name: beat-v2-client

@AndersonQ AndersonQ merged commit 4fa6bb6 into elastic:main Feb 7, 2024
9 checks passed
@AndersonQ AndersonQ deleted the beats-report-package-version branch February 7, 2024 05:52
@AndersonQ
Copy link
Member Author

Tested this manually, looks like it works. The agent.name field in each event matches the agent package version.

I did note that the build hash field is not included in the version info output from elastic-agent status --output=yaml (and json, and full)

- id: system/metrics-default
  name: system/metrics
  state: 2
  message: 'Healthy: communicating with pid ''86008'''
  units:
  - unit_id: system/metrics-default
    unit_type: 1
    state: 2
    message: Healthy
  - unit_id: system/metrics-default-system/metrics-system-56875859-e15e-48ab-ad98-29698fd331ad
    unit_type: 0
    state: 2
    message: Healthy
  version_info:
    name: beat-v2-client
    meta:
      build_time: 2024-02-06 11:59:31 +0000 UTC
      commit: ac6e2230bcfe766f1bd5695a59aa7d55116098f7

Interesting, the --output full already did not show it:

❯ sudo /opt/Elastic/Agent/elastic-agent status --output full
┌─ fleet
│  └─ status: (HEALTHY) Connected
└─ elastic-agent
   ├─ status: (HEALTHY) Running
   ├─ info
   │  ├─ id: 7c2067ff-30ff-4628-b8cf-af36fa56a524
   │  ├─ version: 8.12.0
   │  └─ commit: 5cbf2e403c761f91d11eca6b9cb5385e0f07f2ce
   ├─ beat/metrics-monitoring
   │  ├─ status: (HEALTHY) Healthy: communicating with pid '2343'
   │  ├─ beat/metrics-monitoring
   │  │  ├─ status: (HEALTHY) Healthy
   │  │  └─ type: OUTPUT
   │  └─ beat/metrics-monitoring-metrics-monitoring-beats
   │     ├─ status: (HEALTHY) Healthy
   │     └─ type: INPUT

❯ sudo /opt/Elastic/Agent/elastic-agent status --output yaml
info:
  id: 7c2067ff-30ff-4628-b8cf-af36fa56a524
  version: 8.12.0
  commit: 5cbf2e403c761f91d11eca6b9cb5385e0f07f2ce
  build_time: 2024-01-11 13:25:50 +0000 UTC
  snapshot: false
  pid: 1274
state: 2
message: Running
components:
- id: beat/metrics-monitoring
  name: beat/metrics
  state: 2
  message: 'Healthy: communicating with pid ''2343'''
  units:
  - unit_id: beat/metrics-monitoring
    unit_type: 1
    state: 2
    message: Healthy
  - unit_id: beat/metrics-monitoring-metrics-monitoring-beats
    unit_type: 0
    state: 2
    message: Healthy
  version_info:
    name: beat-v2-client
    version: 8.12.0
    meta:
      build_time: 2024-01-10 21:09:16 +0000 UTC
      commit: 27c592782c25906c968a41f0a6d8b1955790c8c5

I'll open an issue to fix the status command as a whole then

@AndersonQ
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip enhancement New feature or request skip-changelog Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants