Add new configuration APIs/mechanisms for starting to introduce new MR API version policy #585
Conversation
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
sergenyalcin
changed the title
ulucinar
left a comment
ulucinar
left a comment
There was a problem hiding this comment.
Thank you @sergenyalcin, leaving some comments for us to discuss.
pkg/config/resource.go
Outdated
| DeprecatedInRelease string | ||
|
|
||
| // RemovalPlannedRelease indicates the provider release in which this version | ||
| // is planned to be removed. According to the versioning policy, a version must | ||
| // remain deprecated for at least one release before removal. | ||
| // Format: "v1.3.0" | ||
| RemovalPlannedRelease string |
There was a problem hiding this comment.
We are coupling the specific release versions with the resource configurations I believe for the first time, optionally. I think this should be fine because:
- It's optional. Provider authors can opt out if they don't want such coupling.
- We are planning to drive the deprecation policy enforcement as autonomously as possible, i.e., there will be automatic configurators which utilize the information from
VersionDeprecationto drive the policy as we make new releases. - If the provider code lives in multiple repositories (has forks), most probably they will be following the same versioning policy and have the same versions. But if there are forks, this may also not be true and if the provider author does not want to maintain this configuration (even via some sort of automatic configuration), she can still opt out using this optional configuration.
So, I think this is perfectly fine. But because we are introducing a coupling with the resource configurations and the release versions of the providers, I believe it's worth surfacing this.
We could hold this information outside of the resource configuration and specific for a provider's implementation. The current approach makes it universally available for all upjet-based providers rather than being part of a repo specific API versioning policy driver. So I believe it's worth the coupling.
There was a problem hiding this comment.
Release notes is a good place to discuss these. At a minimum, we had better talk about the new APIs and link to a public discussion of MR API version lifecycle, and what happens if the provider authors opt out of lifecycle management via the new APIs. It's also probably important to underlines these are relevant if their MRs have multiple versions (most providers do not care for breaking APIs and overwrite their existing API versions instead).
| // - If ServedVersions is explicitly set, it's used as-is | ||
| // - If ServedVersions is empty/nil, all versions are served by default | ||
| // - The returned slice is a new slice to prevent external modifications | ||
| func (r *Resource) GetServedVersions() []string { |
There was a problem hiding this comment.
The default implementations looks good for backward compatibility so that if a provider author does not start using the new APIs, we assume all previously configured API versions via the existing Version and PreviousVersions APIs are considered as being served.
One case we need to be careful for backward compatibility is what would happen if a provider author had multiple versions but did not bother to set the PreviousVersions? Is this possible? How do we use PreviousVersions before these changes? Can you check how we previously make use of the PreviousVersions API and leave a comment here for documentation purposes?
If some scenario like this is actually possible, i.e., the PreviousVersions API was optional even with multi-version CRDs, then we may also consider not using ServedVersions if it's not explicitly configured or making this visible in the release notes.
There was a problem hiding this comment.
About PreviousVersions usage, I added comments to a few places. Also, I will add some statements to the release notes.
In the current situation, the PreviousVersions API is optional and managed by providers. However, if the provider wants to work everything in place (I mean, without the new configuration), they must be set. Because in the current code generation pipeline, we rely on this field being filled.
So at the API level, we don't enforce this, even if the provider has multiversion CRDs. However, in the current code-generation pipeline, we assume this is properly configured.
There was a problem hiding this comment.
So at the API level, we don't enforce this, even if the provider has multiversion CRDs. However, in the current code-generation pipeline, we assume this is properly configured.
What I understand from this statement that in the current shape of the code generation pipeline (without the changes from this PR), if a provider has multiple versions for a CRD, the corresponding PreviousVersions must already be configured properly, otherwise the generation already produces erroneous code, right (as its assumptions are violated)?
…em, unexport post generation hook mechanism, in hook implementation remove ast usage and just do with string manipulation, break hook implementations to their own files, apply some nit comments Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
📝 WalkthroughWalkthroughThis PR adds API version deprecation and served versions support to the Kubernetes API code generator. It introduces configuration types and methods for managing deprecation metadata, implements post-generation hooks to update kubebuilder markers in generated files, and extends CRD templates to conditionally include deprecation and unserved markers. Changes
Sequence DiagramsequenceDiagram
participant Config as Provider Config
participant Resource as Resource API
participant CRDGen as CRD Generator
participant PostHook as Post-Gen Hooks
participant FileSystem as File System
Config->>Resource: SetServedVersions(versions)
Config->>Resource: SetDeprecatedVersion(version, metadata)
Resource->>Resource: Validate & Store
CRDGen->>Resource: GetServedVersions()
CRDGen->>Resource: IsVersionDeprecated(version)
Resource-->>CRDGen: Served list & deprecation metadata
CRDGen->>CRDGen: Build CRD with markers
CRDGen-->>PostHook: Generated files
PostHook->>PostHook: lifecycleMarkerUpdateHook
PostHook->>FileSystem: Read type files
FileSystem-->>PostHook: File content
PostHook->>PostHook: Update unservedversion markers
PostHook->>PostHook: Update deprecatedversion markers
PostHook->>FileSystem: Write updated files
PostHook->>PostHook: storageVersionMarkerUpdateHook
PostHook->>FileSystem: Read previous version files
FileSystem-->>PostHook: File content
PostHook->>PostHook: Add/remove storageversion markers
PostHook->>FileSystem: Write updated files
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/config/resource.go`:
- Around line 1021-1028: The code currently uses lexicographic string comparison
to validate that deprecation.PlannedRemovalRelease is after
deprecation.DeprecationRelease, which fails for multi-digit semver components;
replace the string comparison with a proper semantic version comparison using
golang.org/x/mod/semver (import it) and call
semver.Compare(deprecation.PlannedRemovalRelease,
deprecation.DeprecationRelease) to ensure the planned removal is strictly
greater; keep the existing error message when the comparison indicates the
planned removal is not after the deprecation.
In `@pkg/pipeline/hooks_deprecated_version.go`:
- Around line 251-281: The current findDescriptionBlock function uses a fixed
look-back limit (typeLineIndex-10) which can skip doc lines when many
kubebuilder markers exist; remove the hard cap and iterate i from
typeLineIndex-1 down to 0, stopping only when you hit the first
non-comment/non-kubebuilder line (but still treating "//" as description lines
and ignoring "// +kubebuilder:" markers), and update
descriptionStartIndex/descriptionEndIndex accordingly; apply the identical
change to the analogous function around lines 337-349 so both scanning routines
no longer use the 10/20-line limits.
🧹 Nitpick comments (4)
pkg/pipeline/crd.go (1)
109-119: Consider simplifying the served version check withslices.Contains.The manual loop to check if the current version is served could be simplified using Go's
slices.Contains(available since Go 1.21). This improves readability and reduces boilerplate.♻️ Suggested refactor
+import "slices" + // Check if this version should be marked as unserved // With SetServedVersions() validation, the current version MUST be included // if served versions are explicitly configured. This check is defensive but should // not trigger under normal circumstances with validated configuration. servedVersions := cfg.GetServedVersions() -isServed := false -for _, v := range servedVersions { - if v == cfg.Version { - isServed = true - break - } -} -if !isServed { +if !slices.Contains(servedVersions, cfg.Version) { crdMap["MarkUnserved"] = "true" }pkg/pipeline/hooks_test.go (1)
245-269: Consider adding test for warning messages with special characters.The test cases use clean warning strings. Consider adding a test case with a warning containing quotes or other special characters to ensure proper escaping in the generated marker.
Example test case to add:
"DeprecatedWithSpecialChars": { reason: "Should handle warning with special characters.", args: args{ content: baseContent, typeName: "Member", version: "v1beta1", isServed: true, isDeprecated: true, deprecation: config.VersionDeprecation{ Warning: `Use "v1beta2" instead`, }, }, want: want{ // Verify escaping behavior }, },pkg/pipeline/hooks_deprecated_version.go (1)
7-15: Usegithub.com/pkg/errorsfor wrapping.The import uses
github.com/crossplane/crossplane-runtime/v2/pkg/errors, but the guidelines requiregithub.com/pkg/errorsfor wrapping/context. Please switch to align with the repo standard.🔧 Suggested import change
- "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + "github.com/pkg/errors"As per coding guidelines, please use
github.com/pkg/errors.pkg/pipeline/hooks_storage_version.go (1)
7-16: Usegithub.com/pkg/errorsfor wrapping.This file also imports
github.com/crossplane/crossplane-runtime/v2/pkg/errors. Guidelines requiregithub.com/pkg/errorsfor wrapping/context.🔧 Suggested import change
- "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + "github.com/pkg/errors"As per coding guidelines, please use
github.com/pkg/errors.
| // Validate release ordering if both are provided | ||
| if deprecation.DeprecationRelease != "" && deprecation.PlannedRemovalRelease != "" { | ||
| // Simple string comparison works for semver format (v1.5.0 < v1.7.0) | ||
| if deprecation.PlannedRemovalRelease <= deprecation.DeprecationRelease { | ||
| return errors.Errorf("PlannedRemovalRelease %q must be after DeprecationRelease %q", | ||
| deprecation.PlannedRemovalRelease, deprecation.DeprecationRelease) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the specific lines mentioned
sed -n '1015,1035p' pkg/config/resource.goRepository: crossplane/upjet
Length of output: 857
🏁 Script executed:
# Check what version patterns are actually used in the codebase
rg -i 'v[0-9]+\.[0-9]+\.[0-9]+' --max-count=20Repository: crossplane/upjet
Length of output: 5749
🏁 Script executed:
# Search for any existing semver libraries imported
rg 'import.*semver|Masterminds|coreos/go-semver' pkg/Repository: crossplane/upjet
Length of output: 42
🌐 Web query:
Go semver version comparison library recommended best practices 2024 2025
💡 Result:
Short summary — recommended Go semver libs (2024–2025) and best practices
Recommended libraries
- golang.org/x/mod/semver — official x/mod package for simple/accurate comparisons (requires leading "v", returns canonical form). [1]
- github.com/Masterminds/semver — feature-rich (parsing, ranges/constraints, v3+ for Go modules). [2]
- github.com/blang/semver (v4) — fast, spec‑compliant, zero‑stdlib approach (value types). [3]
- github.com/gomodules/semver — modern, constraint and sort support, optional "v" handling. [4]
- github.com/aquasecurity/go-version — parses semver-like versions, supports extra components and constraints (useful if you need non‑standard variants). [5]
Practical best practices
- Prefer a well‑maintained library rather than writing custom parsing/comparison. Use golang.org/x/mod/semver for straightforward precedence checks; use Masterminds or blang/gomodules when you need constraints, sorting, or richer parsing. [1][2][3][4]
- Always use Go modules (go.mod) and pin exact versions (avoid
@latest/main) for reproducible builds; vendor if you require hermetic CI. Use go mod tidy and run dependency audits. [6] - Normalize/canonicalize versions before comparing (use library canonical functions) and be explicit about accepting or requiring a leading "v". [1][2]
- Handle pre-release and build metadata according to SemVer 2.0.0 rules (pre-release < release; build metadata ignored for precedence). Use libraries that implement the spec rather than ad hoc rules. [1][2][3]
- Use semantic‑aware constraint checking (e.g., range expressions) from a library when deciding allowed upgrades; rely on MVS behavior but assert constraints in CI. [2][4]
- Keep dependencies updated and monitored (Dependabot or CI checks), add tests for version logic, and document version expectations in your project README. [6]
If you want, I can: (a) show short code examples for Compare/IsValid using x/mod or Masterminds, or (b) recommend one library tailored to your exact needs (requirement: simple compare vs. range/constraint support).
Use a proper semver comparison library instead of string comparison.
String comparison fails for semantic versions with multi-digit components. For example, "v1.10.0" < "v1.9.0" evaluates to true lexicographically, but semantically v1.10.0 comes after v1.9.0. Use golang.org/x/mod/semver (standard library) or another dedicated semver library for correct ordering validation.
Example fix
import "golang.org/x/mod/semver"
// Replace the string comparison with semver comparison
if semver.Compare(deprecation.PlannedRemovalRelease, deprecation.DeprecationRelease) <= 0 {
return errors.Errorf("PlannedRemovalRelease %q must be after DeprecationRelease %q",
deprecation.PlannedRemovalRelease, deprecation.DeprecationRelease)
}🤖 Prompt for AI Agents
In `@pkg/config/resource.go` around lines 1021 - 1028, The code currently uses
lexicographic string comparison to validate that
deprecation.PlannedRemovalRelease is after deprecation.DeprecationRelease, which
fails for multi-digit semver components; replace the string comparison with a
proper semantic version comparison using golang.org/x/mod/semver (import it) and
call semver.Compare(deprecation.PlannedRemovalRelease,
deprecation.DeprecationRelease) to ensure the planned removal is strictly
greater; keep the existing error message when the comparison indicates the
planned removal is not after the deprecation.
| // findDescriptionBlock finds the description comment block before the type | ||
| func findDescriptionBlock(lines []string, typeLineIndex int) (int, int) { | ||
| descriptionStartIndex := -1 | ||
| descriptionEndIndex := -1 | ||
|
|
||
| for i := typeLineIndex - 1; i >= 0 && i >= typeLineIndex-10; i-- { | ||
| line := strings.TrimSpace(lines[i]) | ||
| if line == "" { | ||
| if descriptionEndIndex != -1 { | ||
| break | ||
| } | ||
| continue | ||
| } | ||
| if strings.HasPrefix(line, "//") && !strings.HasPrefix(line, "// +kubebuilder:") { | ||
| if descriptionEndIndex == -1 { | ||
| descriptionEndIndex = i | ||
| } | ||
| descriptionStartIndex = i | ||
| } else { | ||
| if descriptionEndIndex != -1 { | ||
| break | ||
| } | ||
| if strings.HasPrefix(line, "// +kubebuilder:") { | ||
| continue | ||
| } | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return descriptionStartIndex, descriptionEndIndex | ||
| } |
There was a problem hiding this comment.
Remove fixed look‑back limits to avoid missing doc blocks.
The 10/20-line caps can skip the description line when many kubebuilder markers exist, leaving stale or missing deprecation notices. Scanning back to the first non-comment line is safer.
🛠️ Proposed fix
- for i := typeLineIndex - 1; i >= 0 && i >= typeLineIndex-10; i-- {
+ for i := typeLineIndex - 1; i >= 0; i-- {
@@
- for i := typeLineIndex - 1; i >= 0 && i >= typeLineIndex-20; i-- {
+ for i := typeLineIndex - 1; i >= 0; i-- {Also applies to: 337-349
🤖 Prompt for AI Agents
In `@pkg/pipeline/hooks_deprecated_version.go` around lines 251 - 281, The current
findDescriptionBlock function uses a fixed look-back limit (typeLineIndex-10)
which can skip doc lines when many kubebuilder markers exist; remove the hard
cap and iterate i from typeLineIndex-1 down to 0, stopping only when you hit the
first non-comment/non-kubebuilder line (but still treating "//" as description
lines and ignoring "// +kubebuilder:" markers), and update
descriptionStartIndex/descriptionEndIndex accordingly; apply the identical
change to the analogous function around lines 337-349 so both scanning routines
no longer use the 10/20-line limits.
ulucinar
left a comment
ulucinar
left a comment
There was a problem hiding this comment.
Hi @sergenyalcin,
I've some further suggestions for simplification. I don't see them as blockers as the current implementation of the post hooks are more generalized than I believe they need to be (in other words, I think there's some room for further simplification by just handling the specific shape/assumptions of upjet's code generation pipeline).
| hasCurrentVersion := false | ||
| for _, v := range versions { | ||
| if v == r.Version { | ||
| hasCurrentVersion = true | ||
| break | ||
| } | ||
| } | ||
| if !hasCurrentVersion { | ||
| return errors.Errorf("served versions must include the current version %q", r.Version) | ||
| } |
There was a problem hiding this comment.
Let's simplify by merging this loop with the one that checks for known versions below or we could also check simply for !slices.Contains(versions, r.Version).
| // - If ServedVersions is explicitly set, it's used as-is | ||
| // - If ServedVersions is empty/nil, all versions are served by default | ||
| // - The returned slice is a new slice to prevent external modifications | ||
| func (r *Resource) GetServedVersions() []string { |
There was a problem hiding this comment.
So at the API level, we don't enforce this, even if the provider has multiversion CRDs. However, in the current code-generation pipeline, we assume this is properly configured.
What I understand from this statement that in the current shape of the code generation pipeline (without the changes from this PR), if a provider has multiple versions for a CRD, the corresponding PreviousVersions must already be configured properly, otherwise the generation already produces erroneous code, right (as its assumptions are violated)?
| // resource.Version = "v1beta2" | ||
| // resource.PreviousVersions = []string{"v1beta1"} | ||
| // err := resource.SetServedVersions([]string{"v1beta2", "v1beta1"}) | ||
| func (r *Resource) SetServedVersions(versions []string) error { |
There was a problem hiding this comment.
Let's add some tests for these methods in resource_test.go.
|
|
||
| // Run implements postGenerationHook for lifecycleMarkerUpdater. | ||
| func (mu *lifecycleMarkerUpdater) Run(runner *PipelineRunner, provider *config.Provider, resourcesGroups map[string]map[string]map[string]*config.Resource) error { //nolint:gocyclo // easier to follow as a unit | ||
| mu.runner = runner |
There was a problem hiding this comment.
We had better do this during the construction (in the new... functions). We also had better remove the pipeline parameter itself from postGenerationHook.Run. We could also just receive the value of the DirAPIs instead of the whole pipeline object for the construction of these post hooks (for information hiding).
| filePath := filepath.Join(mu.runner.DirAPIs, shortGroup, version, fileName) | ||
|
|
||
| // Validate the file path is within the expected directory | ||
| if err := validateFilePath(filePath, mu.runner.DirAPIs); err != nil { |
There was a problem hiding this comment.
Why do we need a validation on file path here? We compute it directly from the resource configuration, i.e., pipeline's DirAPIs, resource's short group, version and kind.
There was a problem hiding this comment.
I believe we could simplify all these by:
- Finding on the first occurrence of the
+kubebuilder:object:root=truemarker, then- Checking whether the
+kubebuilder:deprecatedversion:warningmarker exists or not - Checking whether the
+kubebuilder:storageversionexists or not - Checking whether the
+kubebuilder:unservedversionmarker exists or not
- Checking whether the
until the first line that does not start with a comment, e.g., ^//.* (another strong assumption).
The +kubebuilder:object:root=true can appear at two places in one of our generated files: Before the root object type for the MR, and before the corresponding list type. The MR type occurrence always comes before the list type's marker (strong assumption).
I think it's okay to make these strong assumptions for our generated files. It would also be good to add some comments telling that we prefer to add/remove these markers during the code generation itself once we support it (hence, the post hooks are a workaround).
2 participants
Description of your changes
This PR adds infrastructure for managing the lifecycle of Managed Resource API versions, in accordance with Kubernetes deprecation policies.
ServedVersions: Controls which API versions the API server exposesDeprecatedVersions: Maps versions to deprecation metadata (warning messages, release info)GetServedVersions(): Provides safe access with proper defaulting behavior+kubebuilder:deprecatedversion:warningmarkers for deprecated versions+kubebuilder:unservedversionmarkers for removed versionsNewVersionMarkerUpdateHook(): Updates lifecycle markers in previous version filesNewStorageVersionMarkerUpdateHook(): Ensures only the configured storage version has the markerI have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Tested in the provider-upjet azuread and provider-upjet-gcp. Observed successful generations. Deprecation and serving version markers were observed. There are no unintended changes.
Please see the provider branches that consume this change:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.