Watch specific resources rather than list of all gvrs. #3838
Conversation
| // resource namespace so some charts do not do so (ldap). We explicitly | ||
| // set the namespace for the resource ref so that it can be used as part | ||
| // of the key for the resource ref. At the moment we do not distinguish | ||
| // between cluster-scoped and namespace-scoped resources for the refs. |
There was a problem hiding this comment.
This came up while using the new plugin from the dashboard: the LDAP chart just happens to not specify the namespace metadata in the rendered templates of the helm manifest, which meant that all the resource refs were missing this info, which caused problems on two levels:
- The resource refs are meant to additionally check that a specific resource belongs to a package... if it does not include the namespace, it could be used to read a similarly named resource (even a secret) in a different namespace.
- In the dashboard code, we need to store resources with a unique key in the redux state, which of course needs to include the resource. This was what found the issue for me since the corresponding code which was pulling data out of the redux state wouldn't find it (since the original key did not have the namespace).
There was a problem hiding this comment.
So, just to confirm I'm getting it right: even if packages can generate cluster-scoped resources, we are not supporting it? That is, are we assuming a "no namespace" = "namespace where the pkg in installed" ?
If so, how are we retrieving those cluster-scoped resources? Perhaps it's just me missing something
There was a problem hiding this comment.
Cluster-scoped resources are fine, in that when fetching the referenced resource in the resources plugin, it checks whether the resource is namespaced via the k8s rest mapper and does the right thing (ie. it does not rely on the namespace to determine the scope)... rather than just relying on ref.Namespace (this is a change below).
The issue is that, as we're using the helm manifest to get the references, it is decided by the chart author, so you get (for example) both ClusterRoles with a metadata.namespace set (which, when applied, just results in the cluster-scoped resource) and Roles without a namespace (as Helm automatically applies the resources in the context of the release namespace). I'm just standardising here so the generated ResourceRef(erence) will always have the ResourceRef.Namespace set, regardless of the scope of the resource so that (a) we can define a unique key/identifier for a resource ref, and (b) the reference cannot be used to get/watch resources in other namespaces.
Ideally here we'd only be setting the namespace only when it is not set and the resource is a namespace-scoped resource, which is what I'm referring to in the last sentence of the above comment, but that means providing both plugins with a RESTMapper, which I didn't do here. I'll update that to an explicit TODO.
| // namespace/cluster scope information. Can be replaced in tests with a | ||
| // dummy version using the unsafe helpers while the real implementation | ||
| // queries the k8s API for a REST mapper. | ||
| kindToResource func(meta.RESTMapper, schema.GroupVersionKind) (schema.GroupVersionResource, meta.RESTScopeName, error) |
There was a problem hiding this comment.
This update was required to be able to get/watch resources correctly. The current code only works for namespaced resources as, without this extra info, it assumes a namespace scope.
| @@ -216,8 +220,17 @@ func (s *Server) GetResources(r *v1alpha1.GetResourcesRequest, stream v1alpha1.R | |||
| continue | |||
| } | |||
|
|
|||
| watcher, err := dynamicClient.Resource(gvr).Namespace(namespace).Watch(stream.Context(), metav1.ListOptions{}) | |||
| listOptions := metav1.ListOptions{ | |||
| FieldSelector: fmt.Sprintf("metadata.name=%s", ref.GetName()), | |||
There was a problem hiding this comment.
This is the only way I can see to watch a particular resource using the client-go's Watch.
| // resource namespace so some charts do not do so (ldap). We explicitly | ||
| // set the namespace for the resource ref so that it can be used as part | ||
| // of the key for the resource ref. At the moment we do not distinguish | ||
| // between cluster-scoped and namespace-scoped resources for the refs. |
There was a problem hiding this comment.
So, just to confirm I'm getting it right: even if packages can generate cluster-scoped resources, we are not supporting it? That is, are we assuming a "no namespace" = "namespace where the pkg in installed" ?
If so, how are we retrieving those cluster-scoped resources? Perhaps it's just me missing something
| if err != nil { | ||
| return status.Errorf(codes.Internal, "unable to map group-kind %v to resource: %s", gvk.GroupKind(), err.Error()) | ||
| } | ||
|
|
||
| if !r.GetWatch() { | ||
| var resource interface{} | ||
| if ref.Namespace != "" { | ||
| if scopeName == meta.RESTScopeNameNamespace { |
There was a problem hiding this comment.
Question: I guess scopeName refers to the namespace of the resource, but what
does meta.RESTScopeNameNamespace stand for?
There was a problem hiding this comment.
scopeName is either meta.RESTScopeNameNamespace or meta.RESTScopeNameRoot, both of which are defined in the meta package as string constants corresponding to "namespace" and "root" respectively.
Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
force-pushed
the
3779-required-apiserver-updates
branch
from
8533965
to
f8b8cf7
Compare
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson minelson@vmware.com
Description of the change
While testing the Dashboard using the new resources plugin to watch k8s resources, I found a few issues in the plugin itself.
The main one being that the watch request was not limited to a particular resource but rather all gvr's in the namespace. Only noticed because I had two deployments in the default namespace with different replicas and was seeing the wrong number of pods in the UI.
Other small changes which I'll comment on inline.
Benefits
Dashboard UX won't get confused when displaying k8s resources for an installed package (when I update to use the resources plugin in a following PR).
Possible drawbacks
Applicable issues
Additional information