conversion: Return typed error on failure to convert for CRs #115650

MadhavJivrajani · 2023-02-09T10:57:43Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Return a typed error that also passes as a "Missing Version Error"
so that server side apply updates are not blocked from happening on
custom resources that have a version that is no longer available.

This commit also adds an integration test to verify the same.

Which issue(s) this PR fixes:

Fixes #111937

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-02-09T10:57:46Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

apelisse

I'm not super familiar with this code but that seems reasonable. Can you add a test to reproduce the bug that this is attempting to fix in ssa? Thank you!

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go

MadhavJivrajani · 2023-02-22T15:05:01Z

@apelisse I've added a reproducer test for the bug now, could you PTAL? Thanks!

MadhavJivrajani · 2023-02-22T15:10:57Z

/test pull-kubernetes-conformance-kind-ipv6-parallel
(container not ready)

MadhavJivrajani · 2023-02-22T15:46:04Z

/kind bug

MadhavJivrajani · 2023-07-04T13:18:34Z

@liggitt @apelisse I've addressed most comments (except #115650 (comment)), could you PTAL?
Apologies, this slipped through the cracks 🙏🏼

apelisse · 2023-07-05T17:47:55Z

test/integration/apiserver/apply/apply_crd_test.go

+	// Migrate existing CR to new storage version by applying an
+	// empty merge patch.
+	_, err = dynamicClient.Resource(gvr).
+		Patch(context.TODO(), name, types.MergePatchType, []byte("{}"), metav1.PatchOptions{})


Maybe send an apply in the new version that changes other fields, so that you have two entries at that point, one in the old version and one in the new version.

apelisse · 2023-07-05T17:49:15Z

test/integration/apiserver/apply/apply_crd_test.go

+
+	apiVersion = noxuDefinition.Spec.Group + "/" + noxuDefinition.Spec.Versions[0].Name
+	yamlBody = []byte(fmt.Sprintf(`
+apiVersion: %s


Make sure you send new fields in that apply so that you can see from the list of managed fields exactly what happened. (i.e. the old entry was removed even though it still should have owned some fields), the second entry that was inserted also still exists, and that last one also exist. You should have two entries at the end.

dprotaso · 2024-01-09T18:35:43Z

@MadhavJivrajani bump

MadhavJivrajani · 2024-01-10T08:28:12Z

Thanks for the ping, this had dropped off my radar. Planning to get back to this.

This commit adds the ability to inject a custom string instead of the default scheme name. It also consequently edits what is printed when a custom string is passed. This commit also adds tests to exercise the different code paths taken for notRegisteredErr. This is needed when we want to return typed errors and scheme names are not known before hand, such as conversion in custom resources. Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

Return a typed error that also passes as a "Missing Version Error" so that server side apply updates are not blocked from happening on custom resources that have a version that is no longer available. This commit also adds an integration test to verify the same. Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

k8s-ci-robot · 2024-01-10T09:39:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MadhavJivrajani
Once this PR has been reviewed and has the lgtm label, please assign jpbetz for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

MadhavJivrajani · 2024-01-10T09:41:56Z

@apelisse 👋🏼
I think I understand what you were asking from the test, but still not a 100% sure.
I've pushed a commit with some log lines that tell what is happening at each stage, could you please take a look and correct me if I'm misunderstanding something here?

Copy pasting the output here for easy access:

    apply_crd_test.go:866: Created CR: {"apiVersion":"mygroup.example.com/v1beta2","kind":"WishIHadChosenNoxu","metadata":{"creationTimestamp":"2024-01-10T09:37:35Z","generation":1,"managedFields":[{"apiVersion":"mygroup.example.com/v1beta2","fieldsType":"FieldsV1","fieldsV1":{"f:spec":{"f:cronSpec":{},"f:ports":{"k:{\"containerPort\":80,\"protocol\":\"TCP\"}":{".":{},"f:containerPort":{},"f:name":{},"f:protocol":{}}},"f:replicas":{}}},"manager":"apply_test","operation":"Apply","time":"2024-01-10T09:37:35Z"}],"name":"mytest","resourceVersion":"90","uid":"e306af17-e9e1-4efb-b480-49b79da8fd9f"},"spec":{"cronSpec":"* * * * */5","ports":[{"containerPort":80,"name":"x","protocol":"TCP"}],"replicas":1}}
        
    apply_crd_test.go:882: 
        
        Updated CRD, migrated away from old storage version: [v1beta2 v1beta1]
    apply_crd_test.go:898: 
        
        Migrated CR to new storage version after patch (should have 2 managedFields entries in different versions): &{map[apiVersion:mygroup.example.com/v1beta1 kind:WishIHadChosenNoxu metadata:map[creationTimestamp:2024-01-10T09:37:35Z generation:2 managedFields:[map[apiVersion:mygroup.example.com/v1beta2 fieldsType:FieldsV1 fieldsV1:map[f:spec:map[f:cronSpec:map[] f:ports:map[k:{"containerPort":80,"protocol":"TCP"}:map[.:map[] f:containerPort:map[] f:name:map[] f:protocol:map[]]]]] manager:apply_test operation:Apply time:2024-01-10T09:37:35Z] map[apiVersion:mygroup.example.com/v1beta1 fieldsType:FieldsV1 fieldsV1:map[f:spec:map[f:replicas:map[]]] manager:apply.test operation:Update time:2024-01-10T09:37:35Z]] name:mytest resourceVersion:92 uid:e306af17-e9e1-4efb-b480-49b79da8fd9f] spec:map[cronSpec:* * * * */5 ports:[map[containerPort:80 name:x protocol:TCP]] replicas:2]]}
    apply_crd_test.go:911: 
        
        Updated CRD, remove old storage version from status: [v1beta1]
    apply_crd_test.go:954: 
        
        applying object again but with changed replicas (should have 2 converted managedFields entries): {"apiVersion":"mygroup.example.com/v1beta1","kind":"WishIHadChosenNoxu","metadata":{"creationTimestamp":"2024-01-10T09:37:35Z","generation":2,"managedFields":[{"apiVersion":"mygroup.example.com/v1beta1","fieldsType":"FieldsV1","fieldsV1":{"f:spec":{"f:cronSpec":{},"f:ports":{"k:{\"containerPort\":80,\"protocol\":\"TCP\"}":{".":{},"f:containerPort":{},"f:name":{},"f:protocol":{}}},"f:replicas":{}}},"manager":"apply_test","operation":"Apply","time":"2024-01-10T09:37:35Z"},{"apiVersion":"mygroup.example.com/v1beta1","fieldsType":"FieldsV1","fieldsV1":{"f:spec":{"f:replicas":{}}},"manager":"apply.test","operation":"Update","time":"2024-01-10T09:37:35Z"}],"name":"mytest","resourceVersion":"96","uid":"e306af17-e9e1-4efb-b480-49b79da8fd9f"},"spec":{"cronSpec":"* * * * */5","ports":[{"containerPort":80,"name":"x","protocol":"TCP"}],"replicas":2}}

MadhavJivrajani · 2024-01-10T09:59:59Z

If I run the added test against master that does not have the new typed error logic, I get a failure consistent with the original bug report:

    apply_crd_test.go:866: Created CR: {"apiVersion":"mygroup.example.com/v1beta2","kind":"WishIHadChosenNoxu","metadata":{"creationTimestamp":"2024-01-10T09:54:49Z","generation":1,"managedFields":[{"apiVersion":"mygroup.example.com/v1beta2","fieldsType":"FieldsV1","fieldsV1":{"f:spec":{"f:cronSpec":{},"f:ports":{"k:{\"containerPort\":80,\"protocol\":\"TCP\"}":{".":{},"f:containerPort":{},"f:name":{},"f:protocol":{}}},"f:replicas":{}}},"manager":"apply_test","operation":"Apply","time":"2024-01-10T09:54:49Z"}],"name":"mytest","resourceVersion":"89","uid":"fca7007c-de0d-4048-b714-c777b13103c4"},"spec":{"cronSpec":"* * * * */5","ports":[{"containerPort":80,"name":"x","protocol":"TCP"}],"replicas":1}}
        
    apply_crd_test.go:882: 
        
        Updated CRD, migrated away from old storage version: [v1beta2 v1beta1]
    apply_crd_test.go:898: 
        
        Migrated CR to new storage version after patch: &{map[apiVersion:mygroup.example.com/v1beta1 kind:WishIHadChosenNoxu metadata:map[creationTimestamp:2024-01-10T09:54:49Z generation:2 managedFields:[map[apiVersion:mygroup.example.com/v1beta2 fieldsType:FieldsV1 fieldsV1:map[f:spec:map[f:cronSpec:map[] f:ports:map[k:{"containerPort":80,"protocol":"TCP"}:map[.:map[] f:containerPort:map[] f:name:map[] f:protocol:map[]]]]] manager:apply_test operation:Apply time:2024-01-10T09:54:49Z] map[apiVersion:mygroup.example.com/v1beta1 fieldsType:FieldsV1 fieldsV1:map[f:spec:map[f:replicas:map[]]] manager:apply.test operation:Update time:2024-01-10T09:54:49Z]] name:mytest resourceVersion:91 uid:fca7007c-de0d-4048-b714-c777b13103c4] spec:map[cronSpec:* * * * */5 ports:[map[containerPort:80 name:x protocol:TCP]] replicas:2]]}
    apply_crd_test.go:911: 
        
        Updated CRD, remove old storage version from status: [v1beta1]
E0110 15:24:49.608597   46532 status.go:71] apiserver received an error that is not an metav1.Status: &errors.errorString{s:"request to convert CR to an invalid group/version: mygroup.example.com/v1beta2"}: request to convert CR to an invalid group/version: mygroup.example.com/v1beta2
    apply_crd_test.go:951: failed to create custom resource with apply: an error on the server ("unknown") has prevented the request from succeeding:
        k8s
        

        v1Statusf
        
        FailureNrequest to convert CR to an invalid group/version: mygroup.example.com/v1beta2"0?"

MadhavJivrajani · 2024-01-10T10:29:04Z

/retest

MadhavJivrajani · 2024-01-16T06:49:08Z

/retest

dprotaso · 2024-01-17T16:25:58Z

Can we cherry pick this change back to supported releases? This issue has affected K8s since 1.22

jpbetz · 2024-01-23T15:07:40Z

staging/src/k8s.io/apimachinery/pkg/runtime/error.go

+	if len(s) == 0 {
+		return ""
+	}
+	return " in scheme " + strconv.Quote(string(s))


nit: Padding the context with a space prefix here feels clunky. Could we do all the formatting in one place?

@jpbetz the reason for this is in how the notRegisteredErr string is formatted here: https://github.com/MadhavJivrajani/kubernetes/blob/fa6dcc734bf6d6faa720714628b8709c27736f19/staging/src/k8s.io/apimachinery/pkg/runtime/error.go#L73

There are two main cases to take care of: when we use a genericContext and when we use a schemeContext. If the genericContext is used (meaning we already know what the scheme is, such as in cases of built in types), things would be okay for the most part, however, if we use a schemeContext (for when we don't know the scheme before hand such as in CRs), we need to add in a in scheme there towards the end of the formatted notRegisteredErr error string. So to handle both cases and not overcomplicate the formatting logic, a space is padded here.

jpbetz · 2024-01-23T15:08:03Z

staging/src/k8s.io/apimachinery/pkg/runtime/error.go

+	if len(s) == 0 {
+		return ""
+	}
+	return " " + string(s)


jpbetz · 2024-01-23T15:09:59Z

staging/src/k8s.io/apimachinery/pkg/runtime/error.go

+	return &notRegisteredErr{context: schemeContext(schemeName), gvk: gvk, target: target}
+}
+
+func NewNotRegisteredGVKErrForTargetWithContext(context string, gvk schema.GroupVersionKind, target GroupVersioner) error {


I was surprised to see context use type string here. It makes it easy to bypass the use of genericContext (..and the formatting that genericContext adds, which makes me think we should handle the formatting differently..)

context is probably a misnomer, but happy to change what it is called.

It makes it easy to bypass the use of genericContext (..and the formatting that genericContext adds, which makes me think we should handle the formatting differently..)

Not sure I understand the bypassing part here, could you please elaborate?

cc @liggitt

fabriziopandini · 2024-02-14T19:21:27Z

This issue is biting us pretty hard in Cluster API, is there any chance to backport this to older releases of Kubernetes (as much as possible?)

dims · 2024-02-14T23:55:33Z

/assign @liggitt @apelisse

liggitt · 2024-02-15T15:37:13Z

This issue is biting us pretty hard in Cluster API, is there any chance to backport this to older releases of Kubernetes (as much as possible?)

It's really hard to reason about the implications of the change, so doing that safely on master is the first step. It's too early to comment about the possibility of backports, but my inclination would be not to backport, since this isn't a regression (and the implications are super hard to reason about).

k8s-triage-robot · 2024-05-15T16:12:33Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

dprotaso · 2024-05-30T18:35:49Z

but my inclination would be not to backport

Yeah sounds good @liggitt

@MadhavJivrajani Is there any other action here for this PR?

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2023

k8s-ci-robot requested review from sttts and yue9944882 February 9, 2023 10:58

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 9, 2023

apelisse reviewed Feb 10, 2023

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go Outdated Show resolved Hide resolved

MadhavJivrajani force-pushed the ssa-conversion-return-typed-error branch from 318b8b9 to d4c3974 Compare February 22, 2023 15:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 22, 2023

MadhavJivrajani marked this pull request as ready for review February 22, 2023 15:04

k8s-ci-robot requested review from deads2k and lavalamp February 22, 2023 15:04

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 22, 2023

MadhavJivrajani force-pushed the ssa-conversion-return-typed-error branch from 8689845 to 2c1cadb Compare July 4, 2023 13:14

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2023

MadhavJivrajani requested review from apelisse and liggitt July 4, 2023 13:20

apelisse reviewed Jul 5, 2023

View reviewed changes

MadhavJivrajani added 3 commits January 10, 2024 14:02

flesh out integration test

fa6dcc7

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

MadhavJivrajani force-pushed the ssa-conversion-return-typed-error branch from 2c1cadb to fa6dcc7 Compare January 10, 2024 09:38

MadhavJivrajani requested a review from apelisse January 11, 2024 16:13

jpbetz suggested changes Jan 23, 2024

View reviewed changes

liggitt mentioned this pull request Feb 14, 2024

SSA failure after removal of old API versions kubernetes-sigs/cluster-api#10051

Open

k8s-ci-robot assigned apelisse and liggitt Feb 14, 2024

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conversion: Return typed error on failure to convert for CRs #115650

conversion: Return typed error on failure to convert for CRs #115650

MadhavJivrajani commented Feb 9, 2023 •

edited

k8s-ci-robot commented Feb 9, 2023

apelisse left a comment

MadhavJivrajani commented Feb 22, 2023

MadhavJivrajani commented Feb 22, 2023

MadhavJivrajani commented Feb 22, 2023

MadhavJivrajani commented Jul 4, 2023

apelisse Jul 5, 2023

apelisse Jul 5, 2023

dprotaso commented Jan 9, 2024

MadhavJivrajani commented Jan 10, 2024

k8s-ci-robot commented Jan 10, 2024

MadhavJivrajani commented Jan 10, 2024

MadhavJivrajani commented Jan 10, 2024 •

edited

MadhavJivrajani commented Jan 10, 2024

MadhavJivrajani commented Jan 16, 2024

dprotaso commented Jan 17, 2024

jpbetz Jan 23, 2024

MadhavJivrajani Jan 25, 2024

jpbetz Jan 23, 2024

jpbetz Jan 23, 2024 •

edited

MadhavJivrajani Jan 25, 2024

MadhavJivrajani Jan 25, 2024

fabriziopandini commented Feb 14, 2024

dims commented Feb 14, 2024

liggitt commented Feb 15, 2024

k8s-triage-robot commented May 15, 2024

dprotaso commented May 30, 2024

conversion: Return typed error on failure to convert for CRs #115650

Are you sure you want to change the base?

conversion: Return typed error on failure to convert for CRs #115650

Conversation

MadhavJivrajani commented Feb 9, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Feb 9, 2023

apelisse left a comment

Choose a reason for hiding this comment

MadhavJivrajani commented Feb 22, 2023

MadhavJivrajani commented Feb 22, 2023

MadhavJivrajani commented Feb 22, 2023

MadhavJivrajani commented Jul 4, 2023

apelisse Jul 5, 2023

Choose a reason for hiding this comment

apelisse Jul 5, 2023

Choose a reason for hiding this comment

dprotaso commented Jan 9, 2024

MadhavJivrajani commented Jan 10, 2024

k8s-ci-robot commented Jan 10, 2024

MadhavJivrajani commented Jan 10, 2024

MadhavJivrajani commented Jan 10, 2024 • edited

MadhavJivrajani commented Jan 10, 2024

MadhavJivrajani commented Jan 16, 2024

dprotaso commented Jan 17, 2024

jpbetz Jan 23, 2024

Choose a reason for hiding this comment

MadhavJivrajani Jan 25, 2024

Choose a reason for hiding this comment

jpbetz Jan 23, 2024

Choose a reason for hiding this comment

jpbetz Jan 23, 2024 • edited

Choose a reason for hiding this comment

MadhavJivrajani Jan 25, 2024

Choose a reason for hiding this comment

MadhavJivrajani Jan 25, 2024

Choose a reason for hiding this comment

fabriziopandini commented Feb 14, 2024

dims commented Feb 14, 2024

liggitt commented Feb 15, 2024

k8s-triage-robot commented May 15, 2024

dprotaso commented May 30, 2024

MadhavJivrajani commented Feb 9, 2023 •

edited

MadhavJivrajani commented Jan 10, 2024 •

edited

jpbetz Jan 23, 2024 •

edited