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.
| // buildEnhancedDeprecationWarning creates an enhanced deprecation warning message | ||
| // by appending release information to the base warning message if available. | ||
| func buildEnhancedDeprecationWarning(deprecation config.VersionDeprecation) string { | ||
| warning := deprecation.Warning |
There was a problem hiding this comment.
| warning := deprecation.Warning | |
| warning := strings.TrimSpace(deprecation.Warning) |
| // 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.
Because we've a preposition in the name DeprecatedInRelease, for consistency:
| RemovalPlannedRelease string | |
| RemovalPlannedForRelease string |
Or something like DeprecationRelease & PlannedRemovalRelease (or just RemovalRelease concisely). I think these forms are more suitable for a declarative API btw.
| 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.
|
|
||
| // DeprecatedInRelease indicates which provider release deprecated this version. | ||
| // Format: "v1.2.3" | ||
| DeprecatedInRelease string |
There was a problem hiding this comment.
| DeprecatedInRelease string | |
| DeprecationRelease string |
Please see the comment below also.
| // Version: "v1beta2" | ||
| // PreviousVersions: []string{} // v1beta1 removed | ||
| // ServedVersions: []string{"v1beta2"} // Only v1beta2 served | ||
| ServedVersions []string |
There was a problem hiding this comment.
We must ensure that ServedVersions contains Version. It may not be a superset of the set PreviousVersions, i.e., there might be some previous versions that are no longer served. Another constraint could be if a version is not the Version or is not in PreviousVersions, then it cannot be in ServedVersions, i.e., a removed version should not exist in ServedVersions. We will also likely be serving some deprecated versions but not all of them.
We could enforce these rules with a validation, or maybe unexporting ServedVersions and implement the enforcement in accessors. That would also enforce the use of {Set,Get}ServedVersions at the API level.
| // Deprecated versions should still be listed in ServedVersions until they are | ||
| // completely removed from the API. |
There was a problem hiding this comment.
Is this comment correct? I think we can have some deprecated versions that are no longer served, right?
| // - 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.
| if deprecation.RemovalPlannedRelease != "" { | ||
| notice.WriteString(fmt.Sprintf(" and is planned for removal in release %s.", deprecation.RemovalPlannedRelease)) | ||
| } else { | ||
| notice.WriteString(".") |
There was a problem hiding this comment.
nit: If both deprecation and removal releases are empty strings, then the comment is not terminated with a ".".
| } | ||
|
|
||
| // Add deprecation information if this version is deprecated | ||
| if deprecation, isDeprecated := cfg.DeprecatedVersions[cfg.Version]; isDeprecated { |
There was a problem hiding this comment.
I think we don't expect the actively being generated API version to be deprecated, right? This code path still generates the types of only the current API version, right?
| // Check if this version should be marked as unserved | ||
| // Only mark as unserved if ServedVersions is explicitly configured | ||
| // and the current version is NOT in that list | ||
| if len(cfg.ServedVersions) > 0 { |
There was a problem hiding this comment.
Misconfiguration like forgetting to put the current API version into the served versions would result in the current version not being served. Please also see the discussions above. I believe we had better prevent such invalid configuration cases.
| }) | ||
| } | ||
|
|
||
| type storageVersionMarkerUpdater struct { |
There was a problem hiding this comment.
Let's break the implementations of these two hooks into their own files.
| }) | ||
| } | ||
|
|
||
| type markerUpdater struct { |
There was a problem hiding this comment.
Let's break the implementations of these two hooks into their own files.
| }) | ||
| } | ||
|
|
||
| type markerUpdater struct { |
There was a problem hiding this comment.
Because we already have a struct type declaration (markerUpdater), the type can directly implement the PostGenerationHook interface. Or, you can convert updateMarkers into a PostGenerationHookFn but I think having the type is better for breaking its implementation into multiple methods (we can make both work I think).
Please also consider doing this for storageVersionMarkerUpdater.
| }) | ||
| } | ||
|
|
||
| type markerUpdater struct { |
There was a problem hiding this comment.
Would it be possible to use higher-level string functions (search/replace/regex, etc.) to simplify this processor instead of using go/{ast,parser,token} maybe with some assumptions on the shape of the generated Go files? Can you give this a try?
We can also consider this for storageVersionMarkerUpdater.
| Scope: tjtypes.CRDScopeCluster, | ||
| // Register built-in post-generation hooks | ||
| PostGenerationHooks: []PostGenerationHook{ | ||
| NewStorageVersionMarkerUpdateHook(), |
There was a problem hiding this comment.
A high-level comment: (That we also discussed offline) We would like to break the current monolithic pipeline into smaller, modular steps and be able to selectively generate a single version of a single resource, for example. Once we achieve this, it will become much more feasible for us to generate the selected older API versions of selected resources when we need to do so, like there's an API change in the "preferred" version and we need to update the old versions.
This is currently not practical and we are relying on complex post-processors to work around this. And this also means we are building technical debt when we need updates to the older versions (or simply we rely on manual code updates on previously generated code).
I'm a little bit hesitant with the hooks API we are introducing here, although hooks can be utilized for other purposes than what we are doing here with them (updating the already generated versions).
Furthermore, we also setup a higher level code generation pipeline using the //go:generate markers on Go source, like here. So, the hook API here is a lower-level alternative to that with some convenience of itself being able to programmatically access the upjet resource configuration API.
Because of these reasons, I feel like it's better for us at least not to export this API, even if you'd like to utilize the convenience of accessing the resource configuration (which the new API-related configuration becomes part of).
We could also choose to move the new configuration (ServedVersions, etc.) out of upjet's resource configuration so that the implementations of these two post processors can completely be moved outside of upjet's code generation pipeline, and possibly added to the higher-level go:generate-based pipeline above.
But I still think the current resource configuration is better because:
- Resource configuration determines/affects the shapes of the CRDs generated. Other aspects already live in upjet resource configurations. Why would we then move served version or deprecated version configurations out of upjet's resource configuration?
- Once we achieve a modular upjet code generation pipeline, we will need the resource configuration when re-generating the old versions according to the new version. In my opinion, this is a much cleaner, easier to maintain approach, then trying to post-process the already generated (but no longer auto-generated) code.
Another approach could be the move the new configuration and the post processors both out of upjet's code generation pipeline. But I also think why should we not enjoy the convenience as long as long we don't make this part of the API contract, till we do the refactoring on the code generation pipelines?
TLDR; how about unexporting the hooks APIs? If some unrelated use case pops up (some other use case than modifying the auto-generated code), then we can reconsider exporting them. We already know the drawbacks of updating auto-generated code in post processors so maybe we would like to prioritize the pipeline refactoring over adding new features that will require us to update the older versions. So we may have a change to block further technical debt.
When we export this, with a small probability, other provider maintainers may start relying on these APIs to update the older API versions like we do here. And according to our discussions, I believe we've consensus on preventing this as we this as technical debt. So let's be proactive and block the way to technical debt. Maybe some other use case pops up and then we reconsider.
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: