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

Design and implement GetK8sResourcesForInstalledPackage #3403

Closed
ppbaena opened this issue Sep 13, 2021 · 7 comments
Closed

Design and implement GetK8sResourcesForInstalledPackage #3403

ppbaena opened this issue Sep 13, 2021 · 7 comments
Assignees
Labels
component/apis-server Issue related to kubeapps api-server kind/enhancement An issue that reports an enhancement for an implemented feature

Comments

@ppbaena
Copy link
Collaborator

ppbaena commented Sep 13, 2021

Currently when an app is installed on the cluster, our AppView gets the data about the related k8s resources from the k8s api server.

We want to replace that with an API call which allows getting the (relevant) resources for a particular installed package (only).

This will be a huge step forward in removing the requirement to expose the k8s api server to the dashboard.

@absoludity absoludity changed the title Returning the k8s resources in the GetInstalledPackageDetails Design and implement GetK8sResourcesForInstalledPackage Sep 13, 2021
@antgamdia antgamdia added component/apis-server Issue related to kubeapps api-server kind/enhancement An issue that reports an enhancement for an implemented feature priority/high labels Sep 13, 2021
@ppbaena ppbaena added this to the Kubernetes API service milestone Sep 23, 2021
@antgamdia
Copy link
Contributor

In order to avoid a never-ending issue, let's move some of the remaining open items from #3340 to this issue:

Updated on September 28th

@absoludity
Copy link
Contributor

I've been working through the existing code to get a good idea how things are currently working.

Currently,

  1. in the AppView component, the action actions.apps.getApp is triggered which,
  2. hits kubeops to retrieve the specific helm release, including the yaml manifest.
  3. the app.manifest yaml is parsed by the view component into a list of resources (IResource, which uses any for the .spec), ignoring List resources (anything without a .kind property).
  4. the parsed resources are then iterated to produce a list of IResourceRefs that are stored as part of the component state, partitioned into deployments, statefulsets, deamonsets, services, ingresses, secrets and other.
  5. Another effect, dependent on the resourceRefs, then dispatches getAndWatchResource for each ref (?! This confuses me... we don't need to be watching secrets, config maps etc., or even getting them. Also, seems odd to be always watching, regardless of the state, though perhaps we want to catch if a pod changes from a stable state to something broken).

At that point, the dashboard client is getting (and watching/polling?) all resources for installation.

@absoludity
Copy link
Contributor

absoludity commented Oct 8, 2021

What I'd like to propose is the following:

New Packages method: GetInstalledPackageResourceRefs(installed_package_ref)

We could also just include the list of resource refs with the installed package detail, but given it's not specifically part of the installed package (nor needed unless wanting to display info about the k8s resources) I think it makes more sense as a separate request.

This method saves the client from dealing with package specifics too (so for helm, the client won't need to be fetching the release and parsing yaml resources to build the list of resource refs).

It may be worth even returning the partitioned refs for standard resources (deployments, pods, statefulsets, daemonsets, cronjobs, etc.), with other resources like the current model. (Edit: After more thought, the partitioning is a client decision)

The ref type itself would be something between the kubernetes ObjectReference (too many fields and as the docstring there says, ambiguous) and the LocalObjectReference.

New K8s methods: GetResource(installed_pkg_ref, resource_ref) and WatchResource(installed_pkg_ref, resource_ref)

Importantly, these methods only return (or stream) the resource iff the referenced resource belongs to the installed application according to the specific plugin (and the user has sufficient RBAC of course). That is our API allows authenticated fetching the resources for an installed package only.

This method will require a further packages method implemented for each plugin, VerifyInstalledPackageResource(installed_pkg_ref, resource_ref). (Edit: it does not, as instead it can just get the package refs from the plugin and verify that the requested ones are a subset).

I'd also like to investigate instead doing a plural for GetResources so that the web client can use a single request rather than N, while the backend does the harder work.

Given that the current frontend partitions the resources into a few core objects (secrets, pods, deployments, etc.), we could do that partitioning and return a strongly typed response object (using Kubernetes own proto definitions), if that's sane. Otherwise I'll investigate a generic resource with string/byte spec and status fields as per the existing implementation for IResource.

So current status is that I plan to do some investigative work to check the above, see what's possible. Let me know if anything sticks out or needs adjusting.

@absoludity
Copy link
Contributor

After successfully testing out some ideas to enable a separate "resources" plugin with a single streaming endpoint for watching multiple/many resources, I've added a section in the design doc for the GetK8sResources endpoint, as well as updating the core packaging API with an extra endpoint for GetInstalledPackageResourceRefs (which is specific for each packaging backend).

If you can take a look when you have a chance, that'd be great. (cc @gfichtenholt as I don't think you're subscribed to all issue email :) ).

@gfichtenholt
Copy link
Contributor

I will take a close look when I get a chance. How soon do you need it? Are you ready to starting adding code for this and and just waiting for me to review? Right now I am have like 3 non-trivial things on my plate that needed to be done yesterday :-) But I will drop everything, as usual, to unblock you as soon as you need it

Just a couple of superficial comments based on cursory examination:

  1. you need to update InstalledPackageDetail to fix the TODO
  2. don't understand why you called a section at the end "Other plugins..." as if we'd have plug-ins other than flux or helm. Also, this statement "All plugins that implement the core.packages interface will include a GetInstalledPackageResourceRefs method, returning the list of resource references for the resources created by the installed package. Getting or watching those resources does not need to be implemented separately by each packaging plugin and is instead provided by a separate resources plugin" makes it sound like we are about to add some more plug-ins. Are we? Just confusing, that's all. Probably need to discuss in a meeting for me to understand
  3. confused by "plugins.resources.v1alpha1". Why is there nothing in the core to aggregate like for packaging stuff?

@absoludity
Copy link
Contributor

absoludity commented Nov 3, 2021

But I will drop everything, as usual, to unblock you as soon as you need it

Thanks Greg, but no need to drop anything. I only CC'd you in case you're interested, but I can go through this with Antonio early next week too. Or if you're interested, we can chat through it on a call, but no problem if you're busy.

Just a couple of superficial comments based on cursory examination:

1. you need to update InstalledPackageDetail to fix the TODO

Will do - just focused on documenting what I've investigated and am planning, I'll go through and cleanup the TODO tomorrow.

2. don't understand why you called a section at the end "Other plugins..." as if we'd have plug-ins other than flux or helm.

Flux, helm (and kapp-controller) are packaging plugins, but the apis server has been designed so we can support other types of plugins too. As seems to be the case for this work.

Also, this statement "All plugins that implement the core.packages interface will include a GetInstalledPackageResourceRefs method, returning the list of resource references for the resources created by the installed package. Getting or watching those resources does not need to be implemented separately by each packaging plugin and is instead provided by a separate resources plugin" makes it sound like we are about to add some more plug-ins.

Yes, the functionality of getting and watching kubernetes resources seems to be a good example of another plugin. We don't need (or want) each packaging plugin (helm, flux, etc.) to re-implement this functionality. I tested it and am able to add this functionality as a separate plugin which works nicely, and other uses of the apis-server (outside of kubeapps) can choose whether they want to enable this plugin or not.

Are we? Just confusing, that's all. Probably need to discuss in a meeting for me to understand

Yep, let's try to do that. I'll ping you on slack to see when you've time, but again, no problem if you're busy.

3. confused by "plugins.resources.v1alpha1". Why is there nothing in the core to aggregate like for packaging stuff?

Because it's not functionality that needs to be (or should be) implement by each packaging plugin nor be aggregated by the core API. Let's chat about that on the call.

@absoludity
Copy link
Contributor

Moving to Done as the design is now implemented, and opening a separate issue for the dashboard changes to use the new API.

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/enhancement An issue that reports an enhancement for an implemented feature
Projects
None yet
Development

No branches or pull requests

4 participants