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.

Sign up for GitHub

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

Jump to bottom

Added container.privileged field #2219

Merged
merged 3 commits into from
Jun 27, 2023
Merged

Added container.privileged field #2219

merged 3 commits into from
Jun 27, 2023

Conversation

Tacklebox
Copy link
Contributor

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • For proposing substantial changes or additions to the schema, have you reviewed the RFC process?
  • If submitting code/script changes, have you verified all tests pass locally using make test?
  • If submitting schema/fields updates, have you generated new artifacts by running make and committed those changes?
  • Is your pull request against main? Unless there is a good reason otherwise, we prefer pull requests against main and will backport as needed.
  • Have you added an entry to the CHANGELOG.next.md?

@Tacklebox Tacklebox requested review from mmat11 and mitodrummer June 20, 2023 11:49
@Tacklebox Tacklebox marked this pull request as ready for review June 20, 2023 11:49
@Tacklebox Tacklebox requested a review from a team as a code owner June 20, 2023 11:49
ebeahan
ebeahan approved these changes Jun 23, 2023
View reviewed changes
Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

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

LGTM once the conflicts are resolved.

@norrietaylor
Copy link
Member

Before we continue with this issue, I would like to engage @ChrsMark. This could overlap with some of the work for the OTEL initiative.

I also think that using the following might be more future-proof:
container.securityContext.privileged = true

There is a strong chance that future product requirements could involve adding other members of the securityContext.

@ChrsMark
Copy link
Member

Thanks @norrietaylor for the ping! I think this one should fine to be added in https://opentelemetry.io/docs/specs/otel/resource/semantic_conventions/container/ as well. I see no conflicts and looks quite straightforward.

Note: What we need to revisit on ECS side is the container.image.hash.all. See my analysis at open-telemetry/semantic-conventions#48 (comment).

@Tacklebox Tacklebox force-pushed the mborden/container.privileged branch from f57c5f9 to 37ac2a1 Compare June 27, 2023 12:13
@Tacklebox Tacklebox merged commit d5ea291 into main Jun 27, 2023
rylnd pushed a commit to rylnd/ecs that referenced this pull request Jul 12, 2023
@janstarke @ebeahan
fixes elastic#2219: remove the less documented domain field (elastic#…
47a4cde 
…2120)

* remove the less documented domain field

* Update CHANGELOG.next.md

Co-authored-by: Eric Beahan <[email protected]>
eyalkoren
eyalkoren reviewed Jul 31, 2023
View reviewed changes
schemas/container.yml
@@ -121,6 +121,13 @@
The number of bytes (gauge) sent out on all network interfaces by the
container since the last metric collection.

- name: privileged
type: bool
Copy link
Contributor

Choose a reason for hiding this comment

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

@Tacklebox shouldn't this be type: boolean?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eyalkoren eyalkoren eyalkoren left review comments

@mmat11 mmat11 mmat11 approved these changes

@ebeahan ebeahan ebeahan approved these changes

@mitodrummer mitodrummer Awaiting requested review from mitodrummer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Tacklebox @norrietaylor @ChrsMark @ebeahan @mmat11 @eyalkoren