pkg: Stop installing function input CRDs and ignore disallowed kinds#6976
pkg: Stop installing function input CRDs and ignore disallowed kinds#6976phisco merged 3 commits intocrossplane:mainfrom
Conversation
The xpkg spec states that functions may include zero or more CRDs and no other resource types. The linter was not enforcing this, allowing functions to install other resource types such as XRDs and Compositions. Add the `IsCRD` validator to the function linter to prevent other types from being accepted in functions. Fixes crossplane#6975 Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
📝 WalkthroughWalkthroughAdds a FilteringEstablisher that filters package payload objects by allowed GroupKinds before delegating, and injects package Validators into the Reconciler. Reconciliation now validates packages (short-circuiting on failure and emitting a Validate event) before running linting; controllers are wired with type-specific validators. Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant Validator
participant Linter
participant Establisher
participant API_Server
Reconciler->>Validator: Lint(package)
alt validation fails
Validator-->>Reconciler: validation error
Reconciler->>Reconciler: wrap errValidatePackage, record event (ValidatePackage)
Note right of Reconciler: Stop reconcile (mark unhealthy)
else validation succeeds
Validator-->>Reconciler: valid
Reconciler->>Linter: Lint(package) (non-blocking)
Linter-->>Reconciler: lint findings (events)
Reconciler->>Establisher: Establish(filtered objects)
Establisher->>API_Server: apply allowed resources
API_Server-->>Establisher: refs / errors
Establisher-->>Reconciler: refs / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Thanks — do you want me to grep for other establisher usages to confirm all call sites were updated? Pre-merge checksImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/controller/pkg/revision/establisher.go(3 hunks)internal/controller/pkg/revision/establisher_test.go(1 hunks)internal/controller/pkg/revision/reconciler.go(10 hunks)internal/controller/pkg/revision/reconciler_test.go(23 hunks)internal/xpkg/lint.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/pkg/revision/establisher_test.gointernal/controller/pkg/revision/establisher.gointernal/controller/pkg/revision/reconciler_test.gointernal/controller/pkg/revision/reconciler.gointernal/xpkg/lint.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/controller/pkg/revision/establisher_test.gointernal/controller/pkg/revision/reconciler_test.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/pkg/revision/establisher_test.gointernal/controller/pkg/revision/establisher.gointernal/controller/pkg/revision/reconciler_test.gointernal/controller/pkg/revision/reconciler.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Review code for functional correctness, proper documentation and intent explanation, adherence to Coding Style guidelines, idiomatic alignment with existing Crossplane APIs, sufficient test coverage, DCO sign-off, and passing CI jobs
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Use crossplane-runtime/pkg/errors to wrap errors with context rather than using error constants
Applied to files:
internal/controller/pkg/revision/establisher.gointernal/controller/pkg/revision/reconciler_test.gointernal/controller/pkg/revision/reconciler.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Do not emit events for transient issues like apiserver conflict errors; instead, silently requeue the reconciliation
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Follow Crossplane's Observability Developer Guide
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Use cmpopts.EquateErrors to test that code returns expected errors; prefer cmpopts.AnyError for simple cases where you only care if an error was returned
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-11-06T08:30:43.529Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6906
File: internal/xcrd/compatibility_test.go:78-105
Timestamp: 2025-11-06T08:30:43.529Z
Learning: In Go tests using go-cmp, `cmpopts.AnyError` is a sentinel error value (not a cmp.Option) that can be used with `cmpopts.EquateErrors()` to match any non-nil error in test assertions. The pattern `cmp.Diff(tc.want.err, got, cmpopts.EquateErrors())` with `want.err = cmpopts.AnyError` is the correct way to assert that an error occurred without caring about the specific error value.
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use https://github.com/google/go-cmp/ for comparisons in tests to keep test output consistent
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
🧬 Code graph analysis (3)
internal/controller/pkg/revision/establisher.go (1)
internal/protection/usage/finder.go (1)
Object(107-111)
internal/controller/pkg/revision/reconciler_test.go (3)
internal/controller/pkg/revision/reconciler.go (4)
WithValidator(215-219)WithLinter(208-212)ReconcilerOption(126-126)WithRecorder(158-162)apis/pkg/v1/register.go (1)
ProviderRevisionGroupVersionKind(72-72)apis/pkg/v1/revision_types.go (1)
PackageRevisionActive(30-30)
internal/controller/pkg/revision/reconciler.go (2)
internal/controller/pkg/signature/reconciler.go (3)
WithValidator(99-103)ReconcilerOption(59-59)Reconciler(106-116)internal/controller/pkg/revision/establisher.go (2)
NewFilteringEstablisher(593-598)NewAPIEstablisher(98-104)
🔍 Remote MCP
Summary of Additional Context for PR Review
Function Package Specification Requirements
Function packages follow the Crossplane Function Package specification, which is distinct from Provider and Configuration specifications. The changes in this PR directly address requirements that were previously not enforced in Function packages.
Key Design Rationale
Validator vs. Linter Distinction:
The PR introduces a new Validator type that is more permissive than linters. Based on the xpkg pattern:
- Linters like
NewConfigurationLinter(),NewFunctionLinter(), andNewProviderLinter()are convenience functions for creating package linters for different package types - The new validators allow packages to pass through the installation phase (allowing supplementary controllers to handle unknown types) while linters still run and report issues as events
FilteringEstablisher's Role:
The IsCRD check validates that an object is a CustomResourceDefinition, and the FilteringEstablisher uses this to implement filtering by GroupKind. This enables Function packages to be filtered at installation time rather than failing the entire installation process.
Business Justification from Linked Issues
The PR addresses three interconnected problems:
- #6975: Function packages were allowed to contain non-CRD resources, violating the xpkg specification
- #5294: Function CRDs (used only for input schema documentation) were being installed into the cluster, causing management and compatibility issues
- #6525: Package installation was failing on unknown resource types, preventing forward/backward compatibility and third-party extension
The solution uses a validation-based approach where unknown objects are ignored during installation (allowing supplementary controllers to handle them later), while strict linting is still performed and errors are recorded as events for visibility.
🔇 Additional comments (12)
internal/controller/pkg/revision/establisher_test.go (1)
1234-1384: Well-structured tests for FilteringEstablisher — thank you for the thorough coverage!The test cases cover the key scenarios nicely: partial matching, full matching, no matches, empty filters, and error propagation from the wrapped establisher. The table-driven approach with PascalCase names follows the project conventions well.
One small clarifying question: In the
FilterNoMatchesandFilterEmptycases (lines 1318 and 1338), you're comparing against[]runtime.Object{}. Have you verified this works correctly withcmp.Diffwhen the actual result ismake([]runtime.Object, 0, len(objects))? Both should be empty slices, but I just want to confirm the comparison behaves as expected. If you've already validated this, great!internal/xpkg/lint.go (2)
57-85: Nice separation of concerns between Validator and Linter!I appreciate the thoughtful design here — the
Validatortype provides semantic clarity about its purpose (blocking vs. non-blocking checks), and the emptyObjectLinterFns()for validators aligns well with the PR objective of allowing installation to proceed even when unknown object types are present.Could you help me understand the rationale for including
PackageValidSemverin the validators? I'm assuming it's because invalid semver constraints could cause issues during dependency resolution, making it a "must-fix" rather than a "warning"?
109-116: Function linter now enforces CRD-only constraint — addresses issue #6975.Adding
IsCRDtoNewFunctionLinterensures Function packages are linted against the xpkg specification requirement that they may only contain CRDs. This is a welcome addition for spec compliance!internal/controller/pkg/revision/establisher.go (1)
585-616: Clean implementation of FilteringEstablisher — thanks for the clear design!The wrapper pattern here is elegant and maintains good separation of concerns. The use of
slices.Containsfor filtering is appropriate given the expected small size of thegksslice.I have a clarifying question about the
ReleaseObjectsmethod: it delegates directly without filtering. I assume this is intentional because we want to release all objects the parent previously owned (stored in status.objectRefs), regardless of the current filter configuration. Is that the correct understanding?internal/controller/pkg/revision/reconciler.go (3)
214-219: WithValidator option looks good!The option follows the same pattern as the existing
WithLinteroption, maintaining consistency across the codebase.
431-437: Empty FilteringEstablisher for Functions — great solution for issue #5294!The comment clearly explains the intent: per the xpkg spec, CRDs in function packages should not be installed. Using an empty filter elegantly achieves this without special-casing the establishment logic.
831-852: Validator/Linter flow nicely implements the PR objectives — thank you!The two-phase approach is clear:
- Validator runs first and blocks installation on failure (structural issues)
- Linter runs second but only records events (advisory warnings)
This allows packages with unknown object types to install while still surfacing lint issues to users. The TODO comment on line 850-851 about persisting lint errors in status is a good call-out for future work.
One small question: I notice
WithValidatoris always called in all threeSetup*functions (SetupProviderReconciler,SetupConfigurationReconciler,SetupFunctionReconciler), which guarantees the validator is initialized beforeReconcileis invoked. That said, sinceNewReconcilerdoesn't initialize a default validator like it does for the linter, would it be safer to add a nil check at line 833 as a defensive measure? That way, if someone instantiates a reconciler without going through the Setup functions, we avoid a potential panic. Just curious about the intent here.internal/controller/pkg/revision/reconciler_test.go (5)
67-90: MockEstablisher updates align with the new interface — looks good!The signature changes correctly reflect the updated
Establishmethod signature in the interface.
759-809: Test renamed from "ErrLint" to "ErrValidate" — reflects the new validation flow well!This test now correctly validates that validation failures (not just lint failures) block package installation, which aligns with the new validator/linter distinction.
1046-1115: Excellent test case for "SuccessfulWithLintErrors"!This is a key test that validates the PR's behavioral change: lint errors should record events but not block installation. The use of
newTestRecorderto assert the expected events is a nice addition.Thank you for including both the lint warning event and the success event in the expected output — this clearly demonstrates the intended behavior.
2031-2048: Simple and effective testRecorder implementation!The recorder captures events for assertion without over-engineering. The
WithAnnotationsmethod returningrmaintains the fluent interface pattern.One small question: Would it be helpful to also capture the
runtime.Objectpassed toEventfor more comprehensive assertions in future tests, or is the current approach sufficient for your needs?
2014-2019: Nice conditional event assertion!Only checking events when a
testRecorderis present keeps the existing tests working without modification while enabling event assertions where needed.
3f6e8c2 to
122a30a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/controller/pkg/revision/reconciler.go (1)
309-315: FilteringEstablisher setup for ProviderRevision looks correct.I see the typos from the previous review have been addressed — thank you for fixing those! The GroupKinds now correctly reference:
CustomResourceDefinitioninapiextensions.k8s.ioValidatingWebhookConfigurationandMutatingWebhookConfigurationinadmissionregistration.k8s.io
🧹 Nitpick comments (3)
internal/controller/pkg/revision/establisher_test.go (1)
1234-1384: Nice comprehensive test coverage for FilteringEstablisher! 👍The test cases cover the key scenarios well: partial matching, full matching, no matches, empty filter, and error propagation. Thank you for including the empty filter test case — it nicely validates the Function package behavior where no objects should be established.
One small suggestion for robustness: On line 1379, could we add
cmpopts.EquateEmpty()to the comparison? This would make the test resilient to differences betweenniland empty slices, which could vary based on implementation details:- if diff := cmp.Diff(tc.want.refs, refs); diff != "" { + if diff := cmp.Diff(tc.want.refs, refs, cmpopts.EquateEmpty()); diff != "" {This aligns with the pattern used in other tests in this file (e.g., line 826).
internal/controller/pkg/revision/reconciler.go (2)
462-482: Could we add a default validator for defensive safety?I noticed that
NewReconcilersets a default forlinter(line 470) but doesn't set one forvalidator. While all theSetup*functions correctly callWithValidator, if someone creates aReconcilerdirectly without it, line 835 (r.validator.Lint(pkg)) would panic.Would it make sense to add a default validator for consistency with the linter pattern? Something like:
r := &Reconciler{ client: mgr.GetClient(), cache: xpkg.NewNopCache(), revision: resource.NewAPIFinalizer(mgr.GetClient(), finalizer), objects: NewNopEstablisher(), parser: parser.New(nil, nil), linter: parser.NewPackageLinter(nil, nil, nil), + validator: parser.NewPackageLinter(nil, nil, nil), versioner: version.New(), // ... }This is just a suggestion for defensive coding — I understand the current production paths are all covered.
833-854: Validation and lint flow implementation looks correct.This nicely implements the intended behavior:
- Validation (lines 835-844): Failures block installation with
errValidatePackageand emit areasonValidateevent — appropriate for structural package issues.- Linting (lines 849-854): Failures only emit a warning event but allow installation to proceed — good for non-blocking concerns.
The TODO on lines 852-853 is a thoughtful callout. Since events are ephemeral, persisting lint warnings in status could help users diagnose issues after the fact. Would you like me to open an issue to track that enhancement?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/controller/pkg/revision/establisher.go(3 hunks)internal/controller/pkg/revision/establisher_test.go(1 hunks)internal/controller/pkg/revision/reconciler.go(10 hunks)internal/controller/pkg/revision/reconciler_test.go(23 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/pkg/revision/establisher.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/pkg/revision/establisher_test.gointernal/controller/pkg/revision/reconciler_test.gointernal/controller/pkg/revision/reconciler.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/controller/pkg/revision/establisher_test.gointernal/controller/pkg/revision/reconciler_test.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/pkg/revision/establisher_test.gointernal/controller/pkg/revision/reconciler_test.gointernal/controller/pkg/revision/reconciler.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Review code for functional correctness, proper documentation and intent explanation, adherence to Coding Style guidelines, idiomatic alignment with existing Crossplane APIs, sufficient test coverage, DCO sign-off, and passing CI jobs
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Do not emit events for transient issues like apiserver conflict errors; instead, silently requeue the reconciliation
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Use crossplane-runtime/pkg/errors to wrap errors with context rather than using error constants
Applied to files:
internal/controller/pkg/revision/reconciler_test.gointernal/controller/pkg/revision/reconciler.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Follow Crossplane's Observability Developer Guide
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Use cmpopts.EquateErrors to test that code returns expected errors; prefer cmpopts.AnyError for simple cases where you only care if an error was returned
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-11-06T08:30:43.529Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6906
File: internal/xcrd/compatibility_test.go:78-105
Timestamp: 2025-11-06T08:30:43.529Z
Learning: In Go tests using go-cmp, `cmpopts.AnyError` is a sentinel error value (not a cmp.Option) that can be used with `cmpopts.EquateErrors()` to match any non-nil error in test assertions. The pattern `cmp.Diff(tc.want.err, got, cmpopts.EquateErrors())` with `want.err = cmpopts.AnyError` is the correct way to assert that an error occurred without caring about the specific error value.
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use https://github.com/google/go-cmp/ for comparisons in tests to keep test output consistent
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-09-09T21:39:13.640Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6777
File: internal/circuit/mapfunc.go:37-41
Timestamp: 2025-09-09T21:39:13.640Z
Learning: In Kubernetes/controller-runtime, real objects that come through watches and reconciliation always have their GVK populated by the API server and client. obj.GetObjectKind().GroupVersionKind() will never be empty for actual Kubernetes objects in production code paths.
Applied to files:
internal/controller/pkg/revision/reconciler.go
🧬 Code graph analysis (1)
internal/controller/pkg/revision/reconciler.go (5)
internal/controller/pkg/signature/reconciler.go (3)
WithValidator(99-103)ReconcilerOption(59-59)Reconciler(106-116)internal/xpkg/lint.go (3)
Validator(58-58)NewProviderValidator(62-67)NewConfigurationValidator(71-76)internal/controller/pkg/revision/establisher.go (1)
NewFilteringEstablisher(593-598)internal/controller/pkg/revision/dependency.go (1)
NewPackageDependencyManager(71-78)apis/apiextensions/v1/register.go (1)
CompositionGroupVersionKind(56-56)
🔇 Additional comments (7)
internal/controller/pkg/revision/reconciler.go (2)
363-371: Configuration package establishment filter looks appropriate.The allowed GroupKinds cover the expected resource types for Configuration packages: XRDs, Compositions, ManagedResourceActivationPolicy, and the various Operation types. This aligns well with the package specification.
433-439: Good documentation of the Function package filtering rationale.The inline comment clearly explains why an empty filter is used — per the xpkg spec, CRDs in Function packages are for schema/documentation purposes and should not be installed. This directly addresses issue #5294. Thank you for the clear explanation!
internal/controller/pkg/revision/reconciler_test.go (5)
67-90: MockEstablisher signature update is consistent and correct.The updated signature properly matches the
Establisherinterface with thecontext.Context,[]runtime.Object,v1.PackageRevision, andboolparameters. The implementation on lines 88-90 correctly delegates to the mock function.
759-809: ErrValidate test case correctly validates the new validation path.This test properly verifies that:
- When the validator fails, the reconciler returns
errValidatePackage- The status is marked unhealthy with the appropriate message
- The linter is not invoked when validation fails (since it's configured to succeed)
The separation of
WithValidator(failing) andWithLinter(succeeding) makes the test intent clear.
1046-1115: Excellent test for the new lint-errors-don't-block-installation behavior! 🎉This test case is crucial for validating the PR's objective that packages can be installed even when linting fails. The key aspects are well-tested:
- Validator passes, linter fails → reconciliation still succeeds
- Status shows
RevisionHealthy()despite lint errors- Events are verified: both the lint warning and successful sync are recorded
The use of
testRecorder(lines 1106-1109) to verify exact event output is a nice pattern. Thank you for adding this coverage!
2031-2049: Clean testRecorder implementation for event verification.This is a nicely minimal mock that does exactly what's needed. The
Want/Gotpattern makes the assertion on lines 2015-2018 straightforward to understand. TheWithAnnotationsimplementation correctly returns the same recorder to maintain state.
2014-2020: Event verification integration is well-designed.The conditional check for
testRecorder(line 2015) keeps the test harness backward-compatible — tests that don't need event verification continue to work unchanged, while tests likeSuccessfulWithLintErrorscan opt into event assertions. This is a thoughtful approach that avoids breaking existing test patterns.
|
If anyone wants to give this a test drive, I've put together a (publicly pullable) function package that demonstrates the changes at |
negz
left a comment
There was a problem hiding this comment.
Nice, this looks great. I don't feel strongly about the filename suggestion I made.
internal/xpkg/lint.go
Outdated
| // Validator validates packages before installation is attempted. | ||
| type Validator parser.Linter |
There was a problem hiding this comment.
Should these live in a new validate.go - not lint.go?
Crossplane uses package linters in two places: at build time (in the CLI) and at install time (in the revision controller). In both cases, linter errors are treated as errors and stop the operation. At build time, this makes sense: we should be strict about the packages we produce. At install time, this means we may reject packages that we could successfully install. For example (as described in crossplane#6525) if we allow new resource types in a package in a new version of Crossplane (as we've done with Operations and MRDs), older versions should still be able to install packages that contain them; the new resources would just be ignored. If package authors want to limit the versions their package works with, they can set a version constraint in the metadata. Introduce a new Validator type, which is identical to a linter except in name. Add validators for all package types, which are identical to their linters except that they allow any object to be included. In the revision controller, use validators to determine whether package installation should be attempted, where we previously used linters. Run the linters as well and record any lint errors as events, to help the user diagnose any unexpected behavior. Fixes crossplane#6525 Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
The xpkg spec says that CRDs included in Function packages will not be installed into the cluster, but until now they have been since Functions are treated the same as all other packages. Additionally, the previous commit allowed packages with arbitrary objects included to be installed, contrary to the spec (and desired behavior). Filter objects in the establisher, so that regardless of what kinds of resources are present in a package we install only the desired ones. Fixes crossplane#5294 Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
122a30a to
36f59be
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/xpkg/validate.go (1)
21-49: Clear separation between validators and linters — could you add a brief doc comment explaining the distinction?I appreciate the clean design here. Looking at the implementation:
- Validators check only metadata (OneMeta, Is{Type}, PackageValidSemver) with an empty
ObjectLinterFns()for objects.- Linters (in
lint.go) check metadata plus validate object kinds against allowed types.This separation enables the new behavior where validation failures block installation, but lint failures only emit warnings.
Could you consider adding a brief doc comment on the
Validatortype (around line 21-22) explaining this distinction? Something like:// Validator validates packages before installation is attempted. Unlike // linters, validators only check package metadata (meta type, semver // constraints) and do not validate included objects. Validation failures // block installation, whereas lint failures only emit warnings. type Validator = parser.LinterThis would help future contributors understand why validators and linters exist as separate concepts despite having the same underlying type.
internal/controller/pkg/revision/reconciler.go (1)
833-854: Validation and linting flow correctly implements the new behavior.The two-phase approach is well-implemented:
Validation (lines 835-844): Uses the package-specific validator. Failures block installation with
errValidatePackageand emit areasonValidateevent.Linting (lines 849-854): Runs after successful validation. Failures emit a warning event but do not block installation.
This aligns perfectly with the PR objectives from issue #6525.
Regarding the TODO at lines 852-853: I agree that persisting lint errors in status would be valuable since events are ephemeral. This could be a follow-up enhancement. Would you like me to open an issue to track this, or do you have plans to address it in this PR?
internal/controller/pkg/revision/reconciler_test.go (1)
1046-1115: New test case validates the key behavioral change — lint errors don't block installation.This test (
SuccessfulWithLintErrors) is crucial for verifying the PR's core objective. It:
- Configures a passing validator and failing linter
- Expects successful reconciliation (
err: nil)- Uses
newTestRecorderto verify that:
- A warning event is emitted for the lint failure
- A success event is emitted for the sync
This test gives confidence that packages with lint warnings will install successfully while still informing users of potential issues.
One minor suggestion: the test name
SuccessfulWithLintErrorsis clear, but thereasonfield at line 1047 could be slightly more descriptive. Perhaps:"We should record a warning event for lint errors but successfully install the package."? This would make the test output more informative if it fails.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/controller/pkg/revision/establisher.go(3 hunks)internal/controller/pkg/revision/establisher_test.go(1 hunks)internal/controller/pkg/revision/reconciler.go(10 hunks)internal/controller/pkg/revision/reconciler_test.go(23 hunks)internal/xpkg/validate.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/pkg/revision/establisher_test.gointernal/xpkg/validate.gointernal/controller/pkg/revision/reconciler.gointernal/controller/pkg/revision/establisher.gointernal/controller/pkg/revision/reconciler_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/controller/pkg/revision/establisher_test.gointernal/controller/pkg/revision/reconciler_test.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/pkg/revision/establisher_test.gointernal/controller/pkg/revision/reconciler.gointernal/controller/pkg/revision/establisher.gointernal/controller/pkg/revision/reconciler_test.go
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Review code for functional correctness, proper documentation and intent explanation, adherence to Coding Style guidelines, idiomatic alignment with existing Crossplane APIs, sufficient test coverage, DCO sign-off, and passing CI jobs
📚 Learning: 2025-09-09T21:39:13.640Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6777
File: internal/circuit/mapfunc.go:37-41
Timestamp: 2025-09-09T21:39:13.640Z
Learning: In Kubernetes/controller-runtime, real objects that come through watches and reconciliation always have their GVK populated by the API server and client. obj.GetObjectKind().GroupVersionKind() will never be empty for actual Kubernetes objects in production code paths.
Applied to files:
internal/controller/pkg/revision/reconciler.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Review code for functional correctness, proper documentation and intent explanation, adherence to Coding Style guidelines, idiomatic alignment with existing Crossplane APIs, sufficient test coverage, DCO sign-off, and passing CI jobs
Applied to files:
internal/controller/pkg/revision/reconciler.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Use crossplane-runtime/pkg/errors to wrap errors with context rather than using error constants
Applied to files:
internal/controller/pkg/revision/reconciler.gointernal/controller/pkg/revision/establisher.gointernal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Do not emit events for transient issues like apiserver conflict errors; instead, silently requeue the reconciliation
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Follow Crossplane's Observability Developer Guide
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Use cmpopts.EquateErrors to test that code returns expected errors; prefer cmpopts.AnyError for simple cases where you only care if an error was returned
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-11-06T08:30:43.529Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6906
File: internal/xcrd/compatibility_test.go:78-105
Timestamp: 2025-11-06T08:30:43.529Z
Learning: In Go tests using go-cmp, `cmpopts.AnyError` is a sentinel error value (not a cmp.Option) that can be used with `cmpopts.EquateErrors()` to match any non-nil error in test assertions. The pattern `cmp.Diff(tc.want.err, got, cmpopts.EquateErrors())` with `want.err = cmpopts.AnyError` is the correct way to assert that an error occurred without caring about the specific error value.
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use https://github.com/google/go-cmp/ for comparisons in tests to keep test output consistent
Applied to files:
internal/controller/pkg/revision/reconciler_test.go
🧬 Code graph analysis (5)
internal/controller/pkg/revision/establisher_test.go (2)
internal/controller/pkg/revision/establisher.go (2)
Establisher(66-69)NewFilteringEstablisher(593-598)internal/controller/pkg/revision/reconciler_test.go (1)
MockEstablisher(66-69)
internal/xpkg/validate.go (1)
internal/xpkg/lint.go (5)
OneMeta(89-95)IsProvider(98-105)PackageValidSemver(154-169)IsConfiguration(108-115)IsFunction(118-125)
internal/controller/pkg/revision/reconciler.go (7)
internal/xpkg/validate.go (4)
Validator(22-22)NewProviderValidator(26-31)NewConfigurationValidator(35-40)NewFunctionValidator(44-49)internal/controller/pkg/revision/establisher.go (2)
NewFilteringEstablisher(593-598)NewAPIEstablisher(98-104)apis/apiextensions/v1alpha1/register.go (2)
ManagedResourceDefinitionGroupVersionKind(56-56)ManagedResourceActivationPolicyGroupVersionKind(61-61)apis/pkg/v1/register.go (5)
Group(28-28)SchemeGroupVersion(34-34)ProviderGroupVersionKind(64-64)ConfigurationGroupVersionKind(48-48)FunctionGroupVersionKind(80-80)internal/xpkg/lint.go (3)
NewProviderLinter(59-68)NewConfigurationLinter(72-77)NewFunctionLinter(81-86)apis/apiextensions/v1/register.go (1)
CompositionGroupVersionKind(56-56)apis/ops/v1alpha1/register.go (3)
OperationGroupVersionKind(45-45)CronOperationGroupVersionKind(53-53)WatchOperationGroupVersionKind(61-61)
internal/controller/pkg/revision/establisher.go (3)
internal/protection/usage/finder.go (1)
Object(107-111)apis/pkg/v1/interfaces.go (1)
PackageRevision(532-577)internal/controller/pkg/revision/imageback.go (1)
PackageRevision(204-213)
internal/controller/pkg/revision/reconciler_test.go (2)
internal/controller/pkg/revision/reconciler.go (8)
WithValidator(217-221)WithLinter(210-214)ReconcilerOption(128-128)WithParser(188-192)WithEstablisher(181-185)WithCache(139-143)WithConfigStore(203-207)WithRecorder(160-164)internal/controller/pkg/runtime/reconciler.go (3)
ReconcilerOption(77-77)WithConfigStore(141-145)WithRecorder(94-98)
🔍 Remote MCP
The Context7 tools are unavailable due to authentication issues. Let me try searching the web for relevant Crossplane documentation and specifications.## Summary of Additional Context for PR #6976 Review
Based on the linked issues and package manager architecture, here are the key facts relevant to this PR:
Key Design Rationale
Functions were never supposed to use actual CRs, and the package manager was never supposed to install their CRDs. A Function package may optionally define a schema for its input using a CRD to capture the input schema, since it's a KRM-like object, mostly useful for documentation purposes.
Most Go functions that want to take an input define an input Go type the same way they'd define a CRD - using controller-tools. They then deserialize the input from the RunFunctionRequest into the Go type. It's this use of controller-tools that lead to accidentally creating a CRD.
Impact of This PR's Changes
Backward compatibility consideration: The package manager should ignore unknown or disallowed resource kinds instead of failing installation, allowing Crossplane instances to install packages containing types they do not recognize so newer package payloads won't block older Crossplane versions.
Package structure specification: The package.yaml file MUST contain a valid YAML stream. All other content in either the xpkg base layer, or the full image filesystem is ignored by Crossplane.
Parser and Linter Architecture
The Linter executes all linter functions against a package. PackageLinterFn lints an entire package, and if the function applies a check for multiple objects, consider using an ObjectLinterFn. The PR introduces Validator as a type alias to parser.Linter, enabling permissive validation that doesn't block installation on unknown object types.
Review Focus Points
- FilteringEstablisher wrapper - Correctly filters objects by GroupKind before delegation to the wrapped Establisher
- Validator vs Linter distinction - Validators permit unknown kinds while linters enforce strict requirements; linting failures now emit events but don't block installation
- Package-type-specific validators - Each package type (Provider, Configuration, Function) gets its own validator configured with appropriate allowed resource kinds
- Event recording - Validation and lint issues are now recorded as Kubernetes events for observability while allowing installation to proceed
🔇 Additional comments (10)
internal/controller/pkg/revision/establisher_test.go (1)
1234-1384: Thorough test coverage for FilteringEstablisher — nicely done!Thank you for adding comprehensive tests covering the key scenarios. The table-driven structure with clear reasons makes these tests easy to understand and maintain. A few observations:
The test validates filtered objects inside the mock callback using
cmp.Diff, which is a solid approach for verifying the filtering logic.I noticed that in the
FilterNoMatchesandFilterEmptycases (lines 1318 and 1338), you're comparing against[]runtime.Object{}— could you help me understand why an empty slice rather thannilis expected here? Looking at the implementation inestablisher.go, thefilteredslice is initialized withmake([]runtime.Object, 0, len(objects)), so it would indeed be an empty slice, not nil. Just wanted to confirm this is intentional.One edge case that might be worth considering: what happens if an object in the input slice has an empty/unset GVK? The current implementation would not match it against any filter. This might be fine in practice (since parsed objects should have GVK set), but a test documenting this behavior could be helpful for future maintainers. Would you like to add one?
internal/controller/pkg/revision/establisher.go (2)
600-611: Clean decorator implementation for filtering — LGTM!The filtering logic is straightforward and correct. I appreciate that:
- Pre-allocating the
filteredslice withcap=len(objects)avoids reallocation in the common case.- Using
slices.Containskeeps the code readable.- The method delegates to the wrapped establisher after filtering, preserving the original interface contract.
One small thought: if a package contains many objects and only a small subset match the filter, we're allocating more capacity than needed. This is a micro-optimization and probably not worth changing given typical package sizes, but I wanted to mention it in case you had considered it.
613-616: ReleaseObjects delegation is correct.Thank you for not filtering in
ReleaseObjects. The release operation should work on all objects previously established (tracked instatus.objectRefs), regardless of the current filter configuration. This ensures proper cleanup even if the filter changes between install and uninstall.internal/controller/pkg/revision/reconciler.go (3)
309-316: Provider FilteringEstablisher configuration looks correct.Thank you for fixing the typos from the previous review! The GroupKind definitions now properly use the scheme constants:
k8sextv1.SchemeGroupVersion.Groupfor CRDadmv1.SchemeGroupVersion.Groupfor webhook configurationsOne question: I see
ManagedResourceDefinitionis included here viaextv1alpha1.ManagedResourceDefinitionGroupVersionKind.GroupKind(). Is this currently feature-gated (I notice there's aEnableBetaCustomToManagedResourceConversionflag used later in the reconcile loop)? Would it be worth adding a comment noting that MRDs are established only when the conversion actually happens, or is the filter inclusion harmless when the feature is disabled?
363-372: Configuration FilteringEstablisher includes all expected resource types.This covers the full set of configuration package objects:
CompositeResourceDefinition(XRD)CompositionManagedResourceActivationPolicyOperation,CronOperation,WatchOperationI verified these match what's expected in
NewConfigurationLinter()fromlint.go(which checks forIsXRD, IsActivationPolicy, IsComposition, IsOperation, IsCronOperation, IsWatchOperation).
433-439: Excellent documentation for the empty Function filter!The comment at lines 433-436 clearly explains why no objects are installed from function packages, which aligns with the xpkg spec and directly addresses issue #5294. This is exactly the kind of context that helps future maintainers understand the intent.
internal/controller/pkg/revision/reconciler_test.go (4)
67-90: MockEstablisher signature update correctly matches the interface.The updated signature
func(context.Context, []runtime.Object, v1.PackageRevision, bool) ([]xpv1.TypedReference, error)properly reflects theEstablishmethod parameters, enabling tests to verify the objects passed to the establisher when needed.
759-808: Test correctly renamed from ErrLint to ErrValidate with updated semantics.The test now:
- Configures a failing validator (
WithValidator(&MockLinter{MockLint: NewMockLintFn(errBoom)}))- Configures a passing linter (
WithLinter(&MockLinter{MockLint: NewMockLintFn(nil)}))- Expects
errValidatePackageas the wrapped error- Expects the status message to say "validating package contents failed"
This accurately reflects the new behavior where validation failures (not lint failures) block installation.
2031-2049: Simple and effective testRecorder implementation.The
testRecordercleanly captures events for assertion without additional complexity. I appreciate that:
- It stores both
WantandGotfor direct comparison withcmp.Diff- The
WithAnnotationsmethod returnsrto maintain chainability- The factory function
newTestRecordermakes test setup cleanOne thought: the
Eventmethod discards theruntime.Objectparameter. If you ever need to assert which object the event was recorded for, you'd need to extend this. But for current needs, this is perfectly adequate.
2014-2019: Event assertion integration is cleanly implemented.The type assertion
if tr, ok := r.record.(*testRecorder); okensures event assertions only run when a test explicitly opts in viaWithRecorder(newTestRecorder(...)). This keeps existing tests working without modification while enabling event verification where needed.
Description of your changes
This PR fixes three related issues in the package manager's handling of objects:
Enforce that functions can install only CRDs
The xpkg spec states that functions may include zero or more CRDs and no other resource types. The linter was not enforcing this, allowing functions to install other resource types such as XRDs and Compositions.
Add the
IsCRDvalidator to the function linter to prevent other types from being accepted in functions.Fixes #6975
Stop installing CRDs from Function packages
The xpkg spec says that CRDs included in Function packages will not be installed into the cluster, but until now they have been since Functions are treated the same as all other packages. Additionally, the previous commit allowed packages with arbitrary objects included to be installed, contrary to the spec (and desired behavior).
Filter objects in the establisher, so that regardless of what kinds of resources are present in a package we install only the desired ones.
Fixes #5294
Don't fail to install a package if it contains disallowed/unknown objects
Crossplane uses package linters in two places: at build time (in the CLI) and at install time (in the revision controller). In both cases, linter errors are treated as errors and stop the operation.
At build time, this makes sense: we should be strict about the packages we produce. At install time, this means we may reject packages that we could successfully install. For example (as described in #6525) if we allow new resource types in a package in a new version of Crossplane (as we've done with Operations and MRDs), older versions should still be able to install packages that contain them; the new resources would just be ignored. If package authors want to limit the versions their package works with, they can set a version constraint in the metadata.
Introduce a new Validator type, which is identical to a linter except in name. Add validators for all package types, which are identical to their linters except that they allow any object to be included.
In the revision controller, use validators to determine whether package installation should be attempted, where we previously used linters. Run the linters as well and record any lint errors as events, to help the user diagnose any unexpected behavior.
Fixes #6525
I have:
earthly +reviewableto ensure this PR is ready for review.- [ ] Added or updated e2e tests.- [ ] Linked a PR or a docs tracking issue to document this change.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.- [ ] Followed the API promotion workflow if this PR introduces, removes, or promotes an API.