feat: Apply filterrule for /workspacekinds API - #1357
Conversation
Evaluate WORKSPACE_KIND-scoped filterRules against the request's namespaceFilter to compute per-WorkspaceKind hidden/restrictions and to omit api.hide'd kinds from the /workspacekinds response. Adds EvaluateWorkspaceFilterScopeRule and a purpose-built BuildEvalContextForImageAndPodCfg wrapper to the filterrules engine, wires evaluation through the model builder and repository, and adds unit tests at the engine, model, and repository layers. Part of kubeflow#847 Signed-off-by: Gang Li <ganglica@google.com>
google-oss-prow
Bot
added
area/v2
|
/cc @andyatmiami |
andyatmiami
left a comment
andyatmiami
left a comment
There was a problem hiding this comment.
@gangli113 - Another excellent/quality contribution...
Minor nits and clarifications on some aspects of code - but nothing truly concerning...
Another thing we need to pay attention to is the Slack thread I started in #kubeflow-notebooks as I realized the proposal/spec did not do a good job outlining expected behavior for namespaceFilter referencing a non-existent workspace (which then also probably could affect the namespace attribute in ListValuesRequest)
But feel free to address all other comments (and/or push back/discuss if you take issue with any comments I made).
I am comfortable proceeding to merge this PR if everything else gets resolved and the Slack thread doesn't have consensus - because we can always address that in a follow up PR.
Thanks!
- rename EvaluateWorkspaceFilterScopeRule to EvaluateWorkspaceKindFilterScopeRule - extract read-only scope sets to package-level vars - return 422 when namespaceFilter references a non-existent namespace - assert apiHide return values and add filterRules-defined handler tests - add EvaluateWorkspaceKindFilterScopeRule unit tests Part of kubeflow#847 Signed-off-by: Gang Li <ganglica@google.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@andyatmiami thanks for the review comments. I have resolved all your comments. Also changed the bechavior of resolveNamespace() that both ListValues and workspacekinds API can return error 422 when namespace doesn't exist in the cluster. |
andyatmiami
left a comment
andyatmiami
left a comment
There was a problem hiding this comment.
Hey @gangli113 - thanks for the quick turnaround on the last round of comments...
getting real close to having this merge ready - just a few more rough edges to polish.
in addition to the PR comments below - I did also notice something else we missed on last PR (i think) - hoping we can simply sneak it in here to knock it out (see below)
Missing return after serverErrorResponse in PodTemplateOptionsListValuesHandler (workspacekind_podtemplate_options_handler.go:144) — falls through to
dataResponse, writing two HTTP responses.
| if err := r.client.Get(ctx, client.ObjectKey{Name: namespaceName}, namespace); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return nil, nil | ||
| namespacePath := field.NewPath("namespace") |
There was a problem hiding this comment.
Validation error field path does not match either caller's user-facing parameter
resolveNamespaceLabels is shared by two callers with different user-facing parameter names:
GetWorkspaceKinds: the client sends query paramnamespaceFilter(handler validates format withfield.NewPath(constants.NamespaceFilterQueryParam)at line 114)ListPodTemplateOptionsValues: the client sendscontext.namespace.namein the request body
The hardcoded "namespace" matches neither. Clients receiving a 422 with field: "namespace" cannot identify which input was invalid.
Proposed Fix: Accept a *field.Path parameter so each caller passes the correct path:
func (r *WorkspaceKindRepository) resolveNamespaceLabels(ctx context.Context, namespaceName string, fieldPath *field.Path) (map[string]string, error) {GetWorkspaceKindspassesfield.NewPath(constants.NamespaceFilterQueryParam)ListPodTemplateOptionsValuespassesfield.NewPath("context", "namespace", "name")
| }) | ||
| }) | ||
|
|
||
| var _ = Describe("EvaluateWorkspaceFilterScopeRule", func() { |
There was a problem hiding this comment.
| var _ = Describe("EvaluateWorkspaceFilterScopeRule", func() { | |
| var _ = Describe("EvaluateWorkspaceKindFilterScopeRule", func() { |
3 participants
Closes #847
Summary
Implements compatibility selectors for the
/workspacekindsAPI (issue #847),building on the shared
filterrulesevaluation engine introduced in #846.When a
namespaceFilteris supplied, each WorkspaceKind'sspec.filterRules[]withscope: WORKSPACE_KINDare evaluated (first-match-wins) against the namespace's labelsto compute the per-WorkspaceKind
hiddenflag andrestrictions, and to omitapi.hide'd kinds from the response entirely.Changes
filterrulesengineEvaluateWorkspaceFilterScopeRule(wsk, namespaceLabels)to evaluateWORKSPACE_KIND-scoped rules against namespace labels.BuildEvalContextForImageAndPodCfg(...)wrapper for the/listvaluespath(
IMAGE_CONFIG+POD_CONFIGscopes) and makebuildEvalContextunexported.filterRulesByScopeso each endpoint only compiles the scopes it evaluates.NewWorkspaceKindModelFromWorkspaceKind)apiHide=true) whenapi.hidematches.hiddenwith theui.hideeffect (logical OR) and surfacerestrictionsfromapi.deny.GetWorkspaceKinds)namespaceFilterand omitapi.hide'd kinds.Behavior
namespaceFilter(admin listing)matchNamespacerules do not fire; admin-sethiddenpreserved, no denyui.hidematches namespacehidden = adminHidden || trueapi.hidematches namespaceapi.denymatches namespacerestrictionspopulated withdeny+denyMessageTesting
Added unit tests at three layers:
EvaluateWorkspaceFilterScopeRuleandBuildEvalContextForImageAndPodCfg.ui.hide/api.hide/api.denyviamatchNamespace, plus admin-listing.missing namespace).
All packages pass: