fix(frontend): prevent unintended deletion of shared ConfigMaps and Secrets during notebook deletion #3539

Gkrumbach07 · 2024-12-03T22:42:27Z

https://issues.redhat.com/browse/RHOAIENG-16192

fixes #3521 (we will follow up with more flexibility in the future)

Description

This PR addresses an issue where deleting a notebook would unintentionally delete ConfigMaps and Secrets that might be shared between multiple workbenches. The fix introduces prefix-based filtering to ensure only dashboard-generated resources are deleted.

Modified deletion logic to only delete resources that start with these prefixes
Updated tests to verify that custom resources are preserved

How Has This Been Tested?

Test Impact

updated delete test

create test resources

# Create a shared ConfigMap
kind: ConfigMap
apiVersion: v1
metadata:
  name: shared-test-configmap
  labels:
    opendatahub.io/dashboard: 'true'
data:
  STAGE: test

# Create a shared Secret
kind: Secret
apiVersion: v1
metadata:
  name: shared-test-secret
  labels:
    opendatahub.io/dashboard: 'true'
data:
  USER_PW: bXlzYW1wbGVwYXNzd29yZDM0
type: Opaque

Create Notebook with secret and env var
Link manually created resources to notebook (shared-test-secret, shared-test-configmap)

   envFrom:
            # Dashboard-generated (will be deleted with notebook)
            - secretRef:
                name: secret-abc123
            - configMapRef:
                name: configmap-xyz789
            
            # Shared/Custom resources (will NOT be deleted)
            - secretRef:
                name: shared-test-secret
            - configMapRef:
                name: shared-test-configmap

Test Deletion
- Delete notebook-1 through the dashboard
- Verify that:
  - notebook-1 is deleted
  - The shared ConfigMap and Secret remain

Expected Results

Only resources prefixed with secret- or configmap- should be deleted
Shared resources without these prefixes should remain intact
Second notebook should continue to function with shared resources

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

…ionality in tests

codecov · 2024-12-03T23:31:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.57%. Comparing base (e4bac88) to head (477b6a5).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3539    +/-   ##
========================================
  Coverage   85.56%   85.57%            
========================================
  Files        1342     1371    +29     
  Lines       31016    31201   +185     
  Branches     8675     8733    +58     
========================================
+ Hits        26538    26699   +161     
- Misses       4478     4502    +24

Files with missing lines	Coverage Δ
frontend/src/api/k8s/configMaps.ts	`95.00% <100.00%> (+1.25%)`	⬆️
frontend/src/api/k8s/secrets.ts	`96.36% <100.00%> (+0.28%)`	⬆️
...rc/pages/projects/notebook/DeleteNotebookModal.tsx	`90.62% <100.00%> (+7.29%)`	⬆️

... and 65 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4bac88...477b6a5. Read the comment docs.

frontend/src/api/k8s/secrets.ts

shalberd · 2024-12-04T07:47:35Z

frontend/src/pages/projects/notebook/DeleteNotebookModal.tsx

-          .filter((envName): envName is ConfigMapRef => !!envName.configMapRef)
+          .filter(
+            (envName): envName is ConfigMapRef =>
+              !!envName.configMapRef && envName.configMapRef.name.startsWith(CONFIGMAP_PREFIX),


I would not filter by a newly-created configmap or secret name prefix.
Instead, I would suggest by the odh-universal label kex opendatahub.io/managed (DATA_CONNECTION_AWS) which I think you call
https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/const.ts#L5 DATA_CONNECTION_AWS, but it is in fact just indicating that the secret or configmap are managed by opendatahub.

That label would be the ideal one to go for in filtering. Because we only have the name in envFrom sections, i.e. secretRef.name, we'd need a helper .ts function to get the details of that secret and configmap, if possible.
That function could be used later on in the filtering / selection configmap UI as well when selecting configmaps and secrets from the namespace not already assigned to envFrom that have label opendatahub.io/dashboard.
Later on, that helper function could also be used to add a property to ConfigmapKind and SecretKind instances setting "manyNotebooks": true if truly referenced in multiple workbenches / notebooks, determining of which of course would require another function ...

Regarding api call volume / frequency: configmaps and secrets per namespace, not created by dashboard, are not changing frequently. Corporate CICD, ArgoCD apply, and so on.
So, any kind of functions / determination could possibly happen at dashboard startup only, similar to the storageClasses info determination at cluster-level.

My main argument against prefix-based filtering remains: how can we assume non-dashboard, CICD type external processes do not create secrets that have configmap or secret not as a postfix, but as a prefix, then, they would still be deleted, would they not?

If we stay the way it is proposed here currently, it would need to be documented in arhcitecture decision records https://github.com/opendatahub-io/architecture-decision-records/tree/main/documentation

My main argument against prefix-based filtering remains: how can we assume non-dashboard, CICD type external processes do not create secrets that have configmap or secret not as a postfix, but as a prefix, then, they would still be deleted, would they not?

yep and that is the biggest issue here.

for api call volume we would be fetching for every resource in env from which may be small yes, but could be huge.

It could be two-pronged possibly: slow-changing, non-ODH created secrets and configmaps and their info like labels only fetched on first start of dashboard ... and dashboard secrets only fetched by name live, in terms of call volume ...
But then again, that might not be optimal either. Plus, then the logic in dashboard would be more complex, too. I see what you mean by call volume to the API of openshift ...

andrewballantyne · 2024-12-04T17:32:55Z

frontend/src/pages/projects/notebook/DeleteNotebookModal.tsx

-          .filter((envName): envName is SecretRef => !!envName.secretRef)
+          .filter(
+            (envName): envName is SecretRef =>
+              !!envName.secretRef && envName.secretRef.name.startsWith(SECRET_PREFIX),


Lets make sure we also filter for the fixed length of characters (which defaults to 6 today)

See DSPA secret logic

sure can do that too

yeah, good idea. Anyways, I am fine with this intermediate solution. Anything further would need to go into design docs and decision records architecture.

… and secrets in DeleteNotebookModal

andrewballantyne · 2024-12-04T19:58:03Z

frontend/src/api/k8s/configMaps.ts

@@ -18,7 +20,7 @@ export const assembleConfigMap = (
  apiVersion: 'v1',
  kind: 'ConfigMap',
  metadata: {
-    name: configMapName || `configmap-${genRandomChars()}`,
+    name: configMapName || `${CONFIGMAP_PREFIX}${genRandomChars()}`,


Can we tidy this a bit up...

getGeneratedConfigMapName() isGeneratedConfigMapName(name)

Do the same for the secret as well.

andrewballantyne · 2024-12-04T20:00:43Z

frontend/src/pages/projects/notebook/DeleteNotebookModal.tsx

+          .filter(
+            (envName): envName is ConfigMapRef =>
+              !!envName.configMapRef &&
+              Boolean(envName.configMapRef.name.match(new RegExp(`^${CONFIGMAP_PREFIX}.{6}$`))),


.{6} in regexp can be any 6 characters... they can be all dashes but the last character and still be valid k8s name but not what we are looking for.

The DSPA secret looks for [a-z0-9]{6} -- I think that is more accurate of a check.

shalberd · 2024-12-05T08:05:26Z

fixes must-parts (not nice-to-have) parts of
@Gkrumbach07 can you put this line in the main body of the PR description, please? This PR here at least partially fixes the issue I described in #3521

fixes #3521

thanks for all your work. @andrewballantyne do we went to create a follow-up ticket to https://issues.redhat.com/browse/RHOAIENG-16192 or will we just keep it open longer for tracking purposes?

frontend/src/api/k8s/secrets.ts

andrewballantyne · 2024-12-05T13:18:05Z

@shalberd, @Gkrumbach07 will be logging another ticket momentarily... we discussed it yesterday.

andrewballantyne

/hold

Fix the secret name, it's getting kinda bonkers how the code is written and we should try to clean that up imo. But otherwise tested and looks good to me.

Gkrumbach07 · 2024-12-05T16:33:01Z

follow up jira: https://issues.redhat.com/browse/RHOAIENG-16608

… clarity and maintainability

DaoDaoNoCode

left one small comment, it lgtm overall.
/lgtm

DaoDaoNoCode · 2024-12-05T19:19:09Z

frontend/src/api/k8s/configMaps.ts

+
+export const getGeneratedConfigMapName = (): string => `${CONFIGMAP_PREFIX}${genRandomChars()}`;
+export const isGeneratedConfigMapName = (name: string): boolean =>
+  Boolean(name.match(new RegExp(`^${CONFIGMAP_PREFIX}[a-z0-9]{6}$`)));


Though that might be the same result, just curious, would it be better to use new RegExp(`^${CONFIGMAP_PREFIX}[a-z0-9]{6}$`).test(name) instead of forcing to cast the type to boolean here? Same question to the other place.

yeah its cleaner to use test. i made the switch can you readd the lgtm

DaoDaoNoCode · 2024-12-05T20:37:50Z

/lgtm

andrewballantyne

/unhold

openshift-ci · 2024-12-05T20:40:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, DaoDaoNoCode

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Refactor: Update resource naming conventions and enhance delete funct…

8e23755

…ionality in tests

openshift-ci bot requested review from dpanshug and pnaik1 December 3, 2024 22:42

shalberd reviewed Dec 4, 2024

View reviewed changes

frontend/src/api/k8s/secrets.ts Show resolved Hide resolved

shalberd reviewed Dec 4, 2024

View reviewed changes

andrewballantyne requested changes Dec 4, 2024

View reviewed changes

openshift-ci bot assigned andrewballantyne Dec 4, 2024

Update resource names in tests and enhance validation for config maps…

f7c5613

… and secrets in DeleteNotebookModal

andrewballantyne requested changes Dec 4, 2024

View reviewed changes

clean up usages

8bc3b12

andrewballantyne reviewed Dec 5, 2024

View reviewed changes

frontend/src/api/k8s/secrets.ts Outdated Show resolved Hide resolved

andrewballantyne approved these changes Dec 5, 2024

View reviewed changes

openshift-ci bot added do-not-merge/hold This PR is hold for some reason lgtm approved labels Dec 5, 2024

Refactor secret name assignment in assembleSecret function for clarity

f72b4a5

openshift-ci bot removed the lgtm label Dec 5, 2024

Refactor secret name assignment in assembleSecret function to improve…

ca2c963

… clarity and maintainability

DaoDaoNoCode approved these changes Dec 5, 2024

View reviewed changes

openshift-ci bot assigned DaoDaoNoCode Dec 5, 2024

openshift-ci bot added the lgtm label Dec 5, 2024

small code format change

477b6a5

openshift-ci bot removed the lgtm label Dec 5, 2024

openshift-ci bot added the lgtm label Dec 5, 2024

andrewballantyne approved these changes Dec 5, 2024

View reviewed changes

openshift-ci bot removed the do-not-merge/hold This PR is hold for some reason label Dec 5, 2024

openshift-merge-bot bot merged commit 4d9e054 into opendatahub-io:main Dec 5, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(frontend): prevent unintended deletion of shared ConfigMaps and Secrets during notebook deletion #3539

fix(frontend): prevent unintended deletion of shared ConfigMaps and Secrets during notebook deletion #3539

Gkrumbach07 commented Dec 3, 2024 •

edited by andrewballantyne

Loading

codecov bot commented Dec 3, 2024 •

edited

Loading

shalberd Dec 4, 2024 •

edited

Loading

Gkrumbach07 Dec 4, 2024

shalberd Dec 4, 2024

andrewballantyne Dec 4, 2024

Gkrumbach07 Dec 4, 2024

shalberd Dec 4, 2024 •

edited

Loading

andrewballantyne Dec 4, 2024

andrewballantyne Dec 4, 2024

shalberd commented Dec 5, 2024 •

edited

Loading

andrewballantyne commented Dec 5, 2024

andrewballantyne left a comment

Gkrumbach07 commented Dec 5, 2024

DaoDaoNoCode left a comment

DaoDaoNoCode Dec 5, 2024 •

edited

Loading

Gkrumbach07 Dec 5, 2024

DaoDaoNoCode commented Dec 5, 2024

andrewballantyne left a comment

openshift-ci bot commented Dec 5, 2024

fix(frontend): prevent unintended deletion of shared ConfigMaps and Secrets during notebook deletion #3539

fix(frontend): prevent unintended deletion of shared ConfigMaps and Secrets during notebook deletion #3539

Conversation

Gkrumbach07 commented Dec 3, 2024 • edited by andrewballantyne Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

codecov bot commented Dec 3, 2024 • edited Loading

Codecov Report

shalberd Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Gkrumbach07 Dec 4, 2024

Choose a reason for hiding this comment

shalberd Dec 4, 2024

Choose a reason for hiding this comment

andrewballantyne Dec 4, 2024

Choose a reason for hiding this comment

Gkrumbach07 Dec 4, 2024

Choose a reason for hiding this comment

shalberd Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

andrewballantyne Dec 4, 2024

Choose a reason for hiding this comment

andrewballantyne Dec 4, 2024

Choose a reason for hiding this comment

shalberd commented Dec 5, 2024 • edited Loading

andrewballantyne commented Dec 5, 2024

andrewballantyne left a comment

Choose a reason for hiding this comment

Gkrumbach07 commented Dec 5, 2024

DaoDaoNoCode left a comment

Choose a reason for hiding this comment

DaoDaoNoCode Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Gkrumbach07 Dec 5, 2024

Choose a reason for hiding this comment

DaoDaoNoCode commented Dec 5, 2024

andrewballantyne left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 5, 2024

Gkrumbach07 commented Dec 3, 2024 •

edited by andrewballantyne

Loading

codecov bot commented Dec 3, 2024 •

edited

Loading

shalberd Dec 4, 2024 •

edited

Loading

shalberd Dec 4, 2024 •

edited

Loading

shalberd commented Dec 5, 2024 •

edited

Loading

DaoDaoNoCode Dec 5, 2024 •

edited

Loading