Skip to content

Use configmap helm driver instead of secrets #3272

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.

Sign up for GitHub

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

Jump to bottom
Merged
fluid-e2e-bot merged 9 commits into from
Jun 27, 2023
Merged

Use configmap helm driver instead of secrets #3272

fluid-e2e-bot merged 9 commits into from
Jun 27, 2023

Conversation

@TrafalgarZZZ
Copy link
Member

@TrafalgarZZZ TrafalgarZZZ commented Jun 6, 2023 •
edited
Loading

Ⅰ. Describe what this PR does

Fluid now use the default HELM_DRIVER(i.e. secret) to install/uninstall runtime charts which means Fluid controllers must have full permission to operate secret resources. This may incur potential security issue because secrets usually store sensitve user-applied data.

This PR makes the following changes to the code:

  • Default HELM_DRIVER to configmap instead of secret
  • For secrets, remove unnecessary permission like list/watch by skip caching secrets in informers.

IMPORTANT: BREAKING CHANGE NOTE
By migrating HELM_DRIVER from secret to configmap, it is possible that some resources may not be cleared after upgrade to the latest Fluid version. It is highly recommended to delete all the datasets and runtimes before upgrading to the version.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@fluid-e2e-bot
Copy link

fluid-e2e-bot bot commented Jun 6, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@codecov
Copy link

codecov bot commented Jun 6, 2023 •
edited
Loading

Codecov Report

Merging #3272 (e364a1b) into master (7411af5) will not change coverage.
The diff coverage is n/a.

❗ Current head e364a1b differs from pull request most recent head 58038d8. Consider uploading reports for the commit 58038d8 to get more accurate results

@@           Coverage Diff           @@
##           master    #3272   +/-   ##
=======================================
  Coverage   65.51%   65.51%           
=======================================
  Files         399      399           
  Lines       23198    23198           
=======================================
  Hits        15198    15198           
  Misses       6215     6215           
  Partials     1785     1785

cheyang
cheyang reviewed Jun 7, 2023
View reviewed changes
charts/fluid/fluid/templates/role/alluxio/rbac.yaml
resources:
- secrets
verbs:
- list
Copy link
Collaborator

@cheyang cheyang Jun 7, 2023

Choose a reason for hiding this comment

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

Could you add logic for enabling HELM_DRIVER as secret.

@TrafalgarZZZ
HELM_DRIVER migrates from secrets to configmap (AlluxioRuntime)
17a21d5 
Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
@TrafalgarZZZ
HELM_DRIVER migrates from secrets to configmap (JindoRuntime)
bfffc69 
Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
@TrafalgarZZZ
HELM_DRIVER migrates from secrets to configmap (JuiceFSRuntime)
8d40dce 
Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
@TrafalgarZZZ
Fix go fmt
8cb1ae5 
Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
@TrafalgarZZZ
HELM_DRIVER migrates from secrets to configmap (EFCRuntime)
4254632 
Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
@TrafalgarZZZ
Refactor func NewCacheClientBypassSecrets
f4e693a 
Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
@TrafalgarZZZ
HELM_DRIVER migrates from secrets to configmap (ThinRuntime)
0efd462 
Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
@TrafalgarZZZ
Fix merge conflicts
e9bec48 
Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
@TrafalgarZZZ
Fix typo in comments on func NewCacheClientBypassSecrets
58038d8 
Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
@TrafalgarZZZ TrafalgarZZZ force-pushed the feature/support_configmap_helm_driver branch from fede12c to 58038d8 Compare June 26, 2023 10:07
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.0% 2.0% Duplication

@TrafalgarZZZ TrafalgarZZZ marked this pull request as ready for review June 26, 2023 12:49
@cheyang
Copy link
Collaborator

cheyang commented Jun 26, 2023

/test fluid-e2e

cheyang
cheyang approved these changes Jun 27, 2023
View reviewed changes
Copy link
Collaborator

@cheyang cheyang left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@fluid-e2e-bot fluid-e2e-bot bot added the lgtm label Jun 27, 2023
@fluid-e2e-bot
Copy link

fluid-e2e-bot bot commented Jun 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheyang

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@fluid-e2e-bot fluid-e2e-bot bot merged commit 079a653 into fluid-cloudnative:master Jun 27, 2023
@TrafalgarZZZ TrafalgarZZZ mentioned this pull request Jun 29, 2023
Merged
This was referenced Aug 10, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cheyang cheyang cheyang approved these changes

Assignees

@cheyang cheyang

Labels

approved lgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@TrafalgarZZZ @cheyang