Allow search a manifest by label

[machi1990]

I've a manifest created via the restful API that looks like this

{
      "consumer_name":"cluster1",
      "created_at":"2024-05-29T16:08:13.719615Z",
      "href":"/api/maestro/v1/resources/dd54c3b7-a0af-401b-a77e-2c9faf5395f7",
      "id":"dd54c3b7-a0af-401b-a77e-2c9faf5395f7",
      "kind":"Resource",
      "manifest": {
        "apiVersion":"apps/v1",
        "kind":"Deployment",
        "metadata": {
          "name":"nginx",
          "namespace":"default"
        },
        "spec": {
          "replicas":1,
          "selector": {
            "matchLabels": {
              "app":"nginx"
            }
          },
          "template": {
            "metadata": {
              "labels": {
                "app":"nginx"
              }
            },
            "spec": {
              "containers": [
                {
                  "image":"nginxinc/nginx-unprivileged",
                  "name":"nginx"
                }
              ]
            }
          }
        }
      }
      ....

Listing for it without a search parameter works,

ocm get /api/maestro/v1/resources                                                                    
{
  "items": [
    {
      "consumer_name":"cluster1",
      "created_at":"2024-05-29T16:08:13.719615Z",
      "href":"/api/maestro/v1/resources/dd54c3b7-a0af-401b-a77e-2c9faf5395f7",
      "id":"dd54c3b7-a0af-401b-a77e-2c9faf5395f7",
      "kind":"Resource",
      "manifest": {
        "apiVersion":"apps/v1",
        "kind":"Deployment",
        "metadata": {
          "name":"nginx",
          "namespace":"default"
        },
        "spec": {
          "replicas":1,
          "selector": {
            "matchLabels": {
              "app":"nginx"
            }
          },
          "template": {
            "metadata": {
              "labels": {
                "app":"nginx"
              }
            },
            "spec": {
              "containers": [
                {
                  "image":"nginxinc/nginx-unprivileged",
                  "name":"nginx"
                }
              ]
            }
          }
        }
      },
      "name":"dd54c3b7-a0af-401b-a77e-2c9faf5395f7",
      "updated_at":"2024-05-29T16:08:13.719615Z",
      "version":1
    },
...

However, when I attempt to search for it using the label, I receive an error from Maestro

ocm get /api/maestro/v1/resources -p search="manifest->'data'->'manifest'->'metadata'->'labels'->>'foo' = 'bar'"

{
  "code":"maestro-21",
  "href":"/api/maestro/v1/errors/21",
  "id":"21",
  "kind":"Error",
  "operation_id":"2h9Ia9940SLICLxb6GtLDM1yIDR",
  "reason":"Failed to parse search query: manifest->'data'->'manifest'->'metadata'->'labels'->>'foo' = 'bar'"
}

My search query is manifest->'data'->'manifest'->'metadata'->'labels'->>'foo' = 'bar'. I've confirmed that the same query works directly in DB

make db/login

maestro=# select id from resources where manifest->'data'->'manifest'->'metadata'->'labels'->>'foo' = 'bar';
                  id                  
--------------------------------------
 e6963e1b-753b-4e80-b05b-1539a3ae030b

It might be good to allow jsonb like queries in the search argument.

[clyang82]

I did some investigation on it. Right now, we are using https://github.com/yaacov/tree-search-language to generate the sql based on the inputs. But it does not support jsonb query. It seems we need to build this feature by ourselves. but you know, there is potential security issue how to prevent SQL injection with arbitrary JSONB query string provided. That security issue we must consider.

In order to address this requirements, the simple solution is to use ocm get /api/maestro/v1/resources -p search="labels.foo = 'bar'" the code logic is to detect if the search starts with labels then we fill the whole jsonb query like manifest->'data'->'manifest'->'metadata'->'labels'->>. I know it is not as flexibly as possible. but it can be a quick solution we can do.

@machi1990 Do you have other cases to use jsonb query?

[machi1990]

Hi @clyang82 thank you for the reply. So far it could be just labels, but I'll need to check again and how we are going to interact with Maestro. I'll have a reply some time next week.

In order to address this requirements, the simple solution is to use ocm get /api/maestro/v1/resources -p search="labels.foo = 'bar'" the code logic is to detect if the search starts with labels then we fill the whole jsonb query like manifest->'data'->'manifest'->'metadata'->'labels'->>. I know it is not as flexibly as possible. but it can be a quick solution we can do.

I agree, this could be workable for labels. I'd like to be sure that the support will work seamless for both Single /api/maestro/v1/resources and Bundle /api/maestro/v1/manifestbundles resources once done in https://github.com/openshift-online/maestro/issues/103; Is this something that can be done once we've support to list manifests bundle as well?

[clyang82]

Yes. It can be done for the manifest bundles technically. But I want to know the use case, for example: list the bundles' contents can be https://github.com/openshift-online/maestro/issues/103#issuecomment-2138692977. Do you want to select one of resources which is matched your label, and then return this bundle?

[machi1990]

So far it could be just labels, but I'll need to check again and how we are going to interact with Maestro. I'll have a reply some time next week.

I've confirmed that so far it is only the labels that we are interested in searching for.

Yes. It can be done for the manifest bundles technically. But I want to know the use case, for example: list the bundles' contents can be https://github.com/openshift-online/maestro/issues/103#issuecomment-2138692977. Do you want to select one of resources which is matched your label, and then return this bundle?

That's one proposal. An alternative, which I think is rather flexible is having something like GET /api/maestro/v1/manifestbundles -p search="manifests[0].labels.foor='bar'" would be interesting i.e being able to search for label by targeting a manifest by label.

[clyang82]

Would you like to add labels into embedded manifests or would you like to add labels to the resource or resourcebundles directly?

[machi1990]

Hi @clyang82 the label used in the search are of the embedded manifest.

[clyang82]

After offline discussion, we agreed to firstly we implement the client side search. the manifest work client can pass the labelselector in the listoptions to search the manifestwork. after that, we will support search by labels on the restful api for both resource and resource-bundle.

[machi1990]

@clyang82 thank you for the update.

After offline discussion, we agreed to firstly we implement the client side search. the manifest work client can pass the labelselector in the listoptions to search the manifestwork

+1. Just a note on the implementation side, when doing client side search, please take pagination onto account when performing a list of maestro resources; by default the size is 100 IIRC. The manifest work client should be sure that it has performed its search based on label against the whole list of available resources and not just the partial page it has received on the first call.

As a general rule, the manifest work client has to take pagination onto account when listing resources via an API call cc @skeeey @qiujian16 as well

[clyang82]

This PR is trying to enhance to support jsonb function search by tree-search-language. but I think the tree-search-language is not designed to cover this kind of search (resources.payload -> 'data' -> 'manifests' @> '[{\"metadata\":{\"labels\":{\"foo\":\"bar\"}}}]'). so my proposal is to write the sql parser by ourselves. if the search includes ->> or @>, then it won't be validated and parsed by tree-search-language. instead, it will be validated by our own parser.

The valid inputs are

resources.payload -> 'data' -> 'manifest' -> 'metadata' -> 'labels' ->> 'foo'
resources.payload -> 'data' -> 'manifests' @> '[{"metadata":{"labels":{"foo":"bar"}}}]'

What do you think?

[skeeey]

so we only support label equals or exists operation, right?

[clyang82]

that is current requirement. we can enhance the sql parser to meet the further requirements if we have.

[machi1990]

Looking at the ListOptions, only matchingLabels i.e equality check is what we need. However, depending on the adoption for different operators or search cases via ListOptions. From the integrator perspective, I am happy to have a starting point which can help us validate the flow end to end, and progressively rollout support for other operators.