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

cannot use Bidirectional mountPropagation #1680

Closed
junhuihuang opened this issue Aug 17, 2021 · 13 comments · Fixed by #2222
Closed

cannot use Bidirectional mountPropagation #1680

junhuihuang opened this issue Aug 17, 2021 · 13 comments · Fixed by #2222
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@junhuihuang
Copy link

junhuihuang commented Aug 17, 2021

What happened:
Error from server: error when creating "helloworld.yaml": admission webhook "validatejob.volcano.sh" denied the request: spec.task[0].template.spec.containers[1].volumeMounts.mountPropagation: Forbidden: Bidirectional mount propagation is available only to privileged containers.
What you expected to happen:
Can be created
How to reproduce it (as minimally and precisely as possible):

securityContext:
  privileged: true
volumeMounts:
  mountPropagation: Bidirectional

Anything else we need to know?:
pkg/webhooks/admission/jobs/validate/admit_job.go#validateTaskTemplate() reset privileged to nil

// Skip verify container SecurityContex.Privileged as it depends on
// the kube-apiserver `allow-privileged` flag.
for i, container := range coreTemplateSpec.Spec.Containers {
	if container.SecurityContext != nil && container.SecurityContext.Privileged != nil {
		coreTemplateSpec.Spec.Containers[i].SecurityContext.Privileged = nil
	}
}

Environment:

  • Volcano Version:
  • Kubernetes version (use kubectl version):
  • Cloud provider or hardware configuration:
  • OS (e.g. from /etc/os-release):
  • Kernel (e.g. uname -a):
  • Install tools:
  • Others:
@junhuihuang junhuihuang added the kind/bug Categorizes issue or PR as related to a bug. label Aug 17, 2021
@Thor-wl
Copy link
Contributor

Thor-wl commented Aug 19, 2021

/cc @william-wang Please help take a look. THX.

@stale
Copy link

stale bot commented Nov 17, 2021

Hello 👋 Looks like there was no activity on this issue for last 90 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for 60 days, this issue will be closed (we can always reopen an issue if we need!).

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 17, 2021
@Thor-wl Thor-wl removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 17, 2021
@LuBingtan
Copy link
Contributor

LuBingtan commented Dec 11, 2021

We also meet the same problem in our production environment. It seems the reason is the validation in admission controller, i.e this block in admit_job.go:

// Skip verify container SecurityContex.Privileged as it depends on
// the kube-apiserver `allow-privileged` flag.
for i, container := range coreTemplateSpec.Spec.Containers {
    if container.SecurityContext != nil && container.SecurityContext.Privileged != nil {
        coreTemplateSpec.Spec.Containers[i].SecurityContext.Privileged = nil
    }
}

It will will return an error at k8scorevalid.ValidatePodTemplate, because it will check if the container is privileged when container's mount propagation is Bidirectional (in the vendor validation.go)

privileged := container.SecurityContext != nil && container.SecurityContext.Privileged != nil && *container.SecurityContext.Privileged
if *mountPropagation == core.MountPropagationBidirectional && !privileged {
    allErrs = append(allErrs, field.Forbidden(fldPath, "Bidirectional mount propagation is available only to privileged containers"))
}

And we have temporarily solved this problem in our internal environment.
Firsrt, we removed the reassignment for Privileged, and then enabled the AllowPrivileged capability in admission init func, e.g.

func init() {
	capabilities.Initialize(capabilities.Capabilities{
		AllowPrivileged: true,
		PrivilegedSources: capabilities.PrivilegedSources{
			HostNetworkSources: []string{},
			HostPIDSources:     []string{},
			HostIPCSources:     []string{},
		},
	})
}

But I'm not sure about is there any risk in this solution. Why volcano would skip verify container SecurityContex.Privileged? Can we using a flag (e.g. SkipVerifyPrivileged) to control the behavior?

Is there any suggestions ? @Thor-wl

@yuyue9284
Copy link

Same problem encountered, temporarily set --enabled-admission=/jobs/mutate,/podgroups/mutate,/pods/validate,/pods/mutate,/queues/mutate,/queues/validate in volcano admission to bypass the job validation.

@Thor-wl Thor-wl added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 22, 2022
@Thor-wl
Copy link
Contributor

Thor-wl commented Jan 22, 2022

Thanks for the report. I've added important label for this issue. Will take a priority to do a check.
/assign @Thor-wl

@stale
Copy link

stale bot commented Apr 24, 2022

Hello 👋 Looks like there was no activity on this issue for last 90 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for 60 days, this issue will be closed (we can always reopen an issue if we need!).

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 24, 2022
@Thor-wl Thor-wl removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 9, 2022
@Thor-wl
Copy link
Contributor

Thor-wl commented May 9, 2022

Hi, I'm sorry for reply late. I've taken a look at releated history of the commits. Just as @LuBingtan mentioned above, it takes a check about priviledged containers. It depends on the configuration of kube-apiserver. I agree with @LuBingtan 's advice. Perhaps we can add a flag such as SkipVerifyPrivileged. Let me give a fix about that.

@Thor-wl
Copy link
Contributor

Thor-wl commented May 11, 2022

I've added a parameter enabled-privilege-container for admission webhook to decide whether allows privilege containers, whose default value is false.

@Thor-wl
Copy link
Contributor

Thor-wl commented May 11, 2022

Besides, I notice that issue2123 has discussed about this problem. Another approach is to ignore privilege container check in volcano admission webhook for it has already been checked by kube-apiserver before. What do you think about the 2 solutions? @junhuihuang @LuBingtan @yuyue9284

@william-wang
Copy link
Member

As the kube-apiserver already check it. No need to check it in Volcano. It's inconvient for some users to check the api-server configuration and then keep volcano the same with apiserver, e.g. user use the k8s managed by cloud provider.

@k82cn
Copy link
Member

k82cn commented May 11, 2022

I'm a little confiused about this issue? Did we skip "privilege" in Volcano right now? If skipped, why do we still have such kind of erro?

@william-wang
Copy link
Member

We skiped "privilege" by setting coreTemplateSpec.Spec.InitContainers[i].SecurityContext.Privileged = nil, however mountPropagation: Bidirectional in pod depends on the "priviledge" which is changed to nil by us.

@Thor-wl
Copy link
Contributor

Thor-wl commented May 11, 2022

I'm a little confiused about this issue? Did we skip "privilege" in Volcano right now? If skipped, why do we still have such kind of erro?

Because of the the force convert from privilege containers to common containers, which are introduced from PR411 and PR2125, Bidirectional mountPropagation cannot be used for it can only with privilege containers. It is very unreasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants