🌱 Fix SA1019: server-side apply required, needs generated apply configurations#2511
🌱 Fix SA1019: server-side apply required, needs generated apply configurations#2511camilamacedo86 wants to merge 1 commit intooperator-framework:mainfrom
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR addresses the SA1019 deprecation warning by replacing the deprecated client.Apply variable with a custom serverSideApplyPatch implementation. The change maintains server-side apply semantics with field ownership management while avoiding the need to generate ApplyConfiguration types.
Changes:
- Implemented custom
serverSideApplyPatchtype that satisfies theclient.Patchinterface - Replaced deprecated
client.Applyusage withserverSideApplyPatch{} - Removed extensive deprecation comment and nolint directive
- Added two new imports:
k8s.io/apimachinery/pkg/typesandk8s.io/apimachinery/pkg/util/json
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2511 +/- ##
==========================================
+ Coverage 73.21% 73.40% +0.18%
==========================================
Files 102 102
Lines 8504 8564 +60
==========================================
+ Hits 6226 6286 +60
Misses 1799 1799
Partials 479 479
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // nolint:staticcheck // SA1019: server-side apply required, needs generated apply configurations | ||
| return bc.Client.Patch(ctx, rev, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership) | ||
| // Use Server-Side Apply to manage field ownership and handle conflicts during OLM upgrades. | ||
| return bc.Client.Patch(ctx, rev, serverSideApplyPatch{}, client.FieldOwner(bc.FieldOwner), client.ForceOwnership) |
There was a problem hiding this comment.
IMHO, the right fix here would be to use Client.Apply(..) and generate apply configuration from unstructured, see https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/applyconfigurations.go#L38
There was a problem hiding this comment.
Amazing idea. Done !!!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
fix
branch
2 times, most recently
from
767a688 to
83002f3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
This kind of conversion is not safe:
- (typed struct) -> (unstructured) -> (apply configuration)
In this type of conversion it is impossible to tell the difference between:
- Intentionally use the default value of a type for a field, AND
- I don't care about that field
| return bc.Client.Patch(ctx, rev, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership) | ||
| // Convert the revision to an unstructured object to create an apply configuration. | ||
| // This allows us to use Server-Side Apply without needing generated apply types. | ||
| unstructuredRev, err := runtime.DefaultUnstructuredConverter.ToUnstructured(rev) |
There was a problem hiding this comment.
If there is any field in rev without omitzero and omitempty, those fields will be serialized and applied with our field owner, despite rev not having an opinion on them. To use SSA properly there are basically only two options:
- Build unstructured from scratch
- Use an apply configuration type.
There was a problem hiding this comment.
I think the solution here is to generate the apply configurations for our types (which I think is now possible using controller-gen?)
There was a problem hiding this comment.
I think the solution here is to generate the apply configurations for our types (which I think is now possible using controller-gen?)
IMHO, this is a bigger change, perhaps we should merge this first and explore it in another PR?
There was a problem hiding this comment.
I'm reading the comments that are removed and it seems like they all still apply though? So at a minimum, I think we should restore the comments that talk about needing to use generated applyconfigurations.
Otherwise, what is the functional change here?
Does the switch from this:
bc.Client.Patch(ctx, rev, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
to this (with the rev -> unstructured -> applyConfig traversal):
bc.Client.Apply(ctx, applyConfig, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
have any actual change other than removing our use of a deprecated function?
There was a problem hiding this comment.
Joe I checked this one and accordingly to Claude
Your Current Implementation is Already Correct! ✓
Implements the recommended approach for using Server-Side Apply with controller-runtime v0.23+:
- Manual construction of unstructured objects with only managed fields
- client.ApplyConfigurationFromUnstructured() to create apply configurations
- client.Apply() instead of the deprecated client.Patch() with client.Apply type
For custom CRDs, the manual ApplyConfigurationFromUnstructured approach is the recommended pattern because:
- It gives you fine-grained control over which fields to manage
- Avoids ownership conflicts from zero-value fields
- No code generation required
- Works perfectly with Server-Side Apply semantics
However, I think we should invest some times, maybe create a tech-debt task for deeper analyse the possibility to use controller-tools with serversideapply markers to generate the manifests. Looking for better maintainability in long term.
camilamacedo86
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
I will hold based on the @joelanford feedback #2511 (comment) and solve it in another way /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Hi @joelanford and @pedjak The comments raised were all addressed. So, I think it is good to fly 🚀 now. |
|
/hold cancel All addressed |
openshift-ci
bot
removed
the
do-not-merge/hold
| // We cannot safely convert the typed ClusterExtensionRevision struct directly to unstructured | ||
| // because that would serialize all fields, including zero values for fields without omitempty tags. | ||
| // This would incorrectly claim field ownership for fields we don't manage, violating Server-Side | ||
| // Apply semantics. |
There was a problem hiding this comment.
Just checked ClusterExtensionRevisionSpec and we do not have fields without omitempty tag. Hence, it is safe to use the previous approach with DefaultUnstructuredConverter:
func (bc *Boxcutter) createOrUpdate(ctx context.Context, user user.Info, rev *ocv1.ClusterExtensionRevision) error {
// Run auth preflight checks
if err := bc.runPreAuthorizationChecks(ctx, user, rev); err != nil {
return err
}
data, err := runtime.DefaultUnstructuredConverter.ToUnstructured(rev)
if err != nil {
return err
}
u := &unstructured.Unstructured{Object: data}
gvk, err := apiutil.GVKForObject(rev, bc.Scheme)
if err != nil {
return err
}
u.GetObjectKind().SetGroupVersionKind(gvk)
applyConfig := client.ApplyConfigurationFromUnstructured(u)
// Use Server-Side Apply with field ownership to manage the revision.
return bc.Client.Apply(ctx, applyConfig, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
}why this works?
revargument contains desired resource, created bybc.RevisionGenerator.GenerateRevision- that object contains only the fields we are interested in, it is not pulled from the cache.- even if we had a field to zero value, we should own it, actually we should own the whole
.spec, because we are creating and reconciling CERs - no other controller should be able to change the spec.
There was a problem hiding this comment.
I think is safe we keep as it is
WHY?
If something change there all still working
4 participants
No description provided.