fix: API requests with single RM and recover from panic during unauthorized autocreate #9573

amandavialva01 · 2024-06-26T15:22:22Z

Ticket

Description

This PR fixes the following issues:

API requests for single RM only work when the cluster name is specified in the request
Autocreate panic is not handled when attempted with OSS
Some lint issues after feature branch rebase with main
Autocreating namespace fails given workspace whose name doesn't match the accepted namespace regex pattern.

Test Plan

Spin up a single RM kubernetes EE cluster to execute the test plan.
To test API requests for single RM, run the following commands and make sure that they work:

det w create ws1 --auto-create-namespace
det w create ws2 && det w bindings set ws2 --namespace default
det w create ws3 --namespace default
det w create ws4 && det w bindings set ws4 --auto-create-namespace

To test auto-create panic handled gracefully with an intuitive error message:

Kill your Kubernetes cluster (if it's EE) and spin up an OSS Kubernetes cluster (remove license.txt and public.txt from your determined directory).
Run det w create ws10 --auto-create-namespace and verify that we get an error saying that auto create is an EE-only feature
Run det w create ws11 && det w bindings set ws11 --auto-create-namespace and verify that we get an error saying that auto create is an EE-only feature

To test namespace auto-creation with workspaces whose names don't match the accepted namespace regex pattern, run
det w create name,of_workspacE --auto-create-namespace and verify that a Kubernetes namespace is successfully created

Run det w create ANOTHER,name,of_workspacE && det w bindings set ANOTHER,name,of_workspacE --auto-create-namespace and verify that a Kubernetes namespace is successfully created

Checklist

Changes have been manually QA'd
New features have been approved by the corresponding PM
User-facing API changes have the "User-facing API Change" label
Release notes have been added as a separate file under docs/release-notes/
See Release Note for details.
Licenses have been included for new code which was copied and/or modified from any external code

netlify · 2024-06-26T15:22:40Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`82011dc`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/667c323121ae3f0008b78e4b
😎 Deploy Preview	https://deploy-preview-9573--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-06-26T15:22:59Z

Codecov Report

Attention: Patch coverage is 41.66667% with 14 lines in your changes missing coverage. Please review.

Project coverage is 51.75%. Comparing base (86214e5) to head (95a68f0).

Additional details and impacted files

@@                    Coverage Diff                     @@
##           wksp-namespace-binding    #9573      +/-   ##
==========================================================
- Coverage                   51.75%   51.75%   -0.01%     
==========================================================
  Files                        1255     1255              
  Lines                      154074   154076       +2     
  Branches                     3120     3120              
==========================================================
- Hits                        79747    79746       -1     
- Misses                      74172    74175       +3     
  Partials                      155      155

Flag	Coverage Δ
backend	`43.82% <69.23%> (-0.01%)`	⬇️
harness	`72.61% <9.09%> (-0.01%)`	⬇️
web	`49.15% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/internal/api_workspace.go	`59.07% <69.23%> (+0.25%)`	⬆️
harness/determined/cli/workspace.py	`16.50% <9.09%> (-0.32%)`	⬇️

... and 1 file with indirect coverage changes

tara-det-ai

LGTM

kkunapuli · 2024-07-10T19:34:26Z

harness/determined/cli/workspace.py

- ):
+ auto_create_namespace = args.auto_create_namespace or args.auto_create_namespace_all_clusters
+ set_namespace = args.namespace or auto_create_namespace
+ if args.cluster_name and not set_namespace:


I love this change; I think it's really easy to follow!

Nice work. It's easier to understand the intention!

kkunapuli · 2024-07-10T19:36:29Z

helm/charts/determined/templates/master-permissions.yaml

@@ -18,7 +18,7 @@ metadata:
 release: {{ .Release.Name }}
 rules:
 - apiGroups: [""]
- resources: ["pods", "pods/status", "pods/log", "configmaps", "namespaces"]
+ resources: ["pods", "pods/status", "pods/log", "configmaps"]


I'm just curious - does this give master permission to create the resources in the list?

Yea! The pods in the master deployment running the master service are authorized as the specified service account when accessing the Kubernetes API and have the permissions granted to the service account in the helm release, among those permissions being the ability to CRUD the listed resources!

Oh, awesome - thanks!!

kkunapuli · 2024-07-10T19:38:30Z

master/internal/api_workspace.go

- ClusterName: clusterName,
- Namespace: namespace,
+ namespaceMetaWithAllClusterNames[newClusterName] = &workspacev1.WorkspaceNamespaceMeta{
+ ClusterName: clusterName,


Should this be newClusterName instead of clusterName here? I guess I'm a little confused that there's a map where the key is newClusterName but the value has a field where ClusterName = clusterName.

Awesome catch! Yea it should be newClusterName. Thankss

kkunapuli

LGTM!

Minor suggestion: this PR fixes a couple bugs right? Are there any tests we could add to help confirm the issues are fixed and to ensure they don't pop up again? (If not, that's fine. You know the work better than I do!)

amandavialva01 · 2024-07-11T14:45:10Z

LGTM!

Minor suggestion: this PR fixes a couple bugs right? Are there any tests we could add to help confirm the issues are fixed and to ensure they don't pop up again? (If not, that's fine. You know the work better than I do!)
We are planning to do automated testing that covers the bugs fixed here in separate PRs since there are a lot of features to test! I have manually tested everything following the description instructions!

…orized autocreate (#9573)

amandavialva01 requested review from a team as code owners June 26, 2024 15:22

amandavialva01 requested review from jgongd, ShreyaLnuHpe and thiagodallacqua-hpe June 26, 2024 15:22

cla-bot bot added the cla-signed label Jun 26, 2024

amandavialva01 changed the base branch from main to wksp-namespace-binding June 26, 2024 15:22

determined-ci requested a review from a team June 26, 2024 15:22

amandavialva01 force-pushed the amanda/bugBashFixes branch 2 times, most recently from 71d5420 to 86f8657 Compare June 26, 2024 15:37

tara-det-ai approved these changes Jun 26, 2024

View reviewed changes

salonig23 force-pushed the wksp-namespace-binding branch 2 times, most recently from cd9b184 to 1ee0bbf Compare July 2, 2024 20:57

fix: singleRM optional cluster name and OSS error with auto-create

aee5f08

amandavialva01 force-pushed the amanda/bugBashFixes branch from 86f8657 to aee5f08 Compare July 10, 2024 19:19

lint fixes after feature branch rebase

22e1d3f

kkunapuli reviewed Jul 10, 2024

View reviewed changes

kkunapuli approved these changes Jul 10, 2024

View reviewed changes

jgongd approved these changes Jul 10, 2024

View reviewed changes

amandavialva01 force-pushed the amanda/bugBashFixes branch from 675e5ee to 0022747 Compare July 11, 2024 14:00

more lint fixes and API metadata correction

95a68f0

amandavialva01 force-pushed the amanda/bugBashFixes branch from 0022747 to 95a68f0 Compare July 11, 2024 14:03

amandavialva01 merged commit b4b1ed4 into wksp-namespace-binding Jul 11, 2024
78 of 92 checks passed

amandavialva01 deleted the amanda/bugBashFixes branch July 11, 2024 14:49

salonig23 pushed a commit that referenced this pull request Jul 23, 2024

fix: API requests with single RM and recover from panic during unauth…

0f465f7

…orized autocreate (#9573)

salonig23 pushed a commit that referenced this pull request Jul 24, 2024

fix: API requests with single RM and recover from panic during unauth…

7a197a8

…orized autocreate (#9573)

amandavialva01 added a commit that referenced this pull request Jul 24, 2024

fix: API requests with single RM and recover from panic during unauth…

126f7b9

…orized autocreate (#9573)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: API requests with single RM and recover from panic during unauthorized autocreate #9573

fix: API requests with single RM and recover from panic during unauthorized autocreate #9573

amandavialva01 commented Jun 26, 2024 •

edited

Loading

netlify bot commented Jun 26, 2024 •

edited

Loading

codecov bot commented Jun 26, 2024 •

edited

Loading

tara-det-ai left a comment

kkunapuli Jul 10, 2024

jgongd Jul 10, 2024

kkunapuli Jul 10, 2024

amandavialva01 Jul 10, 2024

kkunapuli Jul 10, 2024

kkunapuli Jul 10, 2024

amandavialva01 Jul 10, 2024

kkunapuli left a comment

amandavialva01 commented Jul 11, 2024

fix: API requests with single RM and recover from panic during unauthorized autocreate #9573

fix: API requests with single RM and recover from panic during unauthorized autocreate #9573

Conversation

amandavialva01 commented Jun 26, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Jun 26, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Jun 26, 2024 • edited Loading

Codecov Report

tara-det-ai left a comment

Choose a reason for hiding this comment

kkunapuli Jul 10, 2024

Choose a reason for hiding this comment

jgongd Jul 10, 2024

Choose a reason for hiding this comment

kkunapuli Jul 10, 2024

Choose a reason for hiding this comment

amandavialva01 Jul 10, 2024

Choose a reason for hiding this comment

kkunapuli Jul 10, 2024

Choose a reason for hiding this comment

kkunapuli Jul 10, 2024

Choose a reason for hiding this comment

amandavialva01 Jul 10, 2024

Choose a reason for hiding this comment

kkunapuli left a comment

Choose a reason for hiding this comment

amandavialva01 commented Jul 11, 2024

amandavialva01 commented Jun 26, 2024 •

edited

Loading

netlify bot commented Jun 26, 2024 •

edited

Loading

codecov bot commented Jun 26, 2024 •

edited

Loading