It's not possible to create a Pod, because of the default overhead value (empty map) in v20.0.0

[kvmw]

Describe the bug

The default overhead value of Pod spec in version 20.0.0 is empty map which is causing the following error:

    java.lang.RuntimeException: io.kubernetes.client.openapi.ApiException: Message: 
    HTTP response code: 403
    HTTP response body: {"kind":"Status","apiVersion":"((redacted))","metadata":{},"status":"Failure","message":"pods \"foo\" is forbidden: pod rejected: Pod Overhead set without corresponding RuntimeClass defined Overhead","reason":"Forbidden","details":{"name":"foo","kind":"pods"},"code":403}

As a workaround, one can set the overhead explicitly to null:

V1Pod pod = new V1Pod()
                .metadata(new V1ObjectMeta().name("foo"))
                .spec(new V1PodSpec()
                        .overhead(null)  // HERE: setting the overhead value to null, to avoid above error. 
                        .addContainersItem(new V1ContainerBuilder()
                        // omitted 

Client Version
e.g. 20.0.0

Kubernetes Version
any version

Java Version
any version

To Reproduce
Create a simple Pod, without specifying anything for overhead:

V1Pod pod = new V1Pod()
                .metadata(new V1ObjectMeta().name("foo"))
                .spec(new V1PodSpec()
                        .addContainersItem(new V1ContainerBuilder()
                        .withImage("any-image")
                        .withName("foo-image").build()));

coreV1Api.createNamespacedPod("default", pod).execute();

Expected behavior
Pod creation should be successful but it fails due to overhead: {} being present in the request payload.

I think, either the default value of overhead should be null or when serialising the request, the empty values should be omitted.
KubeConfig
N/A

Server (please complete the following information):
Any

Additional context
N/A

[brendandburns]

This is either a bug in the upstream swagger specification, or the way it is translated in the code generator. We'll have to investigate.

[brendandburns]

The more I think about this, the more I think that it is a bug in Kubernetes. I feel like empty map and null map should be equivalent.

I'll file an issue on the main kubernetes repo and see what people think.

[kvmw]

@brendandburns agree. this might need to be addressed in k8s.

A simple template like the following, fails with the same error:

apiVersion: v1
kind: Pod
metadata:
  name: foo
spec:
  overhead: {}
  containers:
  - image: nginx
    name: foo

error:

Error from server (Forbidden): error when creating "foo.yaml": pods "foo" is forbidden: pod rejected: Pod Overhead set without corresponding RuntimeClass defined Overhead

[yue9944882]

kubernetes/kubernetes#123128

i previously opened this PR will fix the issue in upstream, for now the pod creation via v20.0.0 is not working, but i will get back with a workaround. the PR will take some time before merged & released

[Jonpez2]

I think there's a bigger problem here, but it's entirely possible I have the wrong end of the stick in which case please feel free to dismiss. I think this has broken backwards compatibility: now that all fields have defaults, when a new field is added to a resource that field will immediately start getting streamed by the client. But the server, on an older version, quite rightly rejects unknown fields (even those set to some null/empty value). Thus: breakage.

(P.S. - I love the new builder-style api. Really fantastic upgrade, thank you.)

[Jonpez2]

Am I off-base?

[brendandburns]

@Jonpez2 sorry for the delayed response.

20.0.x is a known breaking changes release, because of the move to the new generator style.
20.0.x-legacy is the old api which won't have these issues.

We're going to work on resolving them as best we can (I think that this exposing a bug w/ the upstream apiserver, honestly) but we'll keep this open until it is resolved.

[rjeberhard]

I'd like to argue in favor of not treating this as a bug in the upstream server and instead pushing for a fix to the template generation / generator style.

First, there exist cases, such as the CRD status subresource, where you configure the behavior by including an empty element:

      # subresources describes the subresources for custom resources.
      subresources:
        # status enables the status subresource.
        status: {}

This suggests to me that there may be other such cases that I'm not aware of and that the generator can not assume that the API server can be updated to ignore empty elements where nonexistent elements were previously required.

Second, this issue significantly impacts exports to YAML using the included SnakeYaml Yaml.dump() methods. The output now has empty elements for every one of these Map-typed fields. Staying with the example from CRD's above, when a CRD includes an OpenAPI v3 schema, it must not include fields like definitions and dependencies even though these exist in the schema. Presently, we are having to walk the bean tree and set all of these Map-typed fields to null before generating YAML.

@brendandburns, do you know if there is already a bug against the template generation? If so, I'd like to see the discussion there and likely request some solution such as not initializing fields to a new HashMap if those fields are not required.

[rjeberhard]

Do we think that this behavior will change (such as through a change in the upstream template generation) or should we start analyzing how to workaround?

[Jonpez2]

Thanks for the reply @brendandburns - somehow I missed it. For what it's worth I strongly agree with others on this thread that we cannot assume that default values are exactly equivalent to unset fields (or even nonexistent ones, in the case of version skew across a newly introduced field). It makes the deserialization code much more complicated and hard to reason about.

[rjeberhard]

This appears to be the upstream bug and proposed fix:
OpenAPITools/openapi-generator#18735
OpenAPITools/openapi-generator#18738

[k8s-triage-robot]

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

This bot triages un-triaged issues 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 issue is closed

You can:

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

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

/lifecycle stale

[brendandburns]

/lifecycle-frozen

[brendandburns]

/lifecycle frozen

[rjeberhard]

Just updating here for efforts in openapi-genreator...

There was discussion of the earlier change in the openapi-generator causing regressions such as the one discussed here. Unfortunately, those regressions seemed the appropriate fall out of properly interpreting the OpenAPI specification.

The team offered an option, SET_CONTAINER_TO_NULLABLE, described here: OpenAPITools/openapi-generator#18735, and merged here: OpenAPITools/openapi-generator#18128, that should let this project select the working behavior.

[Jonpez2]

This may be way too big of a diversion, but are there any plans for supporting protobufs? They have a significantly better backwards compatibility story, and as far as I understand it’s natively supported by kubernetes?

…

On Fri, 6 Sep 2024 at 18:09, Ryan Eberhard ***@***.***> wrote: Just updating here for efforts in openapi-genreator... There was discussion of the earlier change in the openapi-generator causing regressions such as the one discussed here. Unfortunately, those regressions seemed the appropriate fall out of properly interpreting the OpenAPI specification. The team offered an option, SET_CONTAINER_TO_NULLABLE, described here: OpenAPITools/openapi-generator#18735 <OpenAPITools/openapi-generator#18735>, and merged here: OpenAPITools/openapi-generator#18128 <OpenAPITools/openapi-generator#18128>, that should let this project select the working behavior. — Reply to this email directly, view it on GitHub <#3076 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABN425KAQJGPL5VWUVM23C3ZVHOWLAVCNFSM6AAAAABDH54Y46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZUGQ4DKNRVGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rjeberhard]

@Jonpez2, I see that there are already protobuf samples: https://github.com/kubernetes-client/java/blob/master/examples/examples-release-18/src/main/java/io/kubernetes/client/examples/ProtoExample.java.

[Jonpez2]

Thanks for that. Having dug a little deeper I now understand that custom resource definitions aren’t supported by the protobuf api, so this would be at best a partial solution.

…

On Fri, 6 Sep 2024 at 19:07, Ryan Eberhard ***@***.***> wrote: @Jonpez2 <https://github.com/Jonpez2>, I see that there are already protobuf samples: https://github.com/kubernetes-client/java/blob/master/examples/examples-release-18/src/main/java/io/kubernetes/client/examples/ProtoExample.java . — Reply to this email directly, view it on GitHub <#3076 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABN425M32TIP7OTOJNDKVKTZVHVPNAVCNFSM6AAAAABDH54Y46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZUGU3TMMJUGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[brendandburns]

@rjeberhard thanks for the pointer, I'll see about integrating that into our generation. Sadly github action code generation is currently broken by: kubernetes/org#5126 but I'll see if I can generate locally.

[brendandburns]

PR out to update the gen repo:

kubernetes-client/gen#267

Once that's in we can try regenerating.

[brendandburns]

I've dug into this a little more, I don't think that the API generator patch quite works, because the patch defaults everything to an empty list/map. If we believe that we need to differentiate between null and empty the api generated code currently doesn't do that.

We can perhaps pursue that fix within the upstream generator, but I'm not sure what we want to do for now. I can definitely revise the PR so that it generates null instead of empty list, but as you mention above, I suspect that will break some people. Perhaps we can make it configuration so that users of the library can choose?

[rjeberhard]

Thanks for trying, @brendandburns. I'm not sure what to suggest either. The specific behavior I was looking for is that V1PodSpec#overhead would default to null (which appears unchanged in the regeneration), but that I would be able to assign an empty map to V1CustomResourceSubresources#status, since that is the appropriate content when you want to enable the status subresource or a CRD. There are likely other good candidate fields to analyze.

I'm perfectly happy with a configuration option, if it comes to that, because I really like the new API design and don't want to always be stuck on the legacy path.

[Jonpez2]

I think it will be awfully hard for people to know which config setting to choose, and the failure modes seem very bad, and only discovered at runtime, right? I also have zero good suggestions, aside from perhaps reinstating the previous behaviour even though it doesn’t conform to the spec, since the spec seems not to be achieving desirable outcomes?

…

On Tue, 10 Sep 2024 at 10:31, Ryan Eberhard ***@***.***> wrote: Thanks for trying, @brendandburns <https://github.com/brendandburns>. I'm not sure what to suggest either. The specific behavior I was looking for is that V1PodSpec#overhead would default to null (which appears unchanged in the regeneration), but that I would be able to assign an empty map to V1CustomResourceSubresources#status, since that is the appropriate content when you want to enable the status subresource or a CRD. There are likely other good candidate fields to analyze. I'm perfectly happy with a configuration option, if it comes to that, because I really like the new API design and don't want to always be stuck on the legacy path. — Reply to this email directly, view it on GitHub <#3076 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABN425OT26CEXEIRCCI7UZDZV37FPAVCNFSM6AAAAABDH54Y46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBRGAYDSNZYGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[martin-traverse]

Hello - I have also hit this issue. I can work around for now by explicitly calling podSpec.setOverhead(null), my pod launches but I don't know what other settings might be affected by this issue.

After reading the thread, and with admittedly limited knowledge, it seems to me that defaulting to null would give greater compatibility between versions, especially since the difference between null and empty map / list carries semantic meaning. I don't know what other issues that would cause though!

[rjeberhard]

@brendandburns, any thoughts on how we might move forward on this? I haven't fully understood the debate over in the OpenAPI Generator project, but I'm assuming that they are now correctly honoring that specification. This might mean that Kubernetes has to adjust the published schema, which would presumably be hard, or we have to find a way to deal with the changes. I'd really like to be able to move forward from the legacy release since you've made really positive changes to the API, but my tests immediately break when I can't create pods.