From d182f9e66f48d854e8afcc74a0962bad89f9dd3c Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Thu, 1 Aug 2024 05:27:51 +0900 Subject: [PATCH] Add attribute name and const names checks (#1209) --- .github/workflows/checks.yml | 7 ++++ Makefile | 8 ++++ policies/attribute_name_collisions.rego | 56 +++++++++++++++++++++++++ policies/registry.rego | 33 ++++++++++++--- 4 files changed, 98 insertions(+), 6 deletions(-) create mode 100644 policies/attribute_name_collisions.rego diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index a889670d86..ceaf8ef7f6 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -108,3 +108,10 @@ jobs: run: | make generate-gh-issue-templates git diff --exit-code '.github/ISSUE_TEMPLATE' || (echo 'Dropdowns in issue templates is out of date, please run "make generate-gh-issue-templates" and commit the changes in this PR. Please note, if you are running it on Mac OS X, please make sure to use GNU version of sed instead of default "sed".' && exit 1) + + policies-check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v1 + - name: verify semantic conventions yaml definitions + run: make check-policies diff --git a/Makefile b/Makefile index 06e7b7d1b1..80772401b4 100644 --- a/Makefile +++ b/Makefile @@ -228,3 +228,11 @@ chlog-update: $(CHLOGGEN) .PHONY: generate-gh-issue-templates generate-gh-issue-templates: $(TOOLS_DIR)/scripts/update-issue-template-areas.sh + +.PHONY: check-policies +check-policies: + docker run --rm -v $(PWD)/model:/source -v $(PWD)/docs:/spec -v $(PWD)/policies:/policies \ + otel/weaver:${WEAVER_VERSION} registry check \ + --registry=/source \ + --policy=/policies/registry.rego \ + --policy=/policies/attribute_name_collisions.rego diff --git a/policies/attribute_name_collisions.rego b/policies/attribute_name_collisions.rego new file mode 100644 index 0000000000..3d38dab177 --- /dev/null +++ b/policies/attribute_name_collisions.rego @@ -0,0 +1,56 @@ +package after_resolution + +deny[attr_registry_collision(description, name)] { + names := attr_names_except(excluded_const_collisions) + name := names[_] + const_name := to_const_name(name) + collisions:= { n | n := attr_names_except(excluded_const_collisions)[_]; n != name; to_const_name(n) == const_name } + count(collisions) > 0 + + # TODO (https://github.com/open-telemetry/weaver/issues/279): provide other violation properties once weaver supports it. + description := sprintf("Attribute '%s' has the same constant name '%s' as '%s'.", [name, const_name, collisions]) +} + +deny[attr_registry_collision(description, name)] { + names := attr_names_except(excluded_namespace_collisions) + name := names[_] + + collisions:= { n | n := input.groups[_].attributes[_].name; startswith(n, to_namespace_prefix(name)) } + count(collisions) > 0 + + # TODO (https://github.com/open-telemetry/weaver/issues/279): provide other violation properties once weaver supports it. + description := sprintf("Attribute '%s' name is used as a namespace in the following attributes '%s'.", [name, collisions]) +} + +attr_registry_collision(description, attr_name) = violation { + violation := { + "id": description, + "type": "semconv_attribute", + "category": "naming_collision", + "attr": attr_name, + "group": "", + } +} + +to_namespace_prefix(name) = namespace { + namespace := concat("", [name, "."]) +} + +to_const_name(name) = const_name { + const_name := replace(name, ".", "_") +} + +attr_names_except(excluded) = names { + names := { n | n := input.groups[_].attributes[_].name } - excluded +} + +# These lists contain exceptions for existing collisions that were introduced unintentionally. +# We'll have a way to specify how collision resolution happens in the schema - +# see phase 2 in https://github.com/open-telemetry/semantic-conventions/issues/1118#issuecomment-2173803006 +# For now we'll exclude existing collisions from the checks. +# ADDING NEW EXCEPTIONS IS NOT ALLOWED. + +# DO NOT ADD ATTRIBUTES TO THIS LIST +excluded_const_collisions := {"messaging.client_id"} +# DO NOT ADD ATTRIBUTES TO THIS LIST +excluded_namespace_collisions := {"messaging.operation", "db.operation", "deployment.environment"} diff --git a/policies/registry.rego b/policies/registry.rego index cc30c75f48..a1925c0887 100644 --- a/policies/registry.rego +++ b/policies/registry.rego @@ -5,10 +5,10 @@ package before_resolution # used by semantic conventions. # Helper to create attribute registry violations. -attr_registry_violation(violation_id, group_id, attr_id) = violation { +attr_registry_violation(description, group_id, attr_id) = violation { violation := { - "id": violation_id, - "type": "semantic_convention_policies", + "id": description, + "type": "semconv_attribute", "category": "attribute_registry_checks", "group": group_id, "attr": attr_id, @@ -16,27 +16,48 @@ attr_registry_violation(violation_id, group_id, attr_id) = violation { } # We only allow attribute groups in the attribute registry. -deny[attr_registry_violation("attribute_registry_can_only_contain_attribute_groups", group.id, "")] { +deny[attr_registry_violation(description, group.id, "")] { group := input.groups[_] startswith(group.id, "registry.") group.type != "attribute_group" + + # TODO (https://github.com/open-telemetry/weaver/issues/279): provide other violation properties once weaver supports it. + # violation_id := "attribute_registry_can_only_contain_attribute_groups" + description := sprintf("Registry group '%s' has invalid type '%s'. Groups in attribute registry must have `attribute_group` type.", [group.id, group.type]) } # Any group that is NOT in the attribute registry that has an attribute id is # in violation of not using the attribute registry. -deny[attr_registry_violation("attributes_must_be_defined_in_attribute_registry", group.id, attr.id)] { +deny[attr_registry_violation(description, group.id, attr.id)] { group := input.groups[_] not startswith(group.id, "registry.") attr := group.attributes[_] attr.id != null + + attr_name := get_attribute_name(attr, group) + + # TODO (https://github.com/open-telemetry/weaver/issues/279): provide other violation properties once weaver supports it. + # violation_id := "attributes_must_be_defined_in_attribute_registry" + description := sprintf("Attribute '%s' is defined in the group '%s' which is not part of the attribute registy. Attributes can be defined in the registry group only.", [attr_name, group.id]) } # A registry `attribute_group` containing at least one `ref` attribute is # considered invalid if it's not in the registry group. -deny[attr_registry_violation("attributes_in_registry_cannot_reference_each_other", group.id, attr.ref)] { +deny[attr_registry_violation(description, group.id, attr.ref)] { # TODO - this will need to be updated to support `embed` in the future. group := input.groups[_] startswith(group.id, "registry.") attr := group.attributes[_] attr.ref != null + + # TODO (https://github.com/open-telemetry/weaver/issues/279): provide other violation properties once weaver supports it. + # violation_id := "attributes_in_registry_cannot_reference_each_other" + description := sprintf("Registy group '%s' references attribute '%s'. Registry groups can only define new attributes.", [group.id, attr.ref]) +} + +get_attribute_name(attr, group) = name { + full_name = concat(".", [group.prefix, attr.id]) + + # if there was no prefix, we have a leading dot + name := trim(full_name, ".") }