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

[kubeapps-apis] Implement "direct-Helm" plugin based on the existing features in kubeapps #2993

Closed
9 tasks done
antgamdia opened this issue Jun 16, 2021 · 4 comments · Fixed by #3083 or #3099
Closed
9 tasks done
Assignees
Labels
component/apis-server Issue related to kubeapps api-server kind/feature An issue that reports a feature (approved) to be implemented
Projects
Milestone

Comments

@antgamdia
Copy link
Contributor

antgamdia commented Jun 16, 2021

In the same way that we already have kapp-controller and fluxv2 plugins (more or less) implemented, it is required to also support the legacy/direct/current Helm approach that kubeapps is using.
Otherwise, existing OCI support would get compromised (AFAIK) unless another plugin does support it.

Edit: FWIW, it will leverage the existing AppRepositories/postgresql logic

Related to #2023

Status

Transversal aspects

Operations already "agreed"

Operations in active discussion (not part of this task)

  • GetInstalledPackageSummaries
  • GetInstalledPackageDetail
  • CreateInstalledPackage
  • UpdateInstalledPackage
  • DeleteInstalledPackage

UI

Tracked at #3085

@antgamdia antgamdia created this issue from a note in Kubeapps (Next iteration discussion) Jun 16, 2021
@antgamdia antgamdia added component/apis-server Issue related to kubeapps api-server kind/feature An issue that reports a feature (approved) to be implemented priority/high labels Jun 16, 2021
@antgamdia antgamdia moved this from Next iteration discussion to Committed in Kubeapps Jun 17, 2021
@antgamdia antgamdia moved this from Committed to In progress in Kubeapps Jun 18, 2021
@antgamdia antgamdia self-assigned this Jun 18, 2021
@antgamdia
Copy link
Contributor Author

antgamdia commented Jun 29, 2021

Originally posted at #3022 (comment)

Additional fields for GetAvailablePackageSummaries

Currently, the client requires the following params for displaying the catalog: icon, name, repo, version, description, namespace, id

A response of this APIs is something like:

{
  "availablePackagesSummaries": [
    {
      "availablePackageRef": {
        "context": {
          "namespace": "kubeapps" namespace
        },
        "identifier": "marketplace/airflow" id
      },
      "latestVersion": "10.2.1", version
      "iconUrl": "https://bitnami.com/assets/stacks/airflow/img/airflow-stack-220x234.png", icon
      "displayName": "airflow", name
      "shortDescription": "Apache Airflow is a platform to programmatically author, schedule and monitor workflows." description
    }
  ]
};

So the only missing field is repo (it could eventually be retrieved using the identifier).

Pagination

Well, this is interesting. Currently, the API is returning something like:

{"data":[ charts ],"meta":{"totalPages": pages }}

Perhaps, we could modify the messages to use this data/meta approach. Or even go with some of these options:

  1. GitHub is using the Links header: https://docs.github.com/en/rest/guides/traversing-with-pagination
  2. Google suggests including the pagination as part of the proto msg: https://cloud.google.com/apis/design/design_patterns#list_pagination

@absoludity
Copy link
Contributor

absoludity commented Jun 30, 2021

So the only missing field is repo (it could eventually be retrieved using the identifier).

It could, but it wouldn't be ideal to request it for each item in a large page of results? I'd assume we'll eventually want this as an optional field on the summary (and detail), but note that it's only available for direct-helm because we cache this data while syncing repositories. kapp_controller, for example, will have no link from the package (or packagemetadata) back to the repository, afaict, which is OK. Right now I wonder if it's best to leave this until we define the package repository messages (including a package repository reference), so the UX when using the new API just won't include the repo until later.

Pagination

Well, this is interesting. Currently, the API is returning something like:

{"data":[ charts ],"meta":{"totalPages": pages }}

Perhaps, we could modify the messages to use this data/meta approach.

We already have something like this, in that the actual return data is in a field available_package_summaries (rather than data). Not sure why we'd need to put the pagination fields under meta, but maybe? Personally I'd rather follow the proto msg design pattern you link to below.

Or even go with some of these options:

1. GitHub is using the [`Links` header](https://datatracker.ietf.org/doc/html/rfc5988): https://docs.github.com/en/rest/guides/traversing-with-pagination

2. Google suggests including the pagination as part of the proto msg: https://cloud.google.com/apis/design/design_patterns#list_pagination

Yep, I see no reason why we wouldn't want to just follow the recommended design patterns where applicable. The fact that the page_token can be opaque may also give us a solution (later) for pagination on the aggregated API (in that we can most likely encode the page tokens for each individual plugin etc). Any objection?

EDIT: I've updated the GetAvailablePackageSummaries and GetInstallPackageSummaries request and responses to include this. See what you think.

@antgamdia
Copy link
Contributor Author

antgamdia commented Jul 1, 2021

I've updated the GetAvailablePackageSummaries and GetInstallPackageSummaries request and responses to include this. See what you think.

Awesome, I also think we should follow the recommended pattern here. Thanks for the changes you made in the design doc!

I'm now addressing the UI changes. Since it is a complex endeavor, let's break this task down into three different milestones:

  1. Proof of concept just changing the endpoints and data models used in the client (no feature flag yet). The outcome of this phase should be a list of requirements for the API responses. For instance: a missing field o some logic that should be addressed as part of the backed. It will allow other people to continue to work in parallel.
  2. Evaluate a major change in the API client to directly use gRPC w/ Typescript. It involves:
    • Check if the current proxying approach works for gRPC or if need to add a new http2 Nginx listener to proxy pass it.
      • Update: nginx cannot multiplex HTTP 1.1 and 2 without custom logic. I've tried adding a new location and using grpc_pass, but no success. I've ended up creating a new server { listen <another port> http2; ... (plus a new k8s svc port mapping) and it works. I'll go this way until we come up with any other idea.
    • Implement a proof of concept (take into account the pagination)
  3. Add a feature flag (or sth similar) for landing code quickly.

Raw issues while performing UI changes

Placeholder section for tracking (raw) the issues I'm finding; later on, I'll create a proper summary

  • Add repo to GetAvailablePackageSummaries and GetAvailablePackageDetails.

    • Status: to be considered as an optional field on the summary and detail.
    • Core API relationship: this field is not available for every plugin
    • ETA: until we define the package repository messages (including a package repository reference)
    • Reason: it is required for filtering by repo in the UI. This functionality is used in both the catalog view (as a filter) and the AppRepo view (as a link to the catalog).
  • Add latestAppVersion to GetAvailablePackageSummaries.

    • Status: TBD
    • Core API relationship: TBD
    • ETA: TBD
    • Reason: appears in the code, but checking why it is needed still pending.
  • Add category to GetAvailablePackageSummaries.

    • Status: TBD
    • Core API relationship: TBD
    • ETA: TBD
    • Reason: it is required for filtering by category in the UI. Is it really needed? It is sth that almost only Bitnami provides, no? Check if Carvel also offers (or will offer) categories.
  • Issue when querying a namespace in the API, 401: {"code":16,"message":"The current user has no access to the namespace \"default\""} Check tomorrow if it is really a problem with the can-I endpoint or is just a personal RBAC issue (though... I'm cluster.admin)

Current status

image

Thoughts for the Helm-less version

  • No category and repo filters whatsoever ?

  • Top filter by "plugin" that triggers package-specific filters like category/repo ?

    • Like this:
      image
  • ID/name collision between plugins, are we handling this no? Consider names like "aaa/bbb"

@absoludity
Copy link
Contributor

Accidentally closed with a Fixed tag in description.

@absoludity absoludity reopened this Jul 5, 2021
Kubeapps automation moved this from Done to In progress Jul 5, 2021
@project-bot project-bot bot moved this from In progress to Inbox in Kubeapps Jul 5, 2021
@absoludity absoludity moved this from Inbox to In progress in Kubeapps Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/apis-server Issue related to kubeapps api-server kind/feature An issue that reports a feature (approved) to be implemented
Projects
No open projects
Kubeapps
  
Done
3 participants