-
Notifications
You must be signed in to change notification settings - Fork 241
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
oss: allow setup mime.types by configmap #1209
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AlbeeSo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would design the API like this:
Add two per-PV options:
- mimeVolume: we just pass it through to fuse pod spec.volumes. Should support both hostPath and configMap type volume. The name of configmap should be configurable.
- mimeSubpath: use this path to access the actual mime.types file in the volume
The current design is not desiable because:
- If user have too many volumes, the
ossfs-mime-config
can be very large. It is not scalable. - The user cannot reuse the same config for multiple volume, making
ossfs-mime-config
even larger. And hard to operate. - It is hard to authorize config for different volumes to different users (e.g. by RBAC) if we put all of them in the same configMap.
|
||
func hasSetMimeConfigMap(volumeId string) bool { | ||
cfg := options.MustGetRestConfig() | ||
clientset := kubernetes.NewForConfigOrDie(cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reuse clientset, for rate-limiter and others to work properly.
func Test_AddDefaultMountOptions(t *testing.T) { | ||
httpmock.Activate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to inject a fake clientset into the tested instance, and avoid modifying global state, to enable parallel test in the future.
And, why not use the fake clientset implemented in client-go?
f8913d2
to
0a2deed
Compare
0a2deed
to
d43ab7a
Compare
sounds great. but i think a entire volume config is a little bit hard to config? maybe we can just support the configmap way to set up the mime.types. users can just specify the name of cm, then CSI convert the map of data to standard format for mime.types. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
allow users setup mime.types with volume scoped, and modify it after CSI startup
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: