[bug]: Nullable causes model binding error in webhooks

[ian-buse]

Describe the bug

Enabling nullability in my operator project causes webhooks to fail. When nullability is enabled, I see the following Request.Object.Uid is required error on the kube-apiserver, which it is receiving from ASP.NET:

I0425 03:24:52.491708 1 request.go:1411] body was not decodable (unable to check for Status): Object 'Kind' is missing in '{"type":"https://tools.ietf.org/html/rfc9110#section-15.5.1","title":"One or more validation errors occurred.","status":400,"errors":{"Request.Object.Uid":["The Uid field is required."]},"traceId":"00-1e0d8715878c6606b3c70f8683bd85d3-c57cbcf6dfc4b1a6-00"}' W0425 03:24:52.491764 1 dispatcher.go:225] Failed calling webhook, failing closed mutate.test.company.com.v1: failed calling webhook "mutate.test.company.com.v1": failed to call webhook: the server rejected our request for an unknown reason

And the AdmissionReview Kubernetes is sending in the POST has the following fields for the object (metadata and spec removed for brevity):

{
  "kind": "AdmissionReview",
  "apiVersion": "admission.k8s.io/v1",
  "request": {
    "object": {
      "apiVersion": "company.com/v1",
      "kind": "test",
      "metadata": "my metadata"
      "spec": "my spec"
    },
  }
}

And what little MVC logging there was:

trce: Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker[52] Action Filter: Before executing OnActionExecuting on filter Microsoft.AspNetCore.Mvc.Infrastructure.ModelStateInvalidFilter. dbug: Microsoft.AspNetCore.Mvc.Infrastructure.ModelStateInvalidFilter[1] The request has model state errors, returning an error response.

It seems that the model binder is trying to bind the uid field for the object, but because 1) nullable is enabled, 2) the Uid is not nullable in k8s.models.V1ObjectMeta, and 3) the request does not contain the uid under the request.object, it is failing to bind.

Some Microsoft documentation might be helpful.

To reproduce

1. Run Kubernetes API server with verbose logging.
2. Create a new operator project with a mutation webhook and nullable enabled.
3. Implement the webhook .
4. Run the operator.
5. Try to create the object in Kubernetes.
6. Observe that the Kubernetes API server logs the error given by ASP.NET.
7. Disable nullable in the project.
8. Do steps 4-5 the and observe that the request finishes (though mine is still failing after the 200 status for unknown reasons).

Expected behavior

Model binding should succeed with nullable-enabled projects.

Screenshots

No response

Additional Context

No response

[ian-buse]

I meant to add to this that I used that Microsoft doc to add this line:

 builder.Services
     .AddControllers(o => o.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes = true);

That seems to have fixed the issue. However, I think there is a chance that anyone using webhooks could run into this, and it was a pretty obscure problem to diagnose. I realize this should probably be fixed in the k8s lib, but it would be nice if the operator had a workaround until kubernetes-client/csharp#606 is fixed.

[buehler]

Hey @ian-buse

Is this still an issue? as far as I saw, they closed the issue over at the k8s client.

[ian-buse]

They closed it because they don't have enough contributors to fix it. :( It's still a problem.

[buehler]

Hey @ian-buse

Just encountered this specific issue while updating the dependencies in #764. When I set all these fields as "required" (via [Required] attribute), then the linter is pleased. However this does not seem to fix the error.

If I understand you correctly, the request that is sent does not match the nullable enabled model?

Do you have any idea how we might fix this issue? I don't know if I'm able to allow a lax model binding per request.

If

[ian-buse]

The only thing I've found that fixed the issue is using Action<MvcOptions> when adding the controllers:

var builder = WebApplication.CreateBuilder(args);
 builder.Services
     .AddControllers(o => o.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes = true);

Yeah, the k8s library doesn't support nullable types, and in MVC, the reference types (like the UID string) are considered required by default if they are not nullable. So, when MVC tries to bind the UID and finds it missing in the webhook, it fails because the model expects it to be there.

I looked into adding something in KubeOps.Operator.ServiceCollectionExtensions.AddKubernetesOperator(), but it would require adding a dependency on AspNetCore to the core operator. Maybe an extension method in Operator.Web would work better instead? This was just a quick writeup, haven't tested it:

using KubeOps.Abstractions.Builder;
using KubeOps.Operator.Builder;

using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;

namespace KubeOps.Operator.Web;

/// <summary>
/// Method extensions for the <see cref="IServiceCollection"/>.
/// </summary>
public static class ServiceCollectionExtensions
{
    /// <summary>
    /// Add the Kubernetes operator to the dependency injection.
    /// </summary>
    /// <param name="services"><see cref="IServiceCollection"/>.</param>
    /// <param name="operatorConfigure">An optional configure action for adjusting settings in the operator.</param>
    /// <param name="mvcConfigure">An optional configure action for the webhook MVCs. Defaults to suppressing required
    /// attributes for non-nullable reference types if no configuration is provided.</param>
    /// <returns>An <see cref="IOperatorBuilder"/> for further configuration and chaining.</returns>
    public static IOperatorBuilder AddKubernetesOperator(
        this IServiceCollection services,
        Action<OperatorSettings>? operatorConfigure = null,
        Action<MvcOptions>? mvcConfigure = null)
    {
        services
            .AddControllers(
                mvcConfigure ?? (o => o.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes = true));

        var settings = new OperatorSettings();
        operatorConfigure?.Invoke(settings);
        return new OperatorBuilder(services, settings);
    }
}

This would require making the OperatorBuilder public.

Or, maybe some documentation on the issue would suffice, like adding it to the examples and the README.