feat: add OpenMetadataEntityTag CRD#5
Conversation
Introduces a new CRD for declaratively tagging OpenMetadata entities discovered by ingestion (tables, topics, schemas, etc.). The reconciler queries OM's search endpoint for entities matching FQN patterns and applies a tag via the bulk tag-asset endpoints, recording assignments in status to drive drift detection and rename cleanup.
There was a problem hiding this comment.
Pull request overview
Adds a new OpenMetadataEntityTag custom resource + controller path to declaratively apply/remove OpenMetadata tags on entities discovered via ingestion (i.e., not created by this operator), using OpenMetadata search + bulk tag-asset endpoints.
Changes:
- Introduces the
OpenMetadataEntityTagCRD/API types (match entity type + FQN patterns, tag FQN, connection ref) and wires the controller intocmd/main.go. - Implements reconciler/handler logic to search matched entities, diff vs.
status.tagAssignments, and bulk add/remove tags (including rename + finalizer cleanup paths). - Extends the OpenMetadata client package with search + bulk tag-asset operations and adds unit/controller tests plus RBAC/CRD manifests.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/omclient/types.go | Adds search/bulk-tag request/response types used by entity-tagging. |
| internal/omclient/interface.go | Introduces EntityTagClient interface for tagging/search operations. |
| internal/omclient/entitytag.go | Implements search pagination/query building and bulk add/remove tag-asset calls. |
| internal/handler/entitytag_index.go | Maps supported entity types to OpenMetadata search index names. |
| internal/handler/entitytag_handler.go | Core Observe→Compare→Converge logic, rename handling, and deletion finalizer cleanup. |
| internal/handler/entitytag_handler_test.go | Unit tests for diffing, recorded tag lookup, and index resolution. |
| internal/controller/openmetadataentitytag_controller.go | New controller wiring finalizer + delegation to handler. |
| internal/controller/openmetadataentitytag_controller_test.go | Envtest coverage for controller flow (finalizer, apply, delete cleanup). |
| config/rbac/role.yaml | Grants manager-role access to the new CR and its status/finalizers. |
| config/rbac/openmetadataentitytag_viewer_role.yaml | Adds read-only ClusterRole for the new CR. |
| config/rbac/openmetadataentitytag_editor_role.yaml | Adds editor ClusterRole for the new CR. |
| config/rbac/openmetadataentitytag_admin_role.yaml | Adds admin ClusterRole for the new CR. |
| config/rbac/kustomization.yaml | Includes the new RBAC role manifests. |
| config/crd/kustomization.yaml | Includes the new CRD base. |
| config/crd/bases/openmetadata.vortexa.com_openmetadataentitytags.yaml | New generated CRD manifest for OpenMetadataEntityTag. |
| cmd/main.go | Registers the new controller with the manager. |
| api/v1alpha1/zz_generated.deepcopy.go | Generated deepcopy updates for new API types. |
| api/v1alpha1/tag_types.go | Adds TagRef API type. |
| api/v1alpha1/openmetadataentitytag_types.go | Adds OpenMetadataEntityTag API types/spec/status. |
| api/v1alpha1/conditions.go | Adds new condition reasons for entity-tagging reconcile outcomes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kujon
left a comment
There was a problem hiding this comment.
Code Review: OpenMetadataEntityTag CRD (PR #5)
Overview
This PR introduces a new OpenMetadataEntityTag CRD that enables declarative tagging of OpenMetadata entities discovered by ingestion pipelines (tables, topics, schemas, databases, dashboards, ML models, pipelines, containers, and search indexes). The implementation includes:
- CRD definition with FQN pattern matching (includes/excludes using wildcards)
- Reconciliation controller that queries OpenMetadata's search API, computes diffs, and applies bulk tag operations
- Status tracking to detect drift and handle tag renames
- Finalizer support for cleanup on deletion
- Comprehensive test coverage for both controller and handler layers
Code Quality: Excellent
The implementation demonstrates strong engineering practices:
✅ Clean architecture — thin controller delegates to a focused handler
✅ Testable design — dependency injection via NewOMClient function
✅ Bulk operations — efficient single HTTP call per reconcile regardless of matched entity count
✅ Status invariant — all assignments share the same tagFQN, simplifying rename detection
✅ Comprehensive error handling with detailed conditions and event recording
✅ Well-documented — inline comments explain non-obvious design decisions
Specific Feedback
1. Search Query Escaping (internal/omclient/entitytag.go:1863)
The escapeQueryValue function properly escapes Lucene special characters while preserving wildcards. However:
Observation: The space character isn't escaped. While this is likely correct for OpenMetadata's keyword field behavior, it might be worth a comment clarifying that spaces in FQNs are literal (not treated as token boundaries in the fullyQualifiedName field).
Suggestion (optional):
// escapeQueryValue escapes characters reserved by OpenMetadata's search
// query syntax inside a pattern, while preserving the wildcards '*' and '?'.
// Spaces are NOT escaped because the fullyQualifiedName field is indexed as
// a keyword (no tokenization), so spaces are matched literally.
// Reserved chars: + - = & | > < ! ( ) { } [ ] ^ " ~ : \ /2. Rename Detection Logic (internal/handler/entitytag_handler.go:1249)
The rename detection is elegant but could be clearer about the invariant it relies on:
if oldTagFQN := recordedTagFQN(et); oldTagFQN != "" && oldTagFQN != tagFQN {Suggestion: Add a comment before this block explaining that the status invariant (all assignments share one tagFQN) makes this a simple string comparison:
// Rename detected: status records assignments under a different tag than
// spec now wants. The status invariant guarantees all assignments share
// one tagFQN, so comparing the first entry's tag against spec is sufficient.
if oldTagFQN := recordedTagFQN(et); oldTagFQN != "" && oldTagFQN != tagFQN {3. Search Pagination (internal/omclient/entitytag.go:1781)
The pagination implementation is correct but has no hard stop. If OpenMetadata returns corrupted responses that never drop below searchPageSize, this could loop indefinitely.
Suggestion: Add a safety cap:
const maxSearchPages = 100 // 100k entities at 1k per page
var (
out []EntitySummary
from int
pages int
)
for {
if pages >= maxSearchPages {
return nil, fmt.Errorf("search exceeded max pages (%d); query may be too broad or API is misbehaving", maxSearchPages)
}
params := url.Values{}
// ... existing code ...
pages++
}4. Error Handling in Deletion (internal/handler/entitytag_handler.go:1342)
The deletion handler gracefully handles 404s for missing tags, but other errors (network failures, auth failures) will cause the finalizer to block deletion indefinitely.
Observation: This is probably the right behavior (don't delete the CR until we've confirmed cleanup), but it might be worth logging a more actionable message or emitting an event so users know why deletion is stuck.
Suggestion (optional enhancement):
if err != nil && !omclient.IsNotFound(err) {
logger.Error(err, "Failed to resolve tag for deletion", "tagFQN", tagFQN)
h.emitEvent(et, corev1.EventTypeWarning, omv1alpha1.ReasonTagResolutionFailed,
fmt.Sprintf("Cannot complete deletion: tag lookup failed for %s", tagFQN))
return ctrl.Result{}, err
}5. RBAC Markers (internal/controller/openmetadataentitytag_controller.go:864)
The RBAC markers correctly declare all required permissions, including dependent resources (openmetadataconnections, secrets, events). This matches the pattern from other controllers in the repo. ✅
Security Considerations
✅ No injection vulnerabilities — query escaping is correct
✅ Auth token handling — reuses existing secure resolveAuthToken helper
✅ No secrets in status — only entity IDs and FQNs are persisted
✅ RBAC properly scoped — read-only access to secrets, connections
Performance Implications
✅ Efficient bulk operations — single HTTP call per reconcile
✅ Pagination handled — won't OOM on large result sets
Observed behavior: The reconcile interval defaults to 5 minutes (inherited from requeueInterval in internal/handler/common.go). For clusters with thousands of entities, search queries against OpenMetadata's Elasticsearch backend should still be sub-second, so this is appropriate.
Test Coverage
✅ Controller tests (internal/controller/openmetadataentitytag_controller_test.go)
- Finalizer addition
- Tag application to matched entities
- Tag removal on deletion
✅ Handler tests (inferred from the diff structure)
Coverage appears comprehensive — the stub client pattern allows testing the full reconcile loop without real OpenMetadata API calls.
Project Conventions
✅ Follows existing patterns from OpenMetadataService, IngestionPipeline, OpenMetadataTestCase
✅ Conventional commit format used in recent commits
✅ CRD markers consistent with other resources
✅ RBAC role split (admin/editor/viewer) matches project structure
Potential Issues / Risks
Low Risk:
- Search pagination cap missing — could loop indefinitely on corrupted API responses (see suggestion #3)
- Deletion may block forever — if tag resolution fails during finalizer cleanup (see suggestion #4)
Medium Risk:
3. No validation for wildcard patterns — users could accidentally write overlapping patterns (e.g., both svc.* and svc.prod.* includes). This won't break anything but might be confusing. Consider adding a validation webhook or doc guidance.
No High-Risk Issues Identified
Recommendations
Before merge:
- ✅ Add search pagination cap (suggestion #3) — prevents runaway loops
- ✅ Improve deletion error visibility (suggestion #4) — helps users debug stuck finalizers
Post-merge / future enhancements:
3. Consider adding a validation webhook to warn about overlapping patterns
4. Consider exposing reconcile interval as a configurable field on the CR (currently hardcoded to 5m)
5. Add metrics for tag operations (entities matched, tags applied/removed per reconcile)
Summary
Verdict: LGTM with minor suggestions
This is production-ready code with excellent test coverage and clean design. The bulk operation strategy is efficient, the status invariant simplifies state management, and the rename detection is elegant. The two minor suggestions (#3 and #4) would further harden the implementation against edge cases, but the PR is mergeable as-is.
Estimated complexity: ~1800 LOC added, no deletions — substantial feature but well-contained within the existing operator architecture.
Great work! 🚀
|
ignore the suggestion #3 from Claude - you are already comparing with |
Description
Adds a new
OpenMetadataEntityTagCRD for declaratively tagging OpenMetadata entities the operator does not create directly (tables, topics, databaseSchemas, databases, dashboards, mlmodels, pipelines, containers, searchIndexes), i.e. entities discovered by OM's ingestion pipelines.Each CR specifies:
spec.match.entityType: which OM entity type to targetspec.match.includes/excludes: Lucene-style FQN patterns (*and?wildcards) selecting the assets in scopespec.tag.tagFQN: the tag to apply (e.g.Tier.Tier3)spec.openMetadataConnectionRef: the cluster-scoped connectionThe reconciler:
/v1/search/queryendpoint with the include/exclude patterns to find currently-matching entitiesstatus.TagAssignments(what we applied last reconcile) and either:status.TagAssignments, sorted by FQNWhy
OM's ingestion pipelines write entities directly to the OM API, bypassing Kubernetes, so we have no native way to declaratively tag them.
Design notes
PUT /v1/tags/{id}/assets/addand.../remove, which take a tag UUID + list of asset references. One HTTP call per CR per reconcile, regardless of how many entities match.status.TagAssignmentsshares the sametagFQN. Simplifies rename detection (one comparison) and deletion (one bulk-remove).