Skip to content
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.

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

run chaos-daemon in privileged by default #1453

Merged
merged 5 commits into from
Jan 28, 2021

Conversation

cwen0
Copy link
Member

@cwen0 cwen0 commented Jan 26, 2021

Signed-off-by: cwen0 cwenyin0@gmail.com

What problem does this PR solve?

What is changed and how does it work?

Add a field to control privileged mode and run chaos-daemon in privileged by default.

Checklist

Tests

  • Unit test
  • E2E test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Breaking backward compatibility

Related changes

  • Need to update the documentation

Does this PR introduce a user-facing change?

NONE

Signed-off-by: cwen0 <cwenyin0@gmail.com>
@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #1453 (510e5f6) into master (7e9ff3f) will decrease coverage by 3.77%.
The diff coverage is 59.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1453      +/-   ##
==========================================
- Coverage   55.78%   52.00%   -3.78%     
==========================================
  Files          68       80      +12     
  Lines        4383     5107     +724     
==========================================
+ Hits         2445     2656     +211     
- Misses       1768     2183     +415     
- Partials      170      268      +98     
Impacted Files Coverage Δ
api/v1alpha1/common_types.go 0.00% <0.00%> (ø)
api/v1alpha1/common_webhook.go 100.00% <ø> (ø)
api/v1alpha1/dnschaos_type.go 0.00% <0.00%> (ø)
api/v1alpha1/dnschaos_webhook.go 0.00% <0.00%> (ø)
api/v1alpha1/httpchaos_types.go 0.00% <0.00%> (ø)
api/v1alpha1/iochaos_types.go 0.00% <ø> (-40.00%) ⬇️
api/v1alpha1/jvmchaos_webhook.go 0.00% <0.00%> (ø)
api/v1alpha1/kernelchaos_types.go 0.00% <ø> (-20.00%) ⬇️
api/v1alpha1/kernelchaos_webhook.go 100.00% <ø> (+14.81%) ⬆️
api/v1alpha1/kinds.go 27.27% <ø> (+0.60%) ⬆️
... and 130 more

Continue to review full report at Codecov.

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

@@ -64,6 +64,9 @@ spec:
value: {{ .Values.timezone | default "UTC" }}
{{- end }}
securityContext:
{{- if .Values.chaosDaemon.privileged }}
privileged: true
Copy link
Member

Choose a reason for hiding this comment

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

Please remove 2 spaces here.
I think privileged: true should keep the same indent level with capabilities.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: cwen0 <cwenyin0@gmail.com>
@cwen0 cwen0 requested a review from STRRL January 27, 2021 05:32
STRRL
STRRL previously approved these changes Jan 27, 2021
Copy link
Member

@STRRL STRRL left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -64,6 +64,9 @@ spec:
value: {{ .Values.timezone | default "UTC" }}
{{- end }}
securityContext:
{{- if .Values.chaosDaemon.privileged }}
privileged: true
{{- else }}
capabilities:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to remove these capabilities when privileged is true

Copy link
Member Author

Choose a reason for hiding this comment

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

SYS_PTRACE must be kept and others can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Other capabilities are unnecessary when privileged is true, except SYS_PTRACE. I think we can delete them to make the code cleaner

Signed-off-by: cwen0 <cwenyin0@gmail.com>
Signed-off-by: cwen0 <cwenyin0@gmail.com>
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 1330

@ti-srebot
Copy link
Contributor

/run-all-tests

@WangXiangUSTC WangXiangUSTC merged commit e7db1ee into chaos-mesh:master Jan 28, 2021
@cwen0 cwen0 deleted the set_privilege branch January 28, 2021 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants