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

rootless: fail early if prerequiresites are not satisfied #2129

Merged
merged 1 commit into from
Mar 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions pkg/cluster/internal/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ type ClusterOptions struct {

// Cluster creates a cluster
func Cluster(logger log.Logger, p providers.Provider, opts *ClusterOptions) error {
// validate provider first
if err := validateProvider(p); err != nil {
Copy link
Contributor

@aojea aojea Mar 15, 2021

Choose a reason for hiding this comment

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

@BenTheElder I don't know if we should validate it only on create cluster or always in DetectNodeProvider() ,

// DetectNodeProvider allows callers to autodetect the node provider
// *without* fallback to the default.
//
// Pass the returned ProviderOption to NewProvider to pass the auto-detect Docker
// or Podman option explicitly (in the future there will be more options)
//
// NOTE: The kind *cli* also checks `KIND_EXPERIMENTAL_PROVIDER` for "podman" or
// "docker" currently and does not auto-detect / respects this if set.
//
// This will be replaced with some other mechanism in the future (likely when
// podman support is GA), in the meantime though your tool may wish to match this.
//
// In the future when this is not considered experimental,
// that logic will be in a public API as well.
func DetectNodeProvider() (ProviderOption, error) {
// auto-detect based on each node provider's IsAvailable() function
if docker.IsAvailable() {
return ProviderWithDocker(), nil
}
if podman.IsAvailable() {
return ProviderWithPodman(), nil
}
return nil, errors.WithStack(NoNodeProviderDetectedError)
}

we currently have <provider>.IsAvailable()
Should it be <provider>.IsAvailableAndValid() ?

Copy link
Member Author

@AkihiroSuda AkihiroSuda Mar 15, 2021

Choose a reason for hiding this comment

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

I added this validation to create.go because deletion should not require this validation.

When a user booted the host with cgroup v2, created a rootless kind cluster, and then rebooted with cgroup v1 for running some other apps that do not support cgroup v2, the user still want to be able to remove the kind cluster.

Copy link
Contributor

Choose a reason for hiding this comment

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

my comment is more about the long term solution, Ben always wanted to model the providers API (as you can see in the comment that I pasted above)

So I'm wondering if this is the time to do it, to avoid start to grow it organically, 👍

When a user booted the host with cgroup v2, created a rootless kind cluster, and then rebooted with cgroup v1 for running some other apps that do not support cgroup v2, the user still want to be able to remove the kind cluster.

we should also start to think in what is supported , that is an interesting use case

Copy link
Member

Choose a reason for hiding this comment

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

I failed to click submit on my comment earlier it seems ...

Delete should not require validation indeed, the validation is wether the tagged resource exists.

It's fine for this to grow somewhat organically internally for now, we can create a better API in the future and consider it exporting it once we have a better idea what we need. It's the public APIs that we need to be more careful with (because people already depend on them and we can't refactor them easily, so we need to make any changes minimally difficult to deal with / not really remove APIs etc.). We can completely rewrite our own internal usage. I'm going to take a sledgehammer to the "actions" thing when I get some freetime someday, and the node build code ...

return err
}

// default / process options (namely config)
if err := fixupOptions(opts); err != nil {
return err
Expand Down Expand Up @@ -234,3 +239,19 @@ func fixupOptions(opts *ClusterOptions) error {

return nil
}

func validateProvider(p providers.Provider) error {
info, err := p.Info()
if err != nil {
return err
}
if info.Rootless {
if !info.Cgroup2 {
return errors.New("running kind with rootless provider requires cgroup v2, see https://kind.sigs.k8s.io/docs/user/rootless/")
}
if !info.SupportsMemoryLimit || !info.SupportsPidsLimit || !info.SupportsCPUShares {
return errors.New("running kind with rootless provider requires setting systemd property \"Delegate=yes\", see https://kind.sigs.k8s.io/docs/user/rootless/")
}
}
return nil
}
40 changes: 35 additions & 5 deletions pkg/cluster/internal/providers/docker/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ func NewProvider(logger log.Logger) providers.Provider {
// see NewProvider
type provider struct {
logger log.Logger
info *providers.ProviderInfo
}

// String implements fmt.Stringer
Expand Down Expand Up @@ -285,18 +286,47 @@ func (p *provider) CollectLogs(dir string, nodes []nodes.Node) error {
}

// Info returns the provider info.
// The info is cached on the first time of the execution.
func (p *provider) Info() (*providers.ProviderInfo, error) {
cmd := exec.Command("docker", "info", "--format", "{{json .SecurityOptions}}")
var err error
if p.info == nil {
p.info, err = info()
}
return p.info, err
}

// dockerInfo corresponds to `docker info --format '{{json .}}'`
type dockerInfo struct {
CgroupDriver string `json:"CgroupDriver"` // "systemd", "cgroupfs", "none"
CgroupVersion string `json:"CgroupVersion"` // e.g. "2"
MemoryLimit bool `json:"MemoryLimit"`
PidsLimit bool `json:"PidsLimit"`
CPUShares bool `json:"CPUShares"`
SecurityOptions []string `json:"SecurityOptions"`
}

func info() (*providers.ProviderInfo, error) {
cmd := exec.Command("docker", "info", "--format", "{{json .}}")
out, err := exec.Output(cmd)
if err != nil {
return nil, errors.Wrap(err, "failed to get docker info")
}
var securityOptions []string
if err := json.Unmarshal(out, &securityOptions); err != nil {
var dInfo dockerInfo
if err := json.Unmarshal(out, &dInfo); err != nil {
return nil, err
}
var info providers.ProviderInfo
for _, o := range securityOptions {
info := providers.ProviderInfo{
Cgroup2: dInfo.CgroupVersion == "2",
}
// When CgroupDriver == "none", the MemoryLimit/PidsLimit/CPUShares
// values are meaningless and need to be considered false.
// https://github.com/moby/moby/issues/42151
if dInfo.CgroupDriver != "none" {
info.SupportsMemoryLimit = dInfo.MemoryLimit
info.SupportsPidsLimit = dInfo.PidsLimit
info.SupportsCPUShares = dInfo.CPUShares
}
for _, o := range dInfo.SecurityOptions {
// o is like "name=seccomp,profile=default", or "name=rootless",
csvReader := csv.NewReader(strings.NewReader(o))
sliceSlice, err := csvReader.ReadAll()
Expand Down
44 changes: 42 additions & 2 deletions pkg/cluster/internal/providers/podman/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package podman
import (
"encoding/json"
"fmt"
"io/ioutil"
"net"
"os"
"path/filepath"
Expand Down Expand Up @@ -53,6 +54,7 @@ func NewProvider(logger log.Logger) providers.Provider {
// see NewProvider
type provider struct {
logger log.Logger
info *providers.ProviderInfo
}

// String implements fmt.Stringer
Expand Down Expand Up @@ -354,9 +356,47 @@ func (p *provider) CollectLogs(dir string, nodes []nodes.Node) error {
}

// Info returns the provider info.
// The info is cached on the first time of the execution.
func (p *provider) Info() (*providers.ProviderInfo, error) {
Copy link
Member

Choose a reason for hiding this comment

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

we should probably note that this is cached.

Copy link
Contributor

Choose a reason for hiding this comment

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

this can not change on runtime, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment line about that.

this can not change on runtime, right?

Right

if p.info == nil {
p.info = info(p.logger)
}
return p.info, nil
}

func info(logger log.Logger) *providers.ProviderInfo {
euid := os.Geteuid()
info := &providers.ProviderInfo{
Rootless: os.Geteuid() != 0,
Rootless: euid != 0,
}
if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

this is going to bite us someday when someone requests remote podman 🙃
(not a blocker I think, but leaving a breadcrumb @aojea)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that will be a future work, and probably needs some work on Podman side.

info.Cgroup2 = true
// Unlike `docker info`, `podman info` does not print available cgroup controllers.
// So we parse "cgroup.subtree_control" file by ourselves.
subtreeControl := "/sys/fs/cgroup/cgroup.subtree_control"
if info.Rootless {
// Change subtreeControl to the path of the systemd user-instance.
// Non-systemd hosts are not supported.
subtreeControl = fmt.Sprintf("/sys/fs/cgroup/user.slice/user-%d.slice/user@%d.service/cgroup.subtree_control", euid, euid)
}
if subtreeControlBytes, err := ioutil.ReadFile(subtreeControl); err != nil {
logger.Warnf("failed to read %q: %+v", subtreeControl, err)
} else {
for _, controllerName := range strings.Fields(string(subtreeControlBytes)) {
switch controllerName {
case "cpu":
info.SupportsCPUShares = true
case "memory":
info.SupportsMemoryLimit = true
case "pids":
info.SupportsPidsLimit = true
}
}
}
} else if !info.Rootless {
info.SupportsCPUShares = true
info.SupportsMemoryLimit = true
info.SupportsPidsLimit = true
}
return info, nil
return info
}
6 changes: 5 additions & 1 deletion pkg/cluster/internal/providers/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,9 @@ type Provider interface {

// ProviderInfo is the info of the provider
type ProviderInfo struct {
Rootless bool
Rootless bool
Cgroup2 bool
SupportsMemoryLimit bool
SupportsPidsLimit bool
SupportsCPUShares bool
}