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

Field selector is not applied even when resource fields are matching #3573

Closed
hanneshofmann opened this issue Nov 8, 2021 · 8 comments
Closed

Comments

@hanneshofmann
Copy link

hanneshofmann commented Nov 8, 2021

After upgrading from 4.9.2 to 5.9.0, I have observed that one of our unit tests (which is using KubernetesServer) is failing. It seems that object are not returned when using the Event API with field selectors. I did not yet checked other API's neither whether it's limited to the KubernetesServer.

Following a small reproduction unit test which illustrates the problem:

import io.fabric8.kubernetes.api.model.Event;
import io.fabric8.kubernetes.api.model.ObjectMeta;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.server.mock.KubernetesServer;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.junit.MockitoJUnitRunner;

import java.util.List;

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

@RunWith(MockitoJUnitRunner.class)
public class KubernetesClientTest {

    @Rule
    public KubernetesServer server = new KubernetesServer(true, true);

    @Test
    public void shouldReturnEventMatchingFieldSelector() {
        // given
        KubernetesClient kubernetesClient = server.getClient();
        Event kubernetesEvent = createKubernetesEventWithReason();
        kubernetesClient.v1().events().create(kubernetesEvent);

        // when
        List<Event> kubernetesEvents = kubernetesClient.v1().events().inAnyNamespace().withField("reason", "Rebooted").list().getItems();

        // then
        assertThat(kubernetesEvents.size()).isEqualTo(1);
    }

    private Event createKubernetesEventWithReason() {
        Event  event = new Event();
        ObjectMeta metaData = new ObjectMeta();
        metaData.setName("EventName");
        event.setMetadata(metaData);
        event.setReason("Rebooted");
        return event;
    }
}

Logs from the test execution:

Nov 08, 2021 9:37:39 AM okhttp3.mockwebserver.MockWebServer$2 execute
INFO: MockWebServer[58829] starting to accept connections
Nov 08, 2021 9:37:40 AM okhttp3.mockwebserver.MockWebServer$3 processOneRequest
INFO: MockWebServer[58829] received request: POST /api/v1/namespaces/test/events HTTP/1.1 and responded: HTTP/1.1 200 OK
Nov 08, 2021 9:37:40 AM okhttp3.mockwebserver.MockWebServer$3 processOneRequest
INFO: MockWebServer[58829] received request: GET /api/v1/events?fieldSelector=reason%3DRebooted HTTP/1.1 and responded: HTTP/1.1 200 OK
Nov 08, 2021 9:37:40 AM okhttp3.mockwebserver.MockWebServer$2 acceptConnections
INFO: MockWebServer[58829] done accepting connections: Socket closed

org.opentest4j.AssertionFailedError: 
Expecting:
 <[]>
to be equal to:
 <1>
but was not.
Expected :1
Actual   :[]

Please let me know if further information are required.

@manusa
Copy link
Member

manusa commented Nov 8, 2021

Could you please quickly check if this is still reproducible in #3562 🙏 ?

Maybe we can tackle the issue in scope of that PR.

@manusa
Copy link
Member

manusa commented Nov 8, 2021

I'm testing in scope of #3562, but this is not working here either.

Are you sure this test was working on 4.9? Field selector support was added in #3375, and I don't think we supported any kind of field selection prior to that (in CRUD mode). Now the selection takes place and we are a little more restrictive.

If that's the test you have, probably the reason it passed before is that you are just not checking the opposite (i.e. add another event to the server, the list should still only return one entry ----> my guess is that in 4.9 it will return 2)

@manusa
Copy link
Member

manusa commented Nov 8, 2021

Further support can be included in the MockServer to support additional fields, but this would be still kind of complex and limitless.

e.g. for the Event we could use reflection to infer the top level fields and add attributes for those (if they represent primitive types)

@hanneshofmann
Copy link
Author

@manusa Thanks for confirming. I also did some tests with #3562 and I have observed the same.

Are you sure this test was working on 4.9?

I am sure that our unit test was working with 4.9 but you are right: we did not check the opposite and field selectors were not considered at all (i.e. all events in the requested namespace were returned and our test did simply only use events matching the field selector).

@manusa
Copy link
Member

manusa commented Nov 8, 2021

This is what I suspected after checking the code. TL;DR field selector was not supported in 4.9, and now it's only partially supported.

We can keep this open to explore the addition of attribute computation/extraction for resources handled by the CRUD dispatcher. However, I really don't see a proper way to move forward with this.

@hanneshofmann
Copy link
Author

I understand that adding support for field selectors (and maybe other features as well) to the mock server might be really complex and limitless (and perhaps even harder to maintain..).

What do you think about to at least extend the documentation so that such limitations are listed there? Once documented, this issue could be closed accordingly.

@manusa
Copy link
Member

manusa commented Nov 8, 2021

What do you think about to at least extend the documentation so that such limitations are listed there? Once documented, this issue could be closed accordingly.

That'd be great :)

@stale
Copy link

stale bot commented Feb 27, 2022

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

@stale stale bot added the status/stale label Feb 27, 2022
@stale stale bot closed this as completed Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants