Add support for running cortex through docker image without root uid #4107

simonswine · 2021-04-23T09:42:31Z

What this PR does:

This allow uses to optionally choose to run the docker image as non-root user with: --user cortex:cortex. It also make sure we test using a non-root user during the integration tests.

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

simonswine · 2021-04-23T09:43:16Z

pkg/ruler/ruler.go

@@ -164,7 +164,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.BoolVar(&cfg.EnableSharding, "ruler.enable-sharding", false, "Distribute rule evaluation using ring backend")
 	f.StringVar(&cfg.ShardingStrategy, "ruler.sharding-strategy", util.ShardingStrategyDefault, fmt.Sprintf("The sharding strategy to use. Supported values are: %s.", strings.Join(supportedShardingStrategies, ", ")))
 	f.DurationVar(&cfg.FlushCheckPeriod, "ruler.flush-period", 1*time.Minute, "Period with which to attempt to flush rule groups.")
-	f.StringVar(&cfg.RulePath, "ruler.rule-path", "/rules", "file path to store temporary rule files for the prometheus rule managers")
+	f.StringVar(&cfg.RulePath, "ruler.rule-path", "./rules", "file path to store temporary rule files for the prometheus rule managers")


Not too sure why we use an absolute path there, but if we want to change that, we should mark it as a breaking change

Unfortunately we can't change the default config value of a non-experimental config option. What we can do in this PR is rolling back this change and configuring differently -ruler.rule-path in NewRuler() in E2E tests if required. WDYT?

That's the approach I went with

cmd/cortex/Dockerfile

integration/ca/ca.go

pracucci

Thanks for working on it! LGTM modulo a comment about the change to default config (and few questions). Could you also take a look at the failing E2E test, please?

pracucci · 2021-04-26T15:14:09Z

pkg/ruler/ruler.go

@@ -164,7 +164,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.BoolVar(&cfg.EnableSharding, "ruler.enable-sharding", false, "Distribute rule evaluation using ring backend")
 	f.StringVar(&cfg.ShardingStrategy, "ruler.sharding-strategy", util.ShardingStrategyDefault, fmt.Sprintf("The sharding strategy to use. Supported values are: %s.", strings.Join(supportedShardingStrategies, ", ")))
 	f.DurationVar(&cfg.FlushCheckPeriod, "ruler.flush-period", 1*time.Minute, "Period with which to attempt to flush rule groups.")
-	f.StringVar(&cfg.RulePath, "ruler.rule-path", "/rules", "file path to store temporary rule files for the prometheus rule managers")
+	f.StringVar(&cfg.RulePath, "ruler.rule-path", "./rules", "file path to store temporary rule files for the prometheus rule managers")


Unfortunately we can't change the default config value of a non-experimental config option. What we can do in this PR is rolling back this change and configuring differently -ruler.rule-path in NewRuler() in E2E tests if required. WDYT?

pracucci · 2021-04-26T15:17:10Z

cmd/cortex/Dockerfile

 COPY       migrations /migrations/
 COPY       cortex /bin/cortex
+
+# allow cortex to bind to port 80 as non-root
+RUN setcap 'cap_net_bind_service=+ep' /bin/cortex


[question] Do you see any problem with this change when running the image in K8S?

I think the way it should work is by specifying in the manifest:

securityContext: capabilities: add: - NET_BIND_SERVICE

However it looks like this doesn't work: kubernetes/kubernetes#56374

Would it be an idea to add a USER line, so it no longer defaults to root?
I guess this might cause trouble upgrading an installation, which has existing files in persistent volumes.

That's what I eventually would like to get to, but in a first step, I want users of the docker image to opt in to not run as root. Otherwise we will need to ensure mounted in persistent volumes are chowned first.

securityContext: ...

I think if we rely on the container orchestration to set it up (k8s in that case), we will break users that are using bare docker. I think without that setcap we probably need to change the default port 80 to something > 1024

integration/configs.go

This allow uses to optionally choose to run the docker image as non-root user with: `--user cortex:cortex`. It also make sure we test using a non-root user during the integration tests Signed-off-by: Christian Simon <simon@swine.de>

During integration tests we are running older versions of Cortex to ensure backwards compatability. These need to still be run as root. Signed-off-by: Christian Simon <simon@swine.de>

pracucci

This PR looks in a good shape. @simonswine do you have any plan to progress on it?

simonswine · 2021-06-18T13:41:59Z

This PR looks in a good shape. @simonswine do you have any plan to progress on it?

@pracucci thanks for the reminder, the outstanding work is to document the way to enable non-root for Kubernetes / docker run. Will hopefully get a chance to do that later today or early next week

stale · 2021-09-21T01:34:22Z

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

oujonny · 2021-10-15T14:46:31Z

Is this PR now merged?

pull-request-size bot added the size/M label Apr 23, 2021

simonswine commented Apr 23, 2021

View reviewed changes

56quarters reviewed Apr 23, 2021

View reviewed changes

cmd/cortex/Dockerfile Show resolved Hide resolved

integration/ca/ca.go Show resolved Hide resolved

pracucci reviewed Apr 26, 2021

View reviewed changes

simonswine force-pushed the 20210423_enable-docker-image-for-non-root branch 2 times, most recently from 291e885 to bfbc991 Compare June 9, 2021 11:43

simonswine added 2 commits June 9, 2021 15:26

Add support for running cortex without root uid

97b561a

This allow uses to optionally choose to run the docker image as non-root user with: `--user cortex:cortex`. It also make sure we test using a non-root user during the integration tests Signed-off-by: Christian Simon <simon@swine.de>

Run older Cortex versions as non-root

f55e3e4

During integration tests we are running older versions of Cortex to ensure backwards compatability. These need to still be run as root. Signed-off-by: Christian Simon <simon@swine.de>

simonswine force-pushed the 20210423_enable-docker-image-for-non-root branch from bfbc991 to f55e3e4 Compare June 9, 2021 14:26

pracucci reviewed Jun 18, 2021

View reviewed changes

stale bot added the stale label Sep 21, 2021

stale bot closed this Oct 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for running cortex through docker image without root uid #4107

Add support for running cortex through docker image without root uid #4107

simonswine commented Apr 23, 2021

simonswine Apr 23, 2021

pracucci Apr 26, 2021

simonswine Jun 8, 2021

pracucci left a comment

pracucci Apr 26, 2021

pracucci Apr 26, 2021

bboreham Jun 22, 2021

bboreham Jun 22, 2021

simonswine Jun 22, 2021

pracucci left a comment

simonswine commented Jun 18, 2021 •

edited

stale bot commented Sep 21, 2021

oujonny commented Oct 15, 2021

Add support for running cortex through docker image without root uid #4107

Add support for running cortex through docker image without root uid #4107

Conversation

simonswine commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

simonswine commented Jun 18, 2021 • edited

stale bot commented Sep 21, 2021

oujonny commented Oct 15, 2021

simonswine commented Jun 18, 2021 •

edited