search

kubernetes-client / python

Official Python client library for kubernetes
http://kubernetes.io/
Apache License 2.0
6.78k stars 3.27k forks source link

list_event_for_all_namespaces_with_http_info raises ValueError due to None value for event_time if there are events #1616

Open haxsaw opened 3 years ago

haxsaw commented 3 years ago

What happened (please include outputs or screenshots): If you try to fetch events when there aren't any, the above mentioned method returns just fine. But if there are events, then an exception is raised in the internal setter for the 'event_time' property as the value being read in from K8s is None:

        if self.local_vars_configuration.client_side_validation and event_time is None:  # noqa: E501
            raise ValueError("Invalid value for `event_time`, must not be `None`")  # noqa: E501

or, as it happens:

  File ".../python3.8/site-packages/kubernetes/client/api_client.py", line 303, in __deserialize
    return self.__deserialize_model(data, klass)
  File ".../lib/python3.8/site-packages/kubernetes/client/api_client.py", line 641, in __deserialize_model
    instance = klass(**kwargs)
  File ".../lib/python3.8/site-packages/kubernetes/client/models/events_v1_event.py", line 112, in __init__
    self.event_time = event_time
  File ".../lib/python3.8/site-packages/kubernetes/client/models/events_v1_event.py", line 291, in event_time
    raise ValueError("Invalid value for `event_time`, must not be `None`")  # noqa: E501
ValueError: Invalid value for `event_time`, must not be `None`

It may be the case that turning off client_side_validation will fix this, however there seem to be problems in this regard as well. In issue #1353 there was a mention of a breaking change (in release 1.12?) that involved setting up default config where client_side_validation can supposedly be turned off, but setting default configs don't seem to have any effect. Here's the code inside of the __init__() of EventsV1Api (local_vars_configuration is a parameter):

        if local_vars_configuration is None:
            local_vars_configuration = Configuration()
        self.local_vars_configuration = local_vars_configuration

So if you don't pass in a value of local_vars_configuration, you get a brand-new Configuration object in which client_side_validation is True. One would have thought that the assignment to local_vars_configuration should have been:

           local_vars_configuration = Configuration.get_default_copy()

which would have kind of lined up with the notes for release 12.0.1. More curiously, if you have a look into the __init__() for Configuration itself, you find this comment before setting client_side_validation:

        # Disable client side validation
        self.client_side_validation = True

So the comment implies that there shouldn't be any client side validation by default, but the code does the opposite.

This is kind of feeling like a regression.

What you expected to happen: I was expecting that default behaviour wouldn't raise an exception for fetching regular event data from K8s (or in my case, k3s). My tests don't cover every resource type that could be fetched, so there may be other similar fields lurking within the library.

How to reproduce it (as minimally and precisely as possible):

from kubernetes.client import EventsV1Api
from kubernetes.client import CoreV1Api
from kubernetes import config

def tryit():
    config.load_kube_config(config_file="/etc/rancher/k3s/k3s.yaml")  # the config for k3s
    # you need to first carry out some action that will result in an Event being reported, so we'll make a simple Pod
    p = {'apiVersion': 'v1',
         'kind': 'Pod',
         'metadata': {'labels': {'app': 'myapp'}, 'name': 'myapp-pod'},
         'spec': {'containers': [{'command': ['sh',
                                              '-c',
                                              'echo Hello Kubernetes! && sleep 3600'],
                                  'image': 'busybox',
                                  'name': 'myapp-container'}]}}
    api = CoreV1Api()
    t0 = api.create_namespaced_pod_with_http_info(namespace='default', body=p)
    ev = EventsV1Api()
    t1 = ev.list_event_for_all_namespaces_with_http_info(async_req=False)  # this craters

if __name__ == "__main__":
    tryit()

Anything else we need to know?:

In my dev environment I use k3s, which may be the source for the empty event_time (versions below). Nonetheless, there's clearly something wrong in how default Configurations are being handled.

As this is impacting objects that are being built inside of the K8s Python client itself, it isn't clear that there's a feasible workaround that could involve the user supplying the config object since it appears the default config isn't used and there's no way to pass in a config into list_event_for_all_namespaces_with_http_info().

Environment:

roycaihw commented 2 years ago

/assign

roycaihw commented 2 years ago

This is likely similar to https://github.com/kubernetes-client/gen/issues/52. EventTime is marked as required in the openapi spec but kube-apiserver doesn't always set/validate it. The fix is to mark the field as optional in https://github.com/kubernetes/kubernetes/blob/ae550b62da15ca5fe4983c79aaa6b2a39e3e711a/staging/src/k8s.io/api/events/v1/types.go#L41-L42

/unassign /help

k8s-ci-robot commented 2 years ago

@roycaihw: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes-client/python/issues/1616): >This is likely similar to https://github.com/kubernetes-client/gen/issues/52. EventTime is marked as required in the openapi spec but kube-apiserver doesn't always set/validate it. The fix is to mark the field as optional in https://github.com/kubernetes/kubernetes/blob/ae550b62da15ca5fe4983c79aaa6b2a39e3e711a/staging/src/k8s.io/api/events/v1/types.go#L41-L42 > >/unassign >/help Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
haxsaw commented 2 years ago

If this gets marked as optional will this then propagate to the swagger? And when does that happen; will there be a patch release? How does that address the issue I found of default configs not being applied when no configuration object is supplied to a constructor? Isn't that incorrect? Or else how are we to use the default config?

roycaihw commented 2 years ago

If this gets marked as optional will this then propagate to the swagger?

Yes, the Go type is the source of truth for the swagger in k/k

And when does that happen; will there be a patch release?

Let's fix the issue at HEAD first. Then we can do a patch in this client

So the comment implies that there shouldn't be any client side validation by default, but the code does the opposite

It was a typo in the upstream generator: https://github.com/OpenAPITools/openapi-generator/pull/6850

default configs not being applied when no configuration object is supplied to a constructor

Not sure I follow. The default config is being used here (and used by EventsV1Api here). You can set the default config using Configuration.set_default(c) as https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v1201 mentioned

benholtzmansmith commented 2 years ago

I am also encountering this issue. One note on this comment by @roycaihw :

Not sure I follow. The default config is being used here (and used by EventsV1Api here). You can set the default config using Configuration.set_default(c) as https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v1201 mentioned

@roycaihw the default config is not being used on the models. For example:

https://github.com/kubernetes-client/python/blob/33c5ea9835356410ce4a9fa54a02c6a2a22143c6/kubernetes/client/models/v1beta1_event.py#L78

https://github.com/kubernetes-client/python/blob/33c5ea9835356410ce4a9fa54a02c6a2a22143c6/kubernetes/client/models/events_v1_event.py#L78

When this code is changed to local_vars_configuration = Configuration.get_default_copy() then it is possible to bypass client_side_validation by setting the default configuration's client_side_validation to False

roycaihw commented 2 years ago

@benholtzmansmith Ah that makes sense. @haxsaw Sorry I thought the models would use the config from the API client and misread your comment.

Unable to disable client side validation is a regression. It has been fixed in upstream code generator: https://github.com/OpenAPITools/openapi-generator/pull/8500. We should upgrade the generator version we use to pick up the fix.

k8s-triage-robot commented 2 years ago

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

This bot triages issues and PRs according to the following rules:

You can:

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

/lifecycle stale

roycaihw commented 2 years ago

/remove-lifecycle stale /lifecycle frozen Upgrading openapi-generator: https://github.com/kubernetes-client/python/pull/1743

mistersouls commented 2 years ago

I have implemented this workaound while finxing this issue in our project for those who are interested:

def _workaround_client_validation_side():
    # Unable to disable client side validation is a regression.
    # Kubernetes considers this fields required, but it is not
    # implemented yet for any controller.
    # It has been fixed in upstream code generator by the PR:
    # https://github.com/OpenAPITools/openapi-generator/pull/8500 .
    # The issue is not processed yet:
    # https://github.com/kubernetes-client/python/issues/1616
    #  We hack his value and let our event class to get the
    # good value.
    if "event_time" in EventsV1Event.attribute_map:
        EventsV1Event.attribute_map["event_time"] = "deprecatedLastTimestamp"
jwezel commented 3 months ago

Is there any plan to fix this?

Is there anything I can do to get this fixed?