kubernetes-client / java

Official Java client library for kubernetes
http://kubernetes.io/
Apache License 2.0
3.46k stars 1.85k forks source link

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

Open kvmw opened 4 months ago

kvmw commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

https://github.com/kubernetes/kubernetes/pull/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 commented 4 months ago

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 commented 4 months ago

Am I off-base?

brendandburns commented 2 months ago

@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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

This appears to be the upstream bug and proposed fix: https://github.com/OpenAPITools/openapi-generator/issues/18735 https://github.com/OpenAPITools/openapi-generator/pull/18738