feat: CCRN architecture hardening & code quality refactor by drochow · Pull Request #5 · cloudoperators/common-cloud-resource-names

drochow · 2026-06-11T12:36:58Z

Summary

Systematic refactoring of the CCRN codebase addressing architectural issues, bugs, and missing test coverage.

Key Changes

New packages:

pkg/ccrn/ — Zero-dependency types package (ParsedResource, Match, MatchAll, DefaultDomain). Consumers can import this without pulling in K8s dependencies.
pkg/parser/lite/ — Lightweight CCRN/URN parser with no K8s deps. Supports both ccrn= and urn:ccrn: formats with explicit templates.

Bug fixes:

Fix nil logger panics (discard logger default)
Fix broken error format strings in validator
Fix URN panic on malformed input (bounds checking)
Fix LoadCRDsFromDirectory to use filepath.WalkDir for recursive subdirectories
Fix CRD regex ^ccrn: .+ → ^ccrn=.+ to match actual CCRN format
Fix Dockerfile Go version mismatch (1.23 → 1.24)
Remove stray import "C" and duplicate SPDX headers

Architecture improvements:

Full parser (pkg/parser) now wraps lite parser — single parsing implementation, DRY
KubernetesBackend.ValidateResource uses dry-run create (no orphan resources)
Webhook no longer creates target resources during admission
RBAC scoped to explicit API groups (no wildcards), delete verb removed
Graceful shutdown with 15s drain on SIGTERM/SIGINT
Readiness gate: /healthz returns 503 until first CRD load succeeds
Deterministic ParsedResource.CCRN() output (alphabetical sort + strings.Builder)
Go-idiomatic naming (APIGroup, DefaultURNTemplate)

Test coverage (0 → 111 specs):

pkg/ccrn/ — 29 specs (types, formatting, wildcard matching)
pkg/parser/ — 8 specs (nil logger, URN bounds)
pkg/parser/lite/ — 30 specs (parsing, edge cases, round-trips)
pkg/validation/ — 24 specs (filesystem backend, recursive loading)
pkg/webhook/ — 20 specs (admission handler, readiness, shutdown)

Breaking Changes

Exported identifier renames: ApiGroup() → APIGroup(), DEFAULT_URN_TEMPLATE → DefaultURNTemplate
KubernetesBackend no longer creates real resources (uses dry-run)
RBAC no longer grants delete verb

Stats

25 files changed, +2091 / -201 lines
111 test specs across 5 packages
All tests passing, go vet clean

Pull request overview

This PR refactors CCRN parsing/validation to reduce dependencies, harden webhook/backend behavior, and add extensive test coverage across the project.

Changes:

Introduces a zero-dependency CCRN types/utilities package and a lightweight CCRN/URN parser, then refactors the main parser to wrap the lite implementation.
Hardens webhook/server behavior (readiness endpoint, graceful shutdown, dry-run validation) and tightens Helm RBAC/CRD validation patterns.
Adds broad unit/integration-style test coverage for parser/validation/webhook behavior.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
pkg/webhook/webhook.go	Adds HTTP server lifecycle management, readiness gating, and message updates.
pkg/webhook/webhook_test.go	Adds readiness/shutdown tests for the webhook server.
pkg/webhook/admission_test.go	Adds Ginkgo-based admission handler tests.
pkg/validation/validator.go	Fixes error formatting and updates default URN template constant usage.
pkg/validation/testdata/subdir/nested_crd.yaml	Adds nested CRD testdata to validate recursive directory loading.
pkg/validation/kubernetes_backend.go	Switches validation to dry-run create and hardens nil logger behavior.
pkg/validation/interface.go	Cleans up duplicated SPDX headers.
pkg/validation/filesystem_backend.go	Refactors directory loading to recursive WalkDir-based discovery.
pkg/validation/filesystem_backend_test.go	Adds coverage for recursive loading, nil logger safety, and error-message formatting.
pkg/parser/parser.go	Refactors parser to wrap lite parser and adds backend URN-template lookup.
pkg/parser/parser_test.go	Adds tests for nil logger handling and URN bounds checking.
pkg/parser/lite/parser.go	Adds new lightweight CCRN/URN parser implementation.
pkg/parser/lite/parser_test.go	Adds comprehensive lite parser tests, including round-trips.
pkg/parser/lite/parser_suite_test.go	Adds Ginkgo suite bootstrap for lite parser tests.
pkg/ccrn/ccrn.go	Adds zero-dependency CCRN ParsedResource utilities and wildcard matching.
pkg/ccrn/ccrn_test.go	Adds tests for deterministic formatting and group utilities.
pkg/ccrn/ccrn_suite_test.go	Adds Ginkgo suite bootstrap for ccrn package tests.
pkg/ccrn/wildcard_test.go	Adds wildcard matching behavior tests.
pkg/apis/validation.go	Updates interface docs to reflect dry-run validation behavior.
pkg/apis/types.go	Cleans up duplicated SPDX headers.
pkg/apis/parsed_resource.go	Makes CCRN string formatting deterministic; renames URNTemplate field and APIGroup accessor.
Dockerfile	Updates Go toolchain version used for builds.
cmd/webhook/main.go	Adds graceful shutdown and readiness handling.
ccrn-chart/templates/rbac.yaml	Tightens RBAC by scoping API groups and removing delete.
ccrn-chart/templates/crds/ccrn.yaml	Fixes CCRN regex pattern to match `ccrn=...` format.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    // Store the directory for potential refresh operations
+    fb.loadedPaths = append(fb.loadedPaths, dir)
+


