KubernetesMockServer regression for patch including status

[Fabian-K]

Hi,

the 5.4.0 release contains a regression in the KubernetesMockServer when patching a resource containing the status field.

The following code triggers the patch of our Checkmarx custom resource

k8s.checkmarxs()
            .inNamespace(cr.metadata.namespace)
            .withName(cr.metadata.name)
            .patch(cr)

This results in the following patch operation

--> PATCH https://127.0.0.1:56702/apis/beekeeper.sap.it/v1alpha1/namespaces/default/checkmarxs/test
Content-Type: application/json-patch+json; charset=utf-8
Content-Length: 188

[{"op":"replace","path":"/status","value":{"status":"FAILED","serviceAccounts":[],"secrets":[],"configmaps":[],"tasks":[],"pipelines":[]}}]

Pre 5.4.0 and when targeting a real k8s cluster, the status of the CR is updated. With 5.4.0 the status is not updated. I assume this was introduced with #3092

Thanks, Fabian

[shawkins]

This would only occur if against kubernetes if /status were not a subresource. With #3092 (comment) we made the assumption that /status is always a subresource. It would take an enhancement of mock server consulting the crd to understand the difference.

[manusa]

I tried the following reproducer but couldn't make it work:

@EnableKubernetesMockClient(crud = true)
class CustomResourceCrud3144Test {

  KubernetesClient client;

  private MixedOperation<Star, KubernetesResourceList<Star>, Resource<Star>> starClient;

  @BeforeEach
  void setUp() {
    client.apiextensions().v1().customResourceDefinitions()
      .create(CustomResourceDefinitionContext.v1CRDFromCustomResourceType(Star.class).build());
    starClient = client.customResources(Star.class);
  }

  @Test
  @DisplayName("")
  void test() {
    // Given
    final Star sun = new Star();
    sun.setMetadata(new ObjectMetaBuilder().withName("sun").build());
    sun.setSpec(new StarSpec());
    sun.getSpec().setLocation("solar-system");
    final Star savedSun = starClient.inNamespace("default").withName("sun").create(sun);
    savedSun.setStatus(new StarStatus());
    savedSun.getStatus().setLocation("solar-system");
    // When
    starClient.inNamespace("default").withName("sun").patch(savedSun);
    // Then
  }

}

[shawkins]

I tried the following reproducer but couldn't make it work

Can you clarify what you mean by make it work?

With that code snippet the current expectation is that /status will be treated as a subresource and unaffected by the patch call.

[manusa]

make it work as in reproduce it. Very bad choice of words.

I'm trying to reproduce the issue. I'm now grasping that in order to reproducer, the CRD must include the subrerources field definition, right?

[Fabian-K]

        // When
        final Star patched = starClient.inNamespace("default").withName("sun").patch(savedSun);
        // Then
        assertThat(patched.getStatus()).isNotNull();
        assertThat(patched.getStatus().getLocation()).isEqualTo("solar-system");

This fails with 5.4.0 and passes with 5.3.1

[manusa]

From your initial message, for some reason I understood that there was a failing operation, not that the resource was missing the update. OK, got it now, thx.

[shawkins]

@Fabian-K can you clarify does your crd have /status as a subresource?

I'm trying to reproduce the issue. I'm now grasping that in order to reproducer, the CRD must include the subrerources field definition, right?

Sorry I'm probably not being clear either. The mock server does not look at crds. It therefore doesn't actually know if /status is a subresource or not. With #3092 (comment) the mock server is always assuming that /status is a subresource. This means that with 5.4 it has the proper behavior with status subresources, but not if /status is not a subresource. That seemed to be a reasonable assumption given that the former should be the dominant case.

To address this you'd have to further enhance the mock server to consult the crd.

[Fabian-K]

No it does not have the status subresource, just like the Star example does not have it. When only considering the no status subresource case, the behavior of patch was correct (-> matching with a real k8s cluster) with 5.3.1 but is wrong with 5.4.

[manusa]

The removeStatus method seems to be causing problems.

[shawkins]

No it does not have the status subresource, just like the Star example does not have it. When only considering the no status subresource case, the behavior of patch was correct (-> matching with a real k8s cluster) with 5.3.1 but is wrong with 5.4.

Yes the old patch behavior was correct for /status when not a subresource, but incorrect otherwise. The new assumption is that /status is a subresource.

For this not to be an assumption an enhancement is needed.

[shawkins]

The removeStatus method seems to be causing problems.

Yes that was an intentional change as per #3092 (comment)

[manusa]

Yes that was an intentional change as per #3092 (comment)

Yes, I think I got that :)

What I can translate from " It would take an enhancement of mock server consulting the crd to understand the difference." is that from the client perspective:

• when we are patching a standard resource to modify the status we use the /resource/${name}/status endpoint
• when we are patching a custom resource, we check the CRD to see if it has a status subresource enabled.
  • in case it does we use it
  • in case it doesn't we patch the complete resource

But as I check our implementation, we are not making such distinction. So I'm not sure why the mock server should ignore the status despite the fact that the CRD implements or not the status subresource. I'm also assuming that you can modify the status of a standard resource which maybe is not possible.

[shawkins]

But as I check our implementation, we are not making such distinction.

Yes, that is why an enhancement is required.

So I'm not sure why the mock server should ignore the status despite the fact that the CRD implements or not the status subresource.

I don't follow what you are saying here. If the status is a subresource, then the patch won't affect it.

I'm also assuming that you can modify the status of a standard resource which maybe is not possible.

Generally that would not be expected. Even if it's modifiable in a real scenario the respective controller owns the status, so if you make a change it would quickly get change back.

[manusa]

If the status is a subresource, then the patch won't affect it.

true

From https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#status-subresource
"When the status subresource is enabled,... ... PUT/POST/PATCH requests to the custom resource ignore changes to the status stanza."

Even if it's modifiable in a real scenario the respective controller owns the status, so if you make a change it would quickly get change back.

This is what logic and common-sense say, but I'm not sure some real (probably dated) resources will follow this assumption. It's a bad practice anyway, so not applicable.

However, some of the existing tests might be affected by this when preparing the test scenario, so once the issue is addressed we may need to provide some reference.

[shawkins]

so once the issue is addressed we may need to provide some reference.

I just want to clarify that I'm absolutely not opposed to requiring a crd to determine the subresource handling - it was just a lot easier to push that out of scope if we could assume that it was always a subresource.

[shawkins]

@manusa @rohanKanojia shawkins#4 is my best guess as to what this fix could look like. If it seems reasonable, I can open it after #3138

[Lutonite]

I'm having some issues with statuses as well.

I have a method which gets an OpenShift project by name and then ensures the status phase is "Active".
So in my UT I created the following:

var status = new ProjectStatus();
status.setPhase("Active");
var project = new ProjectBuilder()
        .withStatus(status)
        .withNewMetadata()
        .withName(ns1cnc.getName())
        .addToAnnotations(NS_ANNOTATION_REQUESTER, "abc")
        .endMetadata()
        .build();
cncClient.projects().create(project);

Pre-5.4.0 this wouldn't be an issue, but now the project's status is always null when getting it on the mock server.

[shawkins]

I've converted shawkins#4 to #3156