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

Adding cluster.id to the orchestrator field set #1875

Merged
merged 2 commits into from
Apr 13, 2022
Merged

Conversation

amitkanfer
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?

@amitkanfer amitkanfer requested a review from a team as a code owner April 13, 2022 07:49
Copy link
Contributor

@MikePaquette MikePaquette left a comment

Choose a reason for hiding this comment

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

Hi @amitkanfer thanks for submitting this pull request!

It looks straightforward to me. LGTM, based on the following rationale:

  • The cluster object already exists within orchestrator.cluster.name and other fields.
  • It is very common, almost universal, for ECS to have an id field associated with a name field.
  • Data type keyword is common for id fields
  • Extended level makes sense becuase usage will be limited to a subset of use cases (those dealing with Kubernetes or other container orchestrators.)

@MikePaquette
Copy link
Contributor

@ebeahan Can you have a look and let us know what you think?

@MikePaquette MikePaquette requested a review from ebeahan April 13, 2022 11:27
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! Thanks, @amitkanfer!

I'll add an entry to CHANGELOG.next and merge.

@ebeahan ebeahan merged commit aae9f23 into main Apr 13, 2022
@ebeahan ebeahan deleted the add_cluster_id branch April 13, 2022 15:20
ebeahan pushed a commit to ebeahan/ecs that referenced this pull request Apr 13, 2022
* Adding cluster.id to the orchestrator field set

* add a CHANGELOG entry

Co-authored-by: Eric Beahan <[email protected]>
(cherry picked from commit aae9f23)

# Conflicts:
#	experimental/generated/csv/fields.csv
#	generated/csv/fields.csv
@ebeahan
Copy link
Member

ebeahan commented Apr 13, 2022

💚 All backports created successfully

Status Branch Result
8.3

Questions ?

Please refer to the Backport tool documentation

ebeahan added a commit that referenced this pull request Apr 13, 2022
* Adding cluster.id to the orchestrator field set

* add a CHANGELOG entry

Co-authored-by: Eric Beahan <[email protected]>
(cherry picked from commit aae9f23)

Co-authored-by: Amit Kanfer <[email protected]>
@eyalkraft
Copy link

@amitkanfer Following a chat with @m-sample as we're aligned about using the kube-system namespace uuid as the identifier here, wdyt about documenting this here in ECS? I'll open a PR unless we've missed something.

This aligns with the requirement for ECS specs to be useful for both data consumers and data providers.
Documenting this field allows anyone to write their own sensor and populate the orchestrator.cluster.id field with values that match those produced by our sensors. (thanks @m-sample, @ebeahan)

@amitkanfer
Copy link
Contributor Author

i think it's a good idea. Do we want to further define it as a f(kube-system) where f is a hash function of some sort?
if we don't do so - the only way to differentiate between a cluster_id and the namespace_id (in a case where we would collect it) - is that the cluster_id will be under orchestrator.cluster.id and the namespace will probably be under orchestrator.resource.id and the corresponding type to indicate it's a namespace.

Makes sense? Do we want to go forward w/o the hashing function? @eyalkraft @m-sample

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants