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

Add missing kubernetes fields; update istio doc to use kubernetes.pod.ip instead of host #37578

Merged

Conversation

tetianakravchenko
Copy link
Contributor

@tetianakravchenko tetianakravchenko commented Jan 8, 2024

Proposed commit message

  • WHAT: Extend documentation to add missing kubernetes fields; update istio documentation to use explicitly kubernetes.pod.ip instead of host
  • WHY:
    • include all kubernetes.* fields;
    • host variable is confusing as it is not clear to which IP it is referring

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.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

….ip instead of host

Signed-off-by: Tetiana Kravchenko <[email protected]>
@tetianakravchenko tetianakravchenko requested review from a team as code owners January 8, 2024 16:33
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 8, 2024
Copy link
Contributor

mergify bot commented Jan 8, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @tetianakravchenko? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2024-01-08T16:33:37.988+0000

  • Duration: 9 min 57 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@pierrehilbert pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Jan 8, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 8, 2024
@@ -221,18 +221,22 @@ These are the fields available within config templating. The `kubernetes.*` fiel
* kubernetes.container.image
* kubernetes.container.name
* kubernetes.namespace
* kubernetes.namespace_uid
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* kubernetes.namespace_uid
* kubernetes.namespace.uid

Copy link
Contributor

Choose a reason for hiding this comment

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

The field name is indeed kubernetes.namespace_uid.
@tetianakravchenko maybe we should find a way in this document to add also kubernetes.labels, kubernetes.annotations, kubernetes.namespace_labels and kubernetes.namespace_annotations. Those metadata fields are also part of the metadata fields that can be used in templating when the right parameters are set.
There is this section in the doc that states

If the `include_annotations` config is added to the provider config, then the list of annotations present in the config
are added to the event.

which is ok as an information but we don't list the field. Maybe we could list all fields above and leave those comments there as is.
Also we could add some comments about the namespace_labels and namespace_annotations based on the add_resource_metadata configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gsantoro there is no such field as kubernetes.namespace.uid :

Screenshot 2024-01-10 at 14 18 30

I see that we have such field documented for elastic-agent - https://www.elastic.co/guide/en/fleet/current/kubernetes-provider.html, I believe it is a mistake in the elastic-agent documentation

Copy link
Contributor

@gsantoro gsantoro Jan 10, 2024

Choose a reason for hiding this comment

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

@MichaelKatsoulis I am trying to wrap my head around the docs and the implementation.

I can found two references kubernetes dynamic variables at https://www.elastic.co/guide/en/fleet/current/kubernetes-provider.html and https://www.elastic.co/guide/en/beats/metricbeat/current/configuration-autodiscover.html. For example kubernetes.namespace.uid is in the first one and not in the second doc.

Should those two docs have the same variables and both of them have kubernetes.namespace_uid?

what about this code from elastic-agent-autodiscover where the docs point at namespace.uid and the test at namespace_uid? at https://github.com/elastic/elastic-agent-autodiscover/blob/4f4898db82e5a5b37eab7e0e093435b9fb739461/kubernetes/metadata/namespace_test.go#L69

I could use some clarification about where in the code those variables are created.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gsantoro kubernetes.namespace is unfortunately a keyword and not an object. There was a try in 8.0 to do this breaking change and put all namespace related fields under the kubernetes.namespace object but it was reverted before it reached production. This was done mainly because of the possible problems it would create to existing customers and the not easy migration procedure.

The documentation in https://www.elastic.co/guide/en/fleet/current/kubernetes-provider.html is wrong. The part of the code where those values are added is here https://github.com/elastic/elastic-agent-autodiscover/blob/b42be0df903aba6580b80761af6df86fa80c845d/kubernetes/metadata/namespace.go#L117

…ata.deployment and add_resource_metadata.cronjob as the default value is false

Signed-off-by: Tetiana Kravchenko <[email protected]>
@tetianakravchenko
Copy link
Contributor Author

tetianakravchenko commented Jan 10, 2024

Hey @MichaelKatsoulis I've extended the list of fields - 00cb450, similar to what we have in https://www.elastic.co/guide/en/fleet/current/kubernetes-provider.html, but with correct namespace fields

which is ok as an information but we don't list the field. Maybe we could list all fields above and leave those comments there as is.

does the commit above address this your comment?

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 8 min 24 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)


|`kubernetes.annotations.*`
|`object`
|Object of labels of the Node. Annotations are not available by default
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a typo here:

Suggested change
|Object of labels of the Node. Annotations are not available by default
|Object of annotations of the Node. Annotations are not available by default

Copy link
Contributor Author

@tetianakravchenko tetianakravchenko Jan 15, 2024

Choose a reason for hiding this comment

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

@belimawr done in this commit 2fbf545

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2024-01-15T18:05:50.040+0000

  • Duration: 8 min 10 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 8 min 3 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2024-01-18T12:01:16.792+0000

  • Duration: 9 min 54 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@tetianakravchenko tetianakravchenko merged commit ca03640 into elastic:main Jan 22, 2024
22 checks passed
@tetianakravchenko tetianakravchenko deleted the autodiscover-documentation branch January 22, 2024 10:06
Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024
….ip instead of host (elastic#37578)

* add missing kubernetes fields; update istio doc to use kubernetes.pod.ip instead of host

Signed-off-by: Tetiana Kravchenko <[email protected]>

* extend documentation with full list of fields; fix add_resource_metadata.deployment and add_resource_metadata.cronjob as the default value is false

Signed-off-by: Tetiana Kravchenko <[email protected]>

* test all variables and adjust documentation accordingly

Signed-off-by: Tetiana Kravchenko <[email protected]>

* Update libbeat/docs/shared-autodiscover.asciidoc

Co-authored-by: Andrew Gizas <[email protected]>

* Update libbeat/docs/shared-autodiscover.asciidoc

Co-authored-by: Andrew Gizas <[email protected]>

---------

Signed-off-by: Tetiana Kravchenko <[email protected]>
Co-authored-by: Andrew Gizas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants