Use the new Repositories API in the UI

[antgamdia]

As the new API for managing Packages Repositories is becoming more stable, the current UI needs to be properly adapted in consequence.

Currently, we can test it with the Flux plugin, but there is ongoing work on both the Helm (#4662) and Carvel (#4681) plugins.

IMO, the new design should be carried out in multiple stages:

• Phase 0: Use the new Repositories API in the current UI #4817
  • Start implementing towards that direction (autogenerated apprepo grpc clients, start using new API calls, report issues back, improve the API if needed, etc)
  • Ensure feature parity between the new API and what Kubeapps already supports.
  • Ensure the implementation is consistent across plugins.
  • PRs:
    • Remove no longer used elements from AppRepoList  #4930
    • Revamp AppRepoForm #4939
    • Switch to the new Repos API #4954
    • Minor file renames from AppRepo to PkgRepo  #4955
• Phase 1:
  • Blocking issues:
  • Allow private global repos in the Helm plugin
    • Tracked at: Repos API/Helm - Support global private repos #5129
  • Re-enable e2e tests: Build E2E tests for the new package repository API UI #4876
    • Done at Add pending e2e tests for the repos UI #5115
    • Blocked by Repos API/Helm - Support global private repos #5129
  • Add a way to specify img pull secrets in the helm plugin for kubeapps-managed secrets. Currently, the UI supports both approaches, but the API only allows setting the secret manually. This has led to disabling the e2e test nº 03/10.
    • Tracked at: Specify imgPullSecrets in Helm's Repos API by its values, not only by secret name #5015
    • Done at:
      • Add imagePullSecrets option to repos API #5081
      • Add pullSecret management in repos UI #5109
  • Add the accessLevel (= type of auth) to the PackageRepositorySummary API + use it in the UI
    • Trivial, done at: Add credential info to repository summary #5003
  • The helm plugin doesn't have a status.ready
    • Trivial, done at: Add status helm repos API #5006
  • The auth type "Bearer" implementation is inconsistent across the plugins
    • Tracked at: [Package repositories] Review and fix backend and frontend handling of auth headers #5028
    • Done at: Make repos' bearer auth prefix-safe #5097
  • Improvements:
  • Clean up the unused IPackageRepositoryState and its actions.
    • Tracked at: Clean up the unused IPackageRepositoryState and its actions #5061
    • Done at: Clean up unused vars from the pkgRepo state #5062
  • Rename RepositoryCustomDetails to HelmRepositoryCustomDetails or RepositoryHelmCustomDetails. Idem with Carvel.
    • Tracked at: Rename RepositoryCustomDetails to something more specific #5016
    • Done at: Rename the repos API's custom detail name field  #5046
  • Delete url.apprepositories.get once we don't need it anymore. It also means we will be able to remove the endpoints from kubeops!
    • Tracked at: Remove apprepositories handling from Kubeops #5014

[antgamdia]

Phase 0:

I've been working these days on this thing, mostly ensuring that what we currently have is actually supported by the new API.

Just writing down some thoughts during the (still ongoing) implementation:

General

• No validation endpoint is present. Ideas?
  • Add it as a core /validate endpoint
  • Add the dryRun: true opt-in flag to the addRepo function?
  • Add a /validate as a per-plugin operation (not core)?
• Access level (private/public) not in the summaries response
  • Currently, we state whether or not a repo has some sort of auth in the repo list. I don't see any harm in adding that field to the summaries response, I think?
• Force resync operation is not available?
  • Maybe it has been already discussed, but just noticed there is no way to force an update/re-fetch/re-sync/whatever of a given repo.
• Packages should have a field for with PackageRepositoryReference
  • Not a short-term thing, but packages API ought to be updated to add a PackageRepositoryReference field, no?

Flux

• Add "description" as a flux CR metadata?
  • Already a todo in the code, but I guess it can be safely implemented as an annotation in the CR?

[antgamdia]

And also... a PoC of Flux repos being managed from our current UI :)

[ppbaena]

Wow! It looks nice 🎉

[absoludity]

Nice - looking great Antonio!

* No validation endpoint is present. Ideas?
  
  * Add it as a core /validate endpoint
  * Add the `dryRun`: true opt-in flag to the addRepo function?
  * Add a `/validate` as a per-plugin operation (not core)?

We chatted about this last week: what is the validate function currently doing? If it is just ensuring that an http endpoint can be reached with certain credentials, we could have it as a generic function in the resources plugin or similar, but I don't think it is - we updated the helm plugin validation to handle OCI repos as well (which did some OCI-related validation).

For 1-1 with the current api, I'd assume we'd want to add a validation endpoint per plugin, but not sure - what do you think?

* Access level (private/public) not in the summaries response
  
  * Currently, we state whether or not a repo has some sort of auth in the repo list. I don't see any harm in adding that field to the summaries response, I think?

I don't see a problem with that. Note: don't think we should conflate the presence of auth with private/public .

* Force resync operation is not available?       
  * Maybe it has been already discussed, but just noticed there is no way to force an update/re-fetch/re-sync/whatever of a given repo.

That's because it's not something that makes sense generally (eg. what would this mean for a Carvel repository? I'm not sure we can tell the carvel controller to resync, it may not even makes sense because the carvel repo version is meant to be frozen?). If we add this for the helm repository, it'll be an extra RPC added to the helm implementation (ie. not in the core) and any other plugin that supports it, but not sure we need to do that initially. WDYT?

* Packages should have a field for with PackageRepositoryReference
  
  * Not a short-term thing, but packages API ought to be updated to add a `PackageRepositoryReference` field, no?

Confused as to which context you think is missing this? For eg, the PackageRepositorySummary message includes a PackageRepositoryReference? Or maybe I've missed what you mean here.

[antgamdia]

We chatted about this last week: what is the validate function currently doing? If it is just ensuring that an http endpoint can be reached with certain credentials, we could have it as a generic function in the resources plugin or similar, but I don't think it is - we updated the helm plugin validation to handle OCI repos as well (which did some OCI-related validation).

For 1-1 with the current api, I'd assume we'd want to add a validation endpoint per plugin, but not sure - what do you think?

As of today, I've left the existing old endpoint + only invoking it if the plugin is helm. During the next days, I will replace it with what Rafa implemented in the Helm plugin natively.
Later on, we can just extend the validation with a per-plugin validation endpoint, as you mentioned.

Access level (private/public) not in the summaries response

I don't see a problem with that. Note: don't think we should conflate the presence of auth with private/public .

Great, I'll do in a folloup PR then. Adding it to this task's items.

That's because it's not something that makes sense generally (eg. what would this mean for a Carvel repository? I'm not sure we can tell the carvel controller to resync, it may not even makes sense because the carvel repo version is meant to be frozen?). If we add this for the helm repository, it'll be an extra RPC added to the helm implementation (ie. not in the core) and any other plugin that supports it, but not sure we need to do that initially. WDYT?

Mmm, fair point. I agree, this is not a required feature right now (in fact, I've disabled the resync buttons in the initial implementation I did. However, I still see this is something useful somehow. Let's suppose I have created a repo with a sync interval of 1 day. However, someone says a new package is available and I want to try it out asap. Inituitively, I'd look for a "sync" button to kind-of-override the 1-day interval and just trigger a re-fetch in this very moment that I need it.
Anyway, let's postpone any action as there are more urgent stuff to handle (handling secrets, mainly).

Confused as to which context you think is missing this? For eg, the PackageRepositorySummary message includes a PackageRepositoryReference? Or maybe I've missed what you mean here.

No, I meant that, currently, our packages API (like GetAvailablePackageSummary) is encoding the repo as part of the identifier (as in bitnami/wordpress --> repo=bitnami). What I was suggesting is adding a new field with the PackageRepositoryReference, but not for now.

[antgamdia]

I have some questions before going further on #4817:

Currently, the UI we have (and, we still want to maintain for the sake of this very first PR/version/phase) is like:

• Repo authorization: none, http-header auth, docker registry credentials (= user will specify the secret name)
• Image pull secrets: docker registry credentials (= user will specify the secret name)

In order to create the secret, it is the UI that handles the secret creation (see the "submit" button below)

I'm now trying to figure out the mapping between our current UX and the API fields. I've sketched up a mapping (sorry for the handwriting haha):

Let me explain it a little bit:

- `auth.header`: bearer token or any http-header-based authentication.
- `auth.passCredentials`: whether or not passing the credentials to 3rd party sites.
- `auth.usernamePassword`: QUESTION A
- `auth.secretRef`: QUESTION B
- `auth.tlsCertKey`: QUESTION C
- `tlsConfig: certAuthority`: our current "custo CA certificate"
- `tlsConfig: insecureSkipVerify`: our current skip tls verification
- `tlsConfig.secretRef`: QUESTION D

1. Question A: if user/password refers to an HTTP Basic Authentication (meaning header like "Basic "+base64(user+":"+password)), why would we need the usernamePassword object? Perhaps to make it easier for clients? Can we just ignore it and use the header field?
2. Question B: if it is the UI that is creating the secret, should we pass the secret name here? I mean, assuming there is a secret my-secret in the "associate docker registry credentials" dropdown AND user selects use docker registry credentials. Am I right?
   • Set auth.secretRef.key to my-secret
   • Set auth.secretRef.name to ???????:
3. Question C: auth.tlsCertKey always unset, we don't support it right now, do we?
4. Question D: tlsConfig.secretRef if user selects a secret in the use docker registry credentials dropdown, populate it as follows. Am I right?
   • Set tlsConfig.secretRef.key to my-secret
   • Set tlsConfig.secretRef.name to ???????:

In that scenario, I guess we are just supporting the user-managed secrets (see discussion on Slack), no?

[castelblanque]

Great job Antonio!

Question A (Basic Authentication)

Only advantage I see to having a separate data structure for u/p is that we will be able to show REDACTED only on the password and not the name. If we have u/p in the header, the whole header value will show REDACTED I guess. Leaving the UI user without knowing which user is being used for authentication. (I promise I will try to avoid the word "user" further in this comment when possible 😄 ). However, I'm fine with showing the whole header REDACTED.

Question B

The UI will not create the secret with Package repository API, it will be the backend. Here is where the UI could have some toggle for selecting whether if Kubeapps will manage secrets or not. See here.
I would rename the secret dropdown to be used for selecting any auth secret.
Meaning of the two fields in the secretRef object are (if I'm not mistaken):

• name -> secret name
• key -> key of the data map inside the secret. This can be inferred in the backend from the auth type. E.g. if auth is Docker credentials, key should be .dockerconfigjson. No need for the UI to send the key, I believe.

Question C auth.tlsCertKey

At least for Helm we do not support it AFAIK

Question D

If user selects Docker registry credentials secret, that would go under auth.secretRef, no?
The tlsConfig.secretRef is for the secret of the Custom CA? (see here)

IMHO in order to make the UI more useful and given that we are restricted to set the "kubeapps manages secrets" flag per installation by now, I would offer the "kubeapps manages secrets" mode by now. This is, no secrets picking from UI. In this way, the whole thing can be done from UI. WDYT?

[antgamdia]

A) Only advantage I see to having a separate data structure for u/p is that we will be able to show REDACTED only on the password and not the name. If we have u/p in the header, the whole header value will show REDACTED I guess. Leaving the UI user without knowing which user is being used for authentication.

Great, added to this issue to add it in a follow-up PR.

EDIT: I've just noticed the Helm plugin is really enforcing it (Username/Password configuration is missing), so will add the fix in the current PR then.

B) The UI will not create the secret with Package repository API, it will be the backend. Here is where the UI could have some toggle for selecting whether Kubeapps will manage secrets or not.

Ah, ok. Yep, I meant the Secret is being created by the resources api, but triggered by the UI. As opposed to the repos API creating it.
For my ongoing PR, I plan to leave it as is, so that we can re-think it and add the toggle for user/kubeapps-managed secrets.

C) auth.tlsCertKey at least for Helm we do not support it AFAIK

Great, thx.

D) If a user selects Docker registry credentials secret, that would go under auth.secretRef, no? The tlsConfig.secretRef is for the secret of the Custom CA?

Yep, you're totally right. Thanks for pointing it out!

IMHO in order to make the UI more useful and given that we are restricted to set the "kubeapps manages secrets" flag per installation by now, I would offer the "kubeapps manages secrets" mode by now. This is, no secrets picking from UI. In this way, the whole thing can be done from UI. WDYT?

+1 to this idea, but not right now. My ongoing PR has several changes and I'd prefer not to add any more disruptive changes here but in a follow-up PR. I mean, getting rid of the secret selector will have an inpact on the test suites :S

I'm going to polish up my PR and extract any pending TODO from it soon.

[castelblanque]

Thanks for the comments. Just a remark:

Yep, I meant the Secret is being created by the resources api, but triggered by the UI. As opposed as the repos API creating it.

With the new API, secrets are created inside the add/update repository operation (when "kubeapps manages secrets" mode). Not created by a previous API invocation to the resources API.

[absoludity]

Currently, the UI we have (and, we still want to maintain for the sake of this very first PR/version/phase) is like:

I don't think it will be possible to maintain the existing UI when switching to the new API. Because we already wanted to simplify the UX for app repositories, the API has been designed not to necessarily support an existing UX, but rather, to support.a simplified data exchange (kubeapps-controlled secrets etc.)

[antgamdia]

Ok, so perhaps a good approach is to leave the current PR as-is (current UI, falling tests) and, on top of it, start stacking the rest of the follow-up PRs removing the current logic for handling secrets manually and start fixing the test cases in this new "simplified" UI.
Thanks for your comments!

[antgamdia]

FTR (from #4361 (comment)):

This issue is a by-product of #3496 The core apis server now has support for RepositoriesService, which currently only has one API AddPackageRepository(). Only flux plugin currently implements the API. There will soon be more APIs defined for Reading, Updaing and Deleting of repositories So:

1. The dashboard needs to be refactored to start to use the new Repositories Service. We should start with 'Add...' and move on to other operations as they become available
2. the Helm plugin and carvel plugins need to implement the AddPackageRepository() API (including the custom field handling the plugin specific stuff)

[antgamdia]

I have extracted some of the longer-term items into a separate issue: #5121