fix: improve error handling and provider registry consistency

isometry · isometry · commit 981e8741e561 · 2026-02-12T14:31:23.000+01:00
- Add include depth limit to config includes
- Improve error handling for flag parsing and TLS/HTTP providers
- Make provider registry internal and update references
- Update Vault provider docs for lowercase health fields
- Return error when all container components fail to resolve
- Add default HTTP status check if no CEL checks configured
- Fix time CEL function panics on wrong type
- Update example config for Kubernetes kind field
- Miscellaneous doc and comment improvements
diff --git a/pkg/checks/README.md b/pkg/checks/README.md
@@ -19,7 +19,7 @@ ph context my-app -o yaml
 
 ## CEL Expression Syntax
 
-CEL expressions must evaluate to a boolean (`true` for healthy, `false` for unhealthy). Expressions have access to provider-specific context variables (e.g., `response` for REST, `resource` for Kubernetes).
+CEL expressions must evaluate to a boolean (`true` for healthy, `false` for unhealthy). Expressions have access to provider-specific context variables (e.g., `response` for HTTP, `resource` for Kubernetes).
 
 ## Common CEL Patterns
 
diff --git a/pkg/checks/cel.go b/pkg/checks/cel.go
@@ -249,7 +249,11 @@ func ParseConfig(raw any) ([]Expression, error) {
 // ValidateExpression validates CEL expression syntax at configuration time.
 // Variables should be declared using cel.Variable() options.
 func ValidateExpression(expression string, variables ...cel.EnvOption) error {
-	env, err := cel.NewEnv(append(variables, standardExtensions...)...)
+	allOpts := make([]cel.EnvOption, 0, len(standardFunctions)+len(variables)+len(standardExtensions))
+	allOpts = append(allOpts, standardFunctions...)
+	allOpts = append(allOpts, variables...)
+	allOpts = append(allOpts, standardExtensions...)
+	env, err := cel.NewEnv(allOpts...)
 	if err != nil {
 		return fmt.Errorf("failed to create CEL environment: %w", err)
 	}
@@ -262,7 +266,7 @@ func ValidateExpression(expression string, variables ...cel.EnvOption) error {
 }
 
 // EvaluateAny compiles and evaluates a CEL expression returning its result.
-// Unlike Evaluate(), this does not require boolean output - any type is allowed.
+// Unlike Check.Evaluate(), this does not require boolean output - any type is allowed.
 // The result is converted to native Go types for serialization.
 // Note: Uses the cached environment but compiles the AST directly (no caching)
 // since the boolean output validation in getOrCompileAST() doesn't apply here.
diff --git a/pkg/checks/functions/time.go b/pkg/checks/functions/time.go
@@ -27,7 +27,10 @@ func Time() []cel.EnvOption {
 				[]*cel.Type{cel.TimestampType},
 				cel.DurationType,
 				cel.UnaryBinding(func(arg ref.Val) ref.Val {
-					ts := arg.(types.Timestamp)
+					ts, ok := arg.(types.Timestamp)
+					if !ok {
+						return types.NewErr("time.Since: expected timestamp, got %T", arg)
+					}
 					return types.Duration{Duration: time.Since(ts.Time)}
 				}),
 			),
@@ -38,7 +41,10 @@ func Time() []cel.EnvOption {
 				[]*cel.Type{cel.TimestampType},
 				cel.DurationType,
 				cel.UnaryBinding(func(arg ref.Val) ref.Val {
-					ts := arg.(types.Timestamp)
+					ts, ok := arg.(types.Timestamp)
+					if !ok {
+						return types.NewErr("time.Until: expected timestamp, got %T", arg)
+					}
 					return types.Duration{Duration: time.Until(ts.Time)}
 				}),
 			),
diff --git a/pkg/commands/check/check.go b/pkg/commands/check/check.go
@@ -115,11 +115,8 @@ func setup(cmd *cobra.Command, _ []string) (err error) {
 		return err
 	}
 
-	// In strict mode, fail if any configuration errors were found
-	if strict {
-		if err := result.EnforceStrict(log); err != nil {
-			return err
-		}
+	if err := result.EnforceStrict(log); err != nil {
+		return err
 	}
 
 	conf = result
diff --git a/pkg/commands/context/context.go b/pkg/commands/context/context.go
@@ -138,8 +138,14 @@ func displayContext(cmd *cobra.Command, checkProvider provider.InstanceWithCheck
 	v := phctx.Viper(cmd.Context())
 	output := v.GetString("output-format")
 
-	defaultExprs, _ := cmd.Flags().GetStringArray("expr")
-	eachExprs, _ := cmd.Flags().GetStringArray("expr-each")
+	defaultExprs, err := cmd.Flags().GetStringArray("expr")
+	if err != nil {
+		return fmt.Errorf("failed to get expr flag: %w", err)
+	}
+	eachExprs, err := cmd.Flags().GetStringArray("expr-each")
+	if err != nil {
+		return fmt.Errorf("failed to get expr-each flag: %w", err)
+	}
 
 	if len(defaultExprs) > 0 || len(eachExprs) > 0 {
 		celConfig := checkProvider.GetCheckConfig()
diff --git a/pkg/commands/server/server.go b/pkg/commands/server/server.go
@@ -59,11 +59,8 @@ func setup(cmd *cobra.Command, args []string) (err error) {
 		return err
 	}
 
-	// In strict mode, fail if any configuration errors were found
-	if strict {
-		if err := result.EnforceStrict(log); err != nil {
-			return err
-		}
+	if err := result.EnforceStrict(log); err != nil {
+		return err
 	}
 
 	conf = result
diff --git a/pkg/config/includes.go b/pkg/config/includes.go
@@ -50,6 +50,8 @@ func (s IncludeStack) CycleString(cyclePath string) string {
 	return strings.Join(parts, " -> ")
 }
 
+const maxIncludeDepth = 10
+
 // ProcessIncludes recursively processes includes in a config map.
 // basePath is the directory containing the current config file.
 // stack is used for loop detection via content hashing.
@@ -58,6 +60,10 @@ func (s IncludeStack) CycleString(cyclePath string) string {
 // (not replaced), and scalar values are overridden by later includes.
 // The local config (non-included content) takes highest priority.
 func ProcessIncludes(configMap map[string]any, basePath string, stack IncludeStack) (map[string]any, error) {
+	if len(stack) > maxIncludeDepth {
+		return nil, fmt.Errorf("include depth exceeds maximum of %d", maxIncludeDepth)
+	}
+
 	// Extract includes list
 	includesRaw, hasIncludes := configMap["includes"]
 	if !hasIncludes {
diff --git a/pkg/provider/README.md b/pkg/provider/README.md
@@ -75,4 +75,24 @@ type BaseWithChecks struct {
 }
 ```
 
-Embed this in your provider to get default implementations of `GetChecks()` and `SetChecks()`. In your provider's `SetChecks()` method, call `SetChecksAndCompile(exprs, celConfig)` to compile the CEL expressions against your provider's CEL configuration.
+Embed this in your provider to get default implementations of `GetChecks()`, `HasChecks()`, `EvaluateChecks()`, and `EvaluateChecksByMode()`. In your provider's `SetChecks()` method, call `SetChecksAndCompile(exprs, celConfig)` to compile the CEL expressions against your provider's CEL configuration.
+
+### Container
+
+Providers that group related child health checks implement the `Container` interface:
+
+```go
+type Container interface {
+    SetComponents(config map[string]any)
+    GetComponents() []Instance
+    ComponentErrors() []error
+}
+```
+
+Methods:
+
+- `SetComponents()`: Stores raw component configuration (called by factory before Setup)
+- `GetComponents()`: Returns resolved child instances (available after Setup)
+- `ComponentErrors()`: Returns validation errors from component resolution
+
+The `BaseContainer` struct provides a default implementation. Call `ResolveComponents()` from your provider's `Setup()` method to resolve child instances from stored config.
diff --git a/pkg/provider/container.go b/pkg/provider/container.go
@@ -61,5 +61,9 @@ func (b *BaseContainer) ResolveComponents() error {
 		b.resolved = append(b.resolved, instance)
 	}
 
-	return nil // errors collected in resolutionErrors, not returned
+	if len(b.resolved) == 0 && len(b.resolutionErrors) > 0 {
+		return errors.Join(b.resolutionErrors...)
+	}
+
+	return nil
 }
diff --git a/pkg/provider/factory.go b/pkg/provider/factory.go
@@ -95,7 +95,7 @@ func WithTimeout(timeout time.Duration) Option {
 // For configured instances, use NewInstance instead.
 func New(providerType string) (Instance, error) {
 	mu.RLock()
-	registeredType, ok := Providers[providerType]
+	registeredType, ok := providers[providerType]
 	mu.RUnlock()
 	if !ok {
 		return nil, fmt.Errorf("unknown provider type: %q", providerType)
@@ -110,7 +110,7 @@ func New(providerType string) (Instance, error) {
 // If no name is provided via WithName, defaults to the provider type.
 func NewInstance(providerType string, opts ...Option) (Instance, error) {
 	mu.RLock()
-	registeredType, ok := Providers[providerType]
+	registeredType, ok := providers[providerType]
 	mu.RUnlock()
 	if !ok {
 		return nil, fmt.Errorf("unknown provider type: %q", providerType)
diff --git a/pkg/provider/flags.go b/pkg/provider/flags.go
@@ -96,6 +96,7 @@ func (f *FlagValue) BuildFlag(flagSet *pflag.FlagSet, flagName string) {
 		flagSet.DurationP(flagName, f.Shorthand, defaultVal, f.Usage)
 	default:
 		slog.Warn("unrecognized flag kind", "flag", flagName, "kind", f.Kind)
+		return
 	}
 
 	if f.NoOptDefault != "" {
diff --git a/pkg/provider/http/http.go b/pkg/provider/http/http.go
@@ -135,6 +135,8 @@ func (c *Component) getCheckContext(ctx context.Context) (map[string]any, []*any
 	if response.TLS != nil {
 		if detail, err := anypb.New(tlsprovider.Detail(response.TLS)); err == nil {
 			details = []*anypb.Any{detail}
+		} else {
+			slog.Warn("failed to serialize TLS detail", "error", err)
 		}
 	}
 
@@ -167,9 +169,20 @@ func (c *Component) GetHealth(ctx context.Context) *ph.HealthCheckResponse {
 		return component.Unhealthy(err.Error())
 	}
 
-	// Apply CEL checks
-	if msgs := c.EvaluateChecks(ctx, checkCtx); len(msgs) > 0 {
-		return component.Unhealthy(msgs...)
+	// Apply CEL checks (or default status validation if no checks configured)
+	if c.HasChecks() {
+		if msgs := c.EvaluateChecks(ctx, checkCtx); len(msgs) > 0 {
+			return component.Unhealthy(msgs...)
+		}
+	} else {
+		// Default: validate successful HTTP status code
+		if responseData, ok := checkCtx["response"].(map[string]any); ok {
+			if status, ok := responseData["status"].(int); ok {
+				if status < 200 || status >= 400 {
+					return component.Unhealthy(fmt.Sprintf("HTTP %d", status))
+				}
+			}
+		}
 	}
 
 	// Add details if requested
diff --git a/pkg/provider/registry.go b/pkg/provider/registry.go
@@ -7,9 +7,9 @@ import (
 
 type ProviderRegistry map[string]reflect.Type
 
-// Providers is a map of provider names to their types, used for registering providers.
+// providers is the internal registry of provider names to their types.
 var (
-	Providers = ProviderRegistry{}
+	providers = ProviderRegistry{}
 	mu        sync.RWMutex
 )
 
@@ -18,17 +18,24 @@ func Register(name string, provider Instance) {
 	mu.Lock()
 	defer mu.Unlock()
 
-	Providers[name] = reflect.TypeOf(provider)
+	providers[name] = reflect.TypeOf(provider)
 }
 
 // List returns a list of registered providers.
 func ProviderList() []string {
 	mu.RLock()
 	defer mu.RUnlock()
 
-	providers := make([]string, 0, len(Providers))
-	for provider := range Providers {
-		providers = append(providers, provider)
+	list := make([]string, 0, len(providers))
+	for provider := range providers {
+		list = append(list, provider)
 	}
+	return list
+}
+
+// RegistryForTesting returns the provider registry for test assertions.
+func RegistryForTesting() ProviderRegistry {
+	mu.RLock()
+	defer mu.RUnlock()
 	return providers
 }
diff --git a/pkg/provider/registry_test.go b/pkg/provider/registry_test.go
@@ -23,7 +23,7 @@ func TestRegisterAuto(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			registeredProvider, ok := provider.Providers[tt.name]
+			registeredProvider, ok := provider.RegistryForTesting()[tt.name]
 			assert.True(t, ok, "Provider was not auto-registered")
 			assert.Equal(t, reflect.TypeOf(tt.provider), registeredProvider)
 		})
@@ -45,7 +45,7 @@ func TestRegisterManual(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			provider.Register(tt.name, tt.provider)
 
-			registeredProvider, ok := provider.Providers[tt.name]
+			registeredProvider, ok := provider.RegistryForTesting()[tt.name]
 			assert.True(t, ok, "Provider was not registered")
 			assert.Equal(t, reflect.TypeOf(tt.provider), registeredProvider)
 		})
diff --git a/pkg/provider/system/system_test.go b/pkg/provider/system/system_test.go
@@ -139,20 +139,11 @@ func TestSystemUnknownProviderReturnsError(t *testing.T) {
 		},
 	})
 
-	// Setup collects errors in ComponentErrors (doesn't return error)
+	// Setup returns an error when all children fail to resolve
 	err := s.Setup()
-	require.NoError(t, err)
-
-	// ComponentErrors should contain the error for unknown provider
-	require.Len(t, s.ComponentErrors(), 1)
-	assert.Contains(t, s.ComponentErrors()[0].Error(), "nonexistent")
-	assert.Contains(t, s.ComponentErrors()[0].Error(), "unknown provider type")
-
-	// GetHealth should still work but return HEALTHY with no children
-	result := s.GetHealth(t.Context())
-	assert.NotNil(t, result)
-	assert.Equal(t, ph.Status_HEALTHY, result.GetStatus())
-	assert.Equal(t, 0, len(result.GetComponents()))
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "nonexistent")
+	assert.Contains(t, err.Error(), "unknown provider type")
 }
 
 func TestSystemChildName(t *testing.T) {
diff --git a/pkg/provider/tls/tls.go b/pkg/provider/tls/tls.go
@@ -122,26 +122,30 @@ func (c *Component) getCheckContext(ctx context.Context) (map[string]any, []*any
 
 	state := tlsConn.ConnectionState()
 
+	if len(state.PeerCertificates) == 0 {
+		return nil, nil, fmt.Errorf("TLS handshake with %s:%d completed without peer certificates", c.Host, c.Port)
+	}
+
 	var details []*anypb.Any
 	if detail, err := anypb.New(Detail(&state)); err == nil {
 		details = []*anypb.Any{detail}
+	} else {
+		slog.Warn("failed to serialize TLS detail", "error", err)
 	}
 
 	// Determine if certificate chain would be verified by system CA pool
 	// (regardless of insecure setting)
 	verified := false
-	if len(state.PeerCertificates) > 0 {
-		opts := x509.VerifyOptions{
-			Roots:         certPool,
-			Intermediates: x509.NewCertPool(),
-			DNSName:       c.Host,
-		}
-		for _, cert := range state.PeerCertificates[1:] {
-			opts.Intermediates.AddCert(cert)
-		}
-		_, err := state.PeerCertificates[0].Verify(opts)
-		verified = (err == nil)
+	opts := x509.VerifyOptions{
+		Roots:         certPool,
+		Intermediates: x509.NewCertPool(),
+		DNSName:       c.Host,
+	}
+	for _, cert := range state.PeerCertificates[1:] {
+		opts.Intermediates.AddCert(cert)
 	}
+	_, verifyErr := state.PeerCertificates[0].Verify(opts)
+	verified = (verifyErr == nil)
 
 	// Build certificate chain
 	chain := make([]string, 0, len(state.PeerCertificates))
@@ -242,22 +246,27 @@ func ClassifyTLSError(err error) string {
 }
 
 // Detail builds a Detail_TLS from a TLS connection state.
+// If PeerCertificates is empty, only connection-level fields are populated.
 func Detail(state *tls.ConnectionState) (detail *details.Detail_TLS) {
 	detail = &details.Detail_TLS{
-		CommonName:         state.PeerCertificates[0].Subject.CommonName,
-		SubjectAltNames:    state.PeerCertificates[0].DNSNames,
-		ValidUntil:         timestamppb.New(state.PeerCertificates[0].NotAfter),
-		SignatureAlgorithm: state.PeerCertificates[0].SignatureAlgorithm.String(),
-		PublicKeyAlgorithm: state.PeerCertificates[0].PublicKeyAlgorithm.String(),
-		Version:            tls.VersionName(state.Version),
-		CipherSuite:        tls.CipherSuiteName(state.CipherSuite),
-		Protocol:           state.NegotiatedProtocol,
+		Version:     tls.VersionName(state.Version),
+		CipherSuite: tls.CipherSuiteName(state.CipherSuite),
+		Protocol:    state.NegotiatedProtocol,
 	}
-	chain := make([]string, 0, len(state.PeerCertificates))
-	for _, cert := range state.PeerCertificates {
-		chain = append(chain, cert.Issuer.CommonName)
+
+	if len(state.PeerCertificates) > 0 {
+		detail.CommonName = state.PeerCertificates[0].Subject.CommonName
+		detail.SubjectAltNames = state.PeerCertificates[0].DNSNames
+		detail.ValidUntil = timestamppb.New(state.PeerCertificates[0].NotAfter)
+		detail.SignatureAlgorithm = state.PeerCertificates[0].SignatureAlgorithm.String()
+		detail.PublicKeyAlgorithm = state.PeerCertificates[0].PublicKeyAlgorithm.String()
+
+		chain := make([]string, 0, len(state.PeerCertificates))
+		for _, cert := range state.PeerCertificates {
+			chain = append(chain, cert.Issuer.CommonName)
+		}
+		detail.Chain = chain
 	}
-	detail.Chain = chain
 
 	return detail
 }
diff --git a/pkg/provider/vault/README.md b/pkg/provider/vault/README.md
diff --git a/platform-health.example.yaml b/platform-health.example.yaml

Original file line number	Diff line number	Diff line change
`@@ -115,11 +115,8 @@ func setup(cmd *cobra.Command, _ []string) (err error) {`
`115`	`115`	`return err`
`116`	`116`	`}`
`117`	`117`
`118`		`- // In strict mode, fail if any configuration errors were found`
`119`		`- if strict {`
`120`		`- if err := result.EnforceStrict(log); err != nil {`
`121`		`- return err`
`122`		`- }`
	`118`	`+ if err := result.EnforceStrict(log); err != nil {`
	`119`	`+ return err`
`123`	`120`	`}`
`124`	`121`
`125`	`122`	`conf = result`
Original file line number	Diff line number	Diff line change
`@@ -59,11 +59,8 @@ func setup(cmd *cobra.Command, args []string) (err error) {`
`59`	`59`	`return err`
`60`	`60`	`}`
`61`	`61`
`62`		`- // In strict mode, fail if any configuration errors were found`
`63`		`- if strict {`
`64`		`- if err := result.EnforceStrict(log); err != nil {`
`65`		`- return err`
`66`		`- }`
	`62`	`+ if err := result.EnforceStrict(log); err != nil {`
	`63`	`+ return err`
`67`	`64`	`}`
`68`	`65`
`69`	`66`	`conf = result`
Original file line number	Diff line number	Diff line change
`@@ -61,5 +61,9 @@ func (b *BaseContainer) ResolveComponents() error {`
`61`	`61`	`b.resolved = append(b.resolved, instance)`
`62`	`62`	`}`
`63`	`63`
`64`		`- return nil // errors collected in resolutionErrors, not returned`
	`64`	`+ if len(b.resolved) == 0 && len(b.resolutionErrors) > 0 {`
	`65`	`+ return errors.Join(b.resolutionErrors...)`
	`66`	`+ }`
	`67`	`+`
	`68`	`+ return nil`
`65`	`69`	`}`