+	// Mark server as ready after successful CRD load (performed inside NewWebhookServerFromConfig)
+	server.SetReady()


+	// Bind to an ephemeral port
+	ln, err := net.Listen("tcp", ":0")
+	if err != nil {
+		t.Fatalf("failed to listen: %v", err)
+	}


+	// Skip the first two segments (the <ccrn> placeholder which covers type.group/version)
+	// The first segment is typically "<ccrn>" or a literal type, second is version or part of <ccrn>
+	// Find where the field placeholders start (after the ccrn key segments)
+	// The <ccrn> placeholder in the backend template covers "type.group/version" (2 slash-separated segments)
+	// So we skip the first 2 segments
+	var fieldSegments []string
+	if len(segments) > 2 {
+		fieldSegments = segments[2:]
+	} else if len(segments) > 1 {
+		// Template like "urn:ccrn:<ccrn>/<field>" - segments[0] is <ccrn> part 1, segments[1] could be version or field
+		// But with <ccrn> taking two segments, there's nothing left
+		return ""
+	} else {
+		return ""
 	}


+			template, err := p.backend.GetURNTemplate(ccrnName, ccrnVersion)
 			if err != nil {
 				return nil, fmt.Errorf("failed to get URN template: %w", err)
 			}


+	// Bind to an ephemeral port
+	ln, err := net.Listen("tcp", ":0")
+	if err != nil {
+		t.Fatalf("failed to listen: %v", err)
+	}
+	addr := ln.Addr().String()
+


Systematic refactoring addressing architectural issues, bugs, and missing test coverage in the CCRN codebase. New packages: - pkg/ccrn/ — Zero-dependency types package (ParsedResource, Match, MatchAll, DefaultDomain). Consumers can import without K8s deps. - pkg/parser/lite/ — Lightweight CCRN/URN parser, no K8s dependencies. Supports both ccrn= and urn:ccrn: formats with explicit templates. Bug fixes: - Fix nil logger panics (discard logger default) - Fix broken error format strings in validator - Fix URN panic on malformed input (bounds checking) - Fix LoadCRDsFromDirectory to use filepath.WalkDir for recursion - Fix CRD regex ^ccrn: .+ to ^ccrn=.+ to match actual format - Fix Dockerfile Go version mismatch (1.23 -> 1.24) - Remove stray import "C" and duplicate SPDX headers Architecture improvements: - Full parser (pkg/parser) wraps lite — single implementation, DRY - KubernetesBackend.ValidateResource uses dry-run create (no orphans) - Webhook no longer creates target resources during admission - RBAC scoped to explicit API groups (no wildcards), delete verb removed - Graceful shutdown with 15s drain on SIGTERM/SIGINT - Readiness gate: /healthz returns 503 until first CRD load succeeds - Deterministic ParsedResource.CCRN() output (alphabetical sort) - Go-idiomatic naming (APIGroup, DefaultURNTemplate) Test coverage (0 -> 111 specs): - pkg/ccrn/ — 29 specs - pkg/parser/ — 8 specs - pkg/parser/lite/ — 30 specs - pkg/validation/ — 24 specs - pkg/webhook/ — 20 specs BREAKING CHANGE: Exported identifier renames (ApiGroup -> APIGroup, DEFAULT_URN_TEMPLATE -> DefaultURNTemplate). KubernetesBackend no longer creates real resources (uses dry-run). RBAC no longer grants delete verb. Signed-off-by: David Rochow <david.rochow@sap.com>

Copilot

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

pkg/parser/parser.go:71

When urnTemplate is empty/default, Parse() looks up a template via backend.GetURNTemplate(ccrnName, ccrnVersion). The current backend implementations expect a CRD name (metadata.name, e.g. clusters.vmware.<domain>), not the CCRN name (kind.group, e.g. cluster.vmware.<domain>), so this lookup will fail for standard CRDs. Consider using backend.GetCRD(ccrnKey) and CRDInfo.URNFormat for template lookup instead.

			// Split ccrnKey into name and version for the backend lookup
			parts := strings.SplitN(ccrnKey, "/", 2)
			if len(parts) < 2 {
				return nil, errors.New("invalid URN format: CCRN key must contain type and version separated by /")
			}
			ccrnName := parts[0]
			ccrnVersion := parts[1]

			template, err := p.backend.GetURNTemplate(ccrnName, ccrnVersion)
			if err != nil {
				return nil, fmt.Errorf("failed to get URN template: %w", err)
			}
			return p.Parse(input, template)

    // Reload from all previously loaded paths
+    // Paths may be glob patterns (from LoadCRDs) or directories (from LoadCRDsFromDirectory)
    var allErrors []error
    for _, path := range fb.loadedPaths {
-        if err := fb.LoadCRDs(path); err != nil {
-            allErrors = append(allErrors, fmt.Errorf("failed to refresh path %s: %w", path, err))
+        info, statErr := os.Stat(path)


+    // Walk the directory tree to find all YAML files
+    var yamlFiles []string
+    err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error {
+        if err != nil {
+            return err


+	// Skip the first segment which is the <ccrn> placeholder
+	var fieldSegments []string
+	if len(segments) > 1 {
+		fieldSegments = segments[1:]
+	} else {
+		return ""
 	}


 		s.log.Infof("CCRN is present, generating URN from CCRN")
 		template, err := s.backend.GetURNTemplate(parsedCCRN.CCRNName(), parsedCCRN.Version())
 		if err != nil {
-			s.log.Errorf("Failed to get URN template for %s/%s: %v", parsedCCRN.ApiGroup(), parsedCCRN.Version(), err)
+			s.log.Errorf("Failed to get URN template for %s/%s: %v", parsedCCRN.APIGroup(), parsedCCRN.Version(), err)
 			return nil, false


+	// Mark server as ready — NewWebhookServerFromConfig performs the initial
+	// backend.Refresh() which populates the CRD cache. If it failed critically,
+	// NewWebhookServerFromConfig would have returned an error above.
+	server.SetReady()


+				Errors:     []string{fmt.Sprintf("A CCRN definition for %s could not be retrieved: %s", parsed.CCRNKey(), err.Error())},
 			}, err
 		}
 		parsed, err = v.parser.Parse(ccrnStr, info.URNFormat)


Copilot AI review requested due to automatic review settings June 11, 2026 12:36

drochow requested review from a team and giobbe as code owners June 11, 2026 12:36

Copilot started reviewing on behalf of drochow June 11, 2026 12:37 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings June 11, 2026 13:44

drochow force-pushed the feat/ccrn-refactor branch from 3be1f80 to 0d28979 Compare June 11, 2026 13:44

Copilot started reviewing on behalf of drochow June 11, 2026 13:44 View session

Automatic application of license header

b4d91fa

Copilot AI reviewed Jun 11, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: CCRN architecture hardening & code quality refactor#5

feat: CCRN architecture hardening & code quality refactor#5
drochow wants to merge 2 commits into
mainfrom
feat/ccrn-refactor

drochow commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Store the directory for potential refresh operations
		fb.loadedPaths = append(fb.loadedPaths, dir)

		// Mark server as ready after successful CRD load (performed inside NewWebhookServerFromConfig)
		server.SetReady()

Conversation

drochow commented Jun 11, 2026

Summary

Key Changes

Breaking Changes

Stats

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants