Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot deserialize PodExecOptions into DynamicObject #986

Closed
pbzweihander opened this issue Aug 17, 2022 · 2 comments · Fixed by #987
Closed

Cannot deserialize PodExecOptions into DynamicObject #986

pbzweihander opened this issue Aug 17, 2022 · 2 comments · Fixed by #987
Labels
bug Something isn't working core generic apimachinery style work

Comments

@pbzweihander
Copy link
Contributor

Current and expected behavior

When making an admission controller for pod/exec resource, I cannot deserialize the admission request into AdmissionReview<DynamicObject>.

The actual request looks like this:

{
  "kind": "AdmissionReview",
  "apiVersion": "admission.k8s.io/v1",
  "request": {
    "uid": "...",
    "kind": {
      "group": "",
      "version": "v1",
      "kind": "PodExecOptions"
    },
    "resource": {
      "group": "",
      "version": "v1",
      "resource": "pods"
    },
    "subResource": "exec",
    "requestKind": {
      "group": "",
      "version": "v1",
      "kind": "PodExecOptions"
    },
    "requestResource": {
      "group": "",
      "version": "v1",
      "resource": "pods"
    },
    "requestSubResource": "exec",
    "name": "test-pod",
    "namespace": "foobar",
    "operation": "CONNECT",
    "userInfo": {
      "username": "kubernetes-admin",
      "groups": [
        "system:masters",
        "system:authenticated"
      ]
    },
    "object": {
      "kind": "PodExecOptions",
      "apiVersion": "v1",
      "stdin": true,
      "stdout": true,
      "tty": true,
      "container": "test",
      "command": [
        "sh"
      ]
    },
    "oldObject": null,
    "dryRun": false,
    "options": null
  }
}

Deserialization into AdmissionReview<DynamicObject> always fails because the object...

{
  "kind": "PodExecOptions",
  "apiVersion": "v1",
  "stdin": true,
  "stdout": true,
  "tty": true,
  "container": "test",
  "command": [
    "sh"
  ]
}

...does not have metadata.

I think this is a bug.

Possible solution

No response

Additional context

I currently using following type as a hacky workaround:

#[derive(Deserialize, Serialize, Clone)]
struct MetadataLessDynamicObjectForAdmissionReview(serde_json::Value);

impl Resource for MetadataLessDynamicObjectForAdmissionReview {
    type DynamicType = ApiResource;

    fn group(dt: &ApiResource) -> Cow<'_, str> {
        dt.group.as_str().into()
    }

    fn version(dt: &ApiResource) -> Cow<'_, str> {
        dt.version.as_str().into()
    }

    fn kind(dt: &ApiResource) -> Cow<'_, str> {
        dt.kind.as_str().into()
    }

    fn api_version(dt: &ApiResource) -> Cow<'_, str> {
        dt.api_version.as_str().into()
    }

    fn plural(dt: &ApiResource) -> Cow<'_, str> {
        dt.plural.as_str().into()
    }

    fn meta(&self) -> &ObjectMeta {
        panic!("This method `meta` should not be called. This is a bug.")
    }

    fn meta_mut(&mut self) -> &mut ObjectMeta {
        panic!("This method `meta_mut` should not be called. This is a bug.")
    }
}

Environment

Client Version: v1.24.3
Kustomize Version: v4.5.4
Server Version: v1.20.15

Configuration and features

No response

Affected crates

kube-core

Would you like to work on fixing this bug?

No response

@pbzweihander pbzweihander added the bug Something isn't working label Aug 17, 2022
@pbzweihander
Copy link
Contributor Author

pbzweihander commented Aug 17, 2022

I think this is a better workaround (or even a solution, maybe):

#[derive(Deserialize, Serialize, Clone, Debug)]
struct DynamicObjectWithOptionalMetadata {
    #[serde(flatten, default)]
    pub types: Option<TypeMeta>,
    #[serde(default)] // Added
    pub metadata: ObjectMeta,

    #[serde(flatten)]
    pub data: serde_json::Value,
}

impl Resource for DynamicObjectWithOptionalMetadata {
    type DynamicType = ApiResource;

    fn group(dt: &ApiResource) -> Cow<'_, str> {
        dt.group.as_str().into()
    }

    fn version(dt: &ApiResource) -> Cow<'_, str> {
        dt.version.as_str().into()
    }

    fn kind(dt: &ApiResource) -> Cow<'_, str> {
        dt.kind.as_str().into()
    }

    fn api_version(dt: &ApiResource) -> Cow<'_, str> {
        dt.api_version.as_str().into()
    }

    fn plural(dt: &ApiResource) -> Cow<'_, str> {
        dt.plural.as_str().into()
    }

    fn meta(&self) -> &ObjectMeta {
        &self.metadata
    }

    fn meta_mut(&mut self) -> &mut ObjectMeta {
        &mut self.metadata
    }
}

@clux
Copy link
Member

clux commented Aug 17, 2022

Thanks for this report. It looks like Kubernetes does not treat subresources (like PodExecOptions and PodAttachOptions - neither are presented with metadata in the schema) like true (top level) objects and appear exempt from the api convention of metadata for objects.

Maybe there's room for another dynamic type here (like your last suggestion), but I am leaning towards just adding the #[serde(default)] to the DynamicObject's metadata direct and document that metadata for subresources are not filled in and must be read from the parent object. All the properties of ObjectMeta are optional after all (and kubernetes generally treats them as +optional anyway) and we no longer assume a lot about them in ResourceExt after the last deprecation.

@clux clux added the core generic apimachinery style work label Aug 17, 2022
@clux clux closed this as completed in #987 Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core generic apimachinery style work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants