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] Add direct-helm GetAvailablePackageSummaries #3022

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Jun 21, 2021

Description of the change

This PR just implements the GetAvailablePackageSummaries method in the direct-helm plugin.

AFAIK, this method just calls the assetsvc db, no Helm logic is required yet.

Test pending.

Benefits

We will have a brand new GetAvailablePackageSummaries operation up and running.

Possible drawbacks

N/A

Applicable issues

Additional information

If this approach is good, we should move some files under pkg No, we shouldn't; we better expose them directly from their current package.

@antgamdia antgamdia changed the title [kubeappaps-apis] Add direct-helm GetAvailablePackageSummaries [kubeapps-apis] Add direct-helm GetAvailablePackageSummaries Jun 22, 2021
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, realised this is draft after first comment, so stopped :)

Comment on lines +88 to +102
- name: POD_NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
- name: ASSET_SYNCER_DB_URL
value: {{ template "kubeapps.postgresql.fullname" . }}-headless:{{ default "5432" .Values.postgresql.service.port }}
- name: ASSET_SYNCER_DB_NAME
value: {{ .Values.postgresql.postgresqlDatabase }}
- name: ASSET_SYNCER_DB_USERNAME
value: postgres
- name: ASSET_SYNCER_DB_USERPASSWORD
valueFrom:
secretKeyRef:
key: postgresql-password
name: {{ include "kubeapps.postgresql.secretName" . }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should somehow indicate that this config should be temporary only (ie. we don't want to be providing such config for plugins) until we can switch to the caching provided to plugins that will eventually come out of #2946 . You've said something similar when pulling in the utils.go temporarily below.

Conflicts:
	chart/kubeapps/Chart.yaml
	cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go
	cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work Antonio. I think there's two things missing here:

  • existing checks to ensure the user has access to the namespace
  • correct handing of a request without the namespace

if request.Context.Cluster != "" {
return nil, status.Errorf(codes.Unimplemented, "Not supported yet: request.Context.Cluster: [%v]", request.Context.Cluster)
}
if request.Context.Namespace != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this condition is required? If it is empty (""), that's the resulting value the code wants, so couldn't this condition just be removed and just include the assignment from below?

Copy link
Contributor Author

@antgamdia antgamdia Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, just a leftover after copy-n-pasting from the kapp_controller plugin (there, it was initialized to namespace := globalPackagingNamespace instead of ""). Thanks for pointing this out.

Also, I'd like to re-check the `GetAvailablePackageSummariesp op in the kapp plugin, now I don't recall if we're properly using the no namespace vs "global" ns.

}
}

// TODO: add more filters in the context?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, FilterOptions will be part of the request message (in a later PR... I've just added this to the request message).


// TODO: add more filters in the context?
cq := ChartQuery{
// TODO(agamez): verify that not including a namespace means that we return everything you can read
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. We can later make this more specific (I'd added an IncludeOptions message originally with options for all_namespaces and global_items, but Dimitri doesn't think it's needed, so we agreed on the above: if a namespace is not included, then the request is for all available packages to which you have access (encapsulating both across all namespaces and global ones). It means there's (currently) know way to request just global available packages, but easy to add in if we find we do need it.

So currently, if namespace is empty, we're going to need to determine the list of namespaces to which the user has access, then query for the charts in those namespaces together with the kubeapps (global) namespace?

namespace: namespace,
}

// We are not returning paginated results here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// We are not returning paginated results here
// TODO: We are not yet returning paginated results here.

?

manager: manager,
clientGetter: getter,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clear tests :)

if request.Context.Namespace != "" {
namespace = request.Context.Namespace
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, I think we need to be checking the users credentials? That is, if a namespace is set, we need to ensure the user has access to that namespace (the existing code checks if the user can read secrets in the namespace). The existing code does this check as a decorator function in AuthGate that wraps the handlers.

If namespace is empty, then we'll need to know the namespaces to which the user has access, I think (diverging from the current assetsvc implementation, which assumes empty means global repos).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, good point here. Thanks for pointing to the auth code (I will comment on my code about it). I've added a check that will prevent users from retrieving packages from ns they can't read.
As you mentioned later, the ns=="" case won't be implemented yet.

}

func TestGetAvailablePackageSummaries(t *testing.T) {
getter := func(context.Context) (dynamic.Interface, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the k8s client is used currently, but it will be when we add a test ensuring that it 403s for namespaces to which the user doesn't have access, etc.

mock.ExpectQuery("^SELECT count(.+) FROM").
WillReturnRows(rowCount)

availablePackageSummaries, err := s.GetAvailablePackageSummaries(context.Background(), &corev1.GetAvailablePackageSummariesRequest{Context: &corev1.Context{}})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this is just testing only one case where the namespace is empty? We'd want to ensure that we test the cases:

  • specific namespace results a 403 if the user doesn't have access to the ns
  • specific namespace results in a query for charts in that ns only otherwise,
  • blank namespace results in a query for charts in the kubeapps namespace and any namespace to which the user has access

I think?

// TODO(agamez): logic temporaryly extracted from:
// /cmd/assetsvc/postgresql_utils.go
// /cmd/assetsvc/utils.go
// we can either: export this logic under pkg or re-implement the data access layer with an ORM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not ensure this functionality is exported in its current location? If that's an issue, keen to know why. If it's not an issue, I'd recommend just doing it as a separate, pre-quel branch, that is:

  • Public interface assetManager -> AssetManager
  • Public NewPGManager
  • Public methods

In fact, let me see if I can do that for you to save you the time: #3036 (if you agree, just land that and update).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for performing that changes. I initially opted to copy-n-paste the required logic just to have a clear idea of what was really necessary and what's not.
Also, I didn't want to mix things up, so I just thought, ok let's do it in another PR, but I didn't manage to do it yet, haha.

However, after yours, I've created this another PR #3040. More details there, but I had to move the files to a different folder and go package so that I was able to import them. Perhaps I'm missing something.

@absoludity
Copy link
Contributor

Great work Antonio. I think there's two things missing here:

* existing checks to ensure the user has access to the namespace

* correct handing of a request without the namespace

Actually, thinking more about the last point, perhaps it's enough to just return Unimplemented for now for GetAvailablePackageSummaries without a namespace, given that the current UI doesn't even do this (it was part of ensuring a future possibility with a different workflow). Thoughts?

@antgamdia
Copy link
Contributor Author

antgamdia commented Jun 25, 2021

Actually, thinking more about the last point, perhaps it's enough to just return Unimplemented for now for GetAvailablePackageSummaries without a namespace, given that the current UI doesn't even do this (it was part of ensuring a future possibility with a different workflow). Thoughts?

That's what I was about to say: let's replicate the current behavior as much as possible, so that we speed up the dev process. Later on, we can start thinking about implementing the TODOs.
I've pushed an initial version of the ns check using the existing logic for the can-i method.

Problem: the client we are passing back to the plugins (via clientGetter) is just for dynamic interfaces, That is, we can't query structured/typed data like a selfsubjectaccessreviews.
Here I've just implemented a temporary workaround, but, my question is: should we also pass to the plugin a normal k8s client besides the existing one?

btw: tests are still pending (I'll do it tomorrow)

Comment on lines 221 to 239
unstructuredData, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&authorizationapi.SelfSubjectAccessReview{
Spec: authorizationapi.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authorizationapi.ResourceAttributes{
Group: "",
Resource: "secrets",
Verb: "get",
Namespace: namespace,
},
},
})
if err != nil {
return false, fmt.Errorf("Error parsing the selfSubjectAccessReviews request: %s", err)
}
selfSubjectAccessReviewsResponse, err := client.Resource(selfSubjectAccessReviews).Create(ctx, &unstructured.Unstructured{Object: unstructuredData}, metav1.CreateOptions{})
if err != nil {
return false, fmt.Errorf("Error creating the selfSubjectAccessReviews request: %s", err)
}
res := &authorizationapi.SelfSubjectAccessReview{}
err = runtime.DefaultUnstructuredConverter.FromUnstructured(selfSubjectAccessReviewsResponse.Object, res)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I meant by temporary ugly workaround :P . I just needed to use the dynamic client, so I converted it with ToUnstructured and with FromUnstructured

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, see #3043 .

@@ -70,12 +70,21 @@ message GetAvailablePackageSummariesRequest {
// The context (cluster/namespace) for the request
Context context = 1;

// FilterOptions
//
// FilterOptions available when requesting summaries
message FilterOptions {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding these comments for a better swagger document.
When embedding these messages inside other msgs we often forgot to add the proper docs, that's why I tend not to embed them. But, on the other side, it's clearer to know which mgs they belong to, so... no strong opinion though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, and better to have those comments already if we later then move the embedded message out to re-use it. But again, if it's OK with you, I'll add those in the separate PR which hasn't landed yet which adds the filter options. It'll be easier if you don't merge that here nor add functionality depending on them in this PR, but rather keep this one focused on getting the available package summaries (securely, checking the namespace) without filter options. Just so we can work together without depending on unlanded branches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or if you want to update once it's landed and still include the filters, that's fine too, but just keen to avoid depending on other people's unlanded code unless it's necessary (ie. if this PR was dependent on filtering options, but I don't think it is).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah - so I just found the source of confusion here. Sorry, it was my fault. I'd thought I'd created a PR with the filter options yesterday... but it turns out I'd created the branch before our standup but didn't get back to push it and create the PR. I only found out because I just went to update my branch and pushed the changes to find that it created a new branch on GH. So from your POV you obviously weren't working on something that I'd done at all. Sorry for the confusion :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahah, I wasn't getting the point here; I added the FilterOptions in my branch so, as you said, I didn't see you were also working on that. It totally makes sense now.
I'll add the remaining changes to the filter (version and app version) here so that we have the whole filtering functionality implemented.

Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed an initial version of the ns check using the existing logic for the can-i method.

Problem: the client we are passing back to the plugins (via clientGetter) is just for dynamic interfaces, That is, we can't query structured/typed data like a selfsubjectaccessreviews.
Here I've just implemented a temporary workaround, but, my question is: should we also pass to the plugin a normal k8s client besides the existing one?

Yep. I've jumped in to do that separately for you in #3043 .

btw: tests are still pending (I'll do it tomorrow)

Yep, let me know when it's ready and I'll finish the review. Thanks!

@@ -56,3 +85,160 @@ func (s *Server) GetClient(ctx context.Context) (dynamic.Interface, error) {
}
return client, nil
}

// GetManager ensures a manager is available and uses it to return the client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// GetManager ensures a manager is available and uses it to return the client.
// GetManager ensures a manager is available and returns it.

// otherwise, first check if the user can access the requested ns
if namespace == "" {
// TODO(agamez): not including a namespace means that it returns everything a user can read
return nil, status.Errorf(codes.Unimplemented, "Not supported yet: not including a namespace means that it returns everything a user can read")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great.

if namespace == "" {
// TODO(agamez): not including a namespace means that it returns everything a user can read
return nil, status.Errorf(codes.Unimplemented, "Not supported yet: not including a namespace means that it returns everything a user can read")
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally avoid an unnecessary else and it's required indentation etc. (since the actual condition above returns, exiting the fn).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree (I'll change it). However, I decided to leave the else statement since, at some point, the if branch will have some logic. This way, fewer git changes (aka indentation) when we add the "no namespace" case.
Anyway, happy to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, up to you (I don't mind, just a general note for clarity).


// TODO(agamez): this is a temporary workaround since the client getter we (the plugin) get is just
// for dynamic interfaces. I guess we should also pass a normal k8s client in case the plugins want to
// interact with typed kubernetes resources.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a good point. We started with the dynamic client because otherwise we're dependent on the client-go version of 3rd party CRDs for packaging related queries. But for standard k8s resources it would be nice to have the typed client too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this in #3043 . Feel free to land that and update, or just review it and continue your PR without it (and I'll update later).

Comment on lines 221 to 239
unstructuredData, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&authorizationapi.SelfSubjectAccessReview{
Spec: authorizationapi.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authorizationapi.ResourceAttributes{
Group: "",
Resource: "secrets",
Verb: "get",
Namespace: namespace,
},
},
})
if err != nil {
return false, fmt.Errorf("Error parsing the selfSubjectAccessReviews request: %s", err)
}
selfSubjectAccessReviewsResponse, err := client.Resource(selfSubjectAccessReviews).Create(ctx, &unstructured.Unstructured{Object: unstructuredData}, metav1.CreateOptions{})
if err != nil {
return false, fmt.Errorf("Error creating the selfSubjectAccessReviews request: %s", err)
}
res := &authorizationapi.SelfSubjectAccessReview{}
err = runtime.DefaultUnstructuredConverter.FromUnstructured(selfSubjectAccessReviewsResponse.Object, res)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, see #3043 .

Conflicts:
	cmd/assetsvc/handler.go
	cmd/assetsvc/handler_test.go
	cmd/assetsvc/main.go
	cmd/assetsvc/pkg/assetsvc_utils/postgresql_utils.go
	cmd/assetsvc/pkg/assetsvc_utils/postgresql_utils_test.go
	cmd/assetsvc/pkg/assetsvc_utils/utils.go
	cmd/assetsvc/pkg/utils/postgresql_utils.go
	cmd/assetsvc/pkg/utils/postgresql_utils_test.go
	cmd/assetsvc/pkg/utils/utils.go
	cmd/assetsvc/postgres_db_test.go
	cmd/assetsvc/postgresql_utils.go
	cmd/assetsvc/postgresql_utils_test.go
	cmd/assetsvc/utils.go
	cmd/kubeapps-apis/gen/core/packages/v1alpha1/packages.pb.go
	cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go
@antgamdia antgamdia force-pushed the 2993-helmPlugin-GetAvailablePackageSummaries branch from 4582e07 to d278b12 Compare June 25, 2021 14:52
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, +1. Thanks Antonio. Is it worth implementing the pagination support in the request for this now? What else is required for this method to be used in the client?

expectedPackages: []*corev1.AvailablePackageSummary{},
statusCode: codes.Unauthenticated,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent :)

Namespace: namespace,
},
},
}, metav1.CreateOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much neater.

// Package version
//
// Package version for the request
string version = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think elsewhere we've stared using pkg_version to be specific together with app_version as you've used below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea indeed. I've added some comments in the doc for unifying the names here. Using just version could be misleading in some cases. I'd change each version by either pkg_ or app_ prefixes.

@antgamdia
Copy link
Contributor Author

antgamdia commented Jun 29, 2021

Is it worth implementing the pagination support in the request for this now? What else is required for this method to be used in the client?

Additional fields for this method

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

A typical response of the new API is something like this:

{
  "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

However, I'd rather land this PR, continue to work on the other operation and, later on, look into these two additional things in a separate PR. Specially pagination... we should come to an agreement before start changing the UI.
(I'm gonna crosspost it in the parent issue)

@antgamdia antgamdia merged commit 2d404ee into vmware-tanzu:master Jun 29, 2021
@antgamdia antgamdia deleted the 2993-helmPlugin-GetAvailablePackageSummaries branch June 29, 2021 16:20
@absoludity
Copy link
Contributor

However, I'd rather land this PR, continue to work on the other operation and, later on, look into these two additional things in a separate PR. Specially pagination... we should come to an agreement before start changing the UI.

Great, yes - I didn't mean to imply it should be in this PR, just trying to ensure we work on the most relevant things next.

(I'm gonna crosspost it in the parent issue)

Great, I'll reply there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants