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

Prototype: Use Weaver to enforce semantic convention policies #1014

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented May 8, 2024

This is a prototype of using Weaver to enforce semantic convention policies via the experimental Open-Policy-Agent support.

This defines three policies:

  • nonregistry_with_id_attr: We only allow new attribute definitions inside the attribute registry, all other attributes must be ref.
  • registry_must_be_attribute_group: All groups in attribute registry MUST be type attribute_group.
  • registry_with_ref_attr: Attribute registry is not allowed to contain ref - An attribute can only belong to one group.

Example output on today's repository:

$ make check-policies 
docker run --rm -v /home/joshuasuereth/src/open-telemetry/semantic-conventions/model:/source -v /home/joshuasuereth/src/open-telemetry/semantic-conventions/docs:/spec -v /home/joshuasuereth/src/open-telemetry/semantic-conventions/policies:/policies \
	otel/weaver:latest registry check \
	--registry=/source \
	--before-resolution-policies=/policies/before_resolution/registry.rego
✔ SemConv registry loaded (129 files)
✖ Policy violation: id=nonregistry_with_id_attr, category=attribute_registry, group=attributes.jvm.buffer, attr=pool.name, provenance: /source/metrics/jvm-metrics-experimental.yaml
✖ Policy violation: id=nonregistry_with_id_attr, category=attribute_registry, group=metric.process.context_switches, attr=process.context_switch_type, provenance: /source/metrics/process-metrics.yaml
✖ Policy violation: id=nonregistry_with_id_attr, category=attribute_registry, group=metric.process.paging.faults, attr=process.paging.fault_type, provenance: /source/metrics/process-metrics.yaml
✖ Policy violation: id=nonregistry_with_id_attr, category=attribute_registry, group=attributes.process.cpu, attr=state, provenance: /source/metrics/process-metrics.yaml
make: *** [Makefile:38: check-policies] Error 1

Each of these current violations are something we should fix.

Changes

  • Add new check-policies command to the makefile which can check weaver policies
  • Add policies/before_resolution/ folder for policies we can enforce on raw YAML model.

Merge requirement checklist

@@ -0,0 +1,37 @@
package otel

# A registry `attribute_group` containing at least one `ref` attribute is
Copy link
Contributor

Choose a reason for hiding this comment

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

not blocking: would it be possible to link some docs on the policy syntax and the properties available on group/attribute/etc?

Copy link

Choose a reason for hiding this comment

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

@lmolkova, you will find some documentation I wrote on this topic here https://github.com/open-telemetry/weaver/tree/main/crates/weaver_checker

The definition of the attributes for the violation entity is defined here https://github.com/open-telemetry/weaver/blob/main/crates/weaver_checker/src/violation.rs

In general, the policy file is based on the Rego language and its documentation is available here https://www.openpolicyagent.org/docs/latest/policy-language/

# considered invalid if it's not in the registry group.
deny[attr_registry_violation("registry_with_ref_attr", group.id, attr.ref)] {
group := input.groups[_]
startswith(group.id, "registry.")
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking, just some thoughts for the future:

I hope we will invent a more formal way to define a registry.
E.g.

  • now someone can add registry.foo.bar in any place in the repo.
  • or we'll add metrics registry and will need a different prefix.
  • the attribute definition in the registry won't even have ref from the schema perspective
  • etc

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

Successfully merging this pull request may close these issues.

None yet

3 participants