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

[installer]: fix incorrectly configured pod security policies #7106

Merged
merged 1 commit into from
Dec 7, 2021

Conversation

mrsimonemms
Copy link
Contributor

@mrsimonemms mrsimonemms commented Dec 7, 2021

Description

This now makes Gitpod work in a cluster with pod security policies enabled. Seems there was some missed configurations.

This does not include the work for InCluster Docker registry, see #7107

Related Issue(s)

Fixes #7095

How to test

Deploy to a cluster with Pod Security Policies enabled. Have tested with all combinations of incluster and external dependencies

Release Notes

[installer]: fix incorrectly configured pod security policies

Documentation

@mrsimonemms mrsimonemms force-pushed the sje/installer-psps branch 2 times, most recently from 294124f to cd1b30a Compare December 7, 2021 15:40
@@ -34,7 +34,7 @@ const (
RegistryAuthSecret = "builtin-registry-auth"
RegistryTLSCertSecret = "builtin-registry-certs"
RegistryFacadeComponent = "registry-facade"
RegistryFacadeServicePort = 3000
RegistryFacadeServicePort = 30000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed value as registry-facade/podsecuritypolicy.go set to 30000-33000. It seems that this is post-processed in SaaS/Core Dev to be in that range

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is done with Helm now like so.

This now makes Gitpod work in a cluster with pod security policies enabled.
@mrsimonemms mrsimonemms marked this pull request as ready for review December 7, 2021 15:48
@mrsimonemms mrsimonemms changed the title [installer]: apply incorrectly configured pod security policies [installer]: fix incorrectly configured pod security policies Dec 7, 2021
@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #7106 (8d34bd9) into main (b6f50e0) will decrease coverage by 24.75%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #7106       +/-   ##
==========================================
- Coverage   30.52%   5.76%   -24.76%     
==========================================
  Files          34      13       -21     
  Lines        5917    1162     -4755     
==========================================
- Hits         1806      67     -1739     
+ Misses       3974    1094     -2880     
+ Partials      137       1      -136     
Flag Coverage Δ
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
components-supervisor-app ?
installer-raw-app 5.76% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/supervisor/pkg/supervisor/supervisor.go
components/supervisor/pkg/supervisor/tasks.go
components/supervisor/pkg/ports/exposed-ports.go
components/supervisor/pkg/terminal/ring-buffer.go
components/supervisor/pkg/supervisor/services.go
components/supervisor/pkg/terminal/terminal.go
components/supervisor/pkg/supervisor/user.go
components/supervisor/pkg/supervisor/config.go
components/supervisor/pkg/supervisor/ssh.go
components/supervisor/pkg/ports/ports-config.go
... and 11 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 b6f50e0...8d34bd9. Read the comment docs.

@@ -277,6 +277,7 @@ var Helm = common.CompositeHelmFunc(
helm.KeyValue("rabbitmq.auth.password", password),
helm.KeyValue("rabbitmq.auth.existingErlangSecret", CookieSecret),
helm.KeyValue("rabbitmq.auth.tls.existingSecret", TLSSecret),
helm.KeyValue("rabbitmq.serviceAccount.name", Component),
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this, this is much better than changing the component!

@kylos101
Copy link
Contributor

kylos101 commented Dec 7, 2021

/approve

@mrsimonemms mrsimonemms requested a review from a team December 7, 2021 16:09
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Have not tested, but change LGTM

Please feel free to ignore this comment; I assume you already considerd that.

@roboquat
Copy link
Contributor

roboquat commented Dec 7, 2021

LGTM label has been added.

Git tree hash: 1ddb68ae75214623e26b1c34775485aee0c717e5

@kylos101
Copy link
Contributor

kylos101 commented Dec 7, 2021

LGTM

@mrsimonemms
Copy link
Contributor Author

Have not tested, but change LGTM

Please feel free to ignore this comment; I assume you already considerd that.

It's always worthwhile checking I've not done something daft. Thanks

@@ -34,7 +34,7 @@ const (
RegistryAuthSecret = "builtin-registry-auth"
RegistryTLSCertSecret = "builtin-registry-certs"
RegistryFacadeComponent = "registry-facade"
RegistryFacadeServicePort = 3000
RegistryFacadeServicePort = 30000
Copy link
Member

Choose a reason for hiding this comment

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

that value could produce some collision (default initial value for node ports)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it could. @kylos101 has done some post-processing for Core Dev on daemonsets to avoid collisions as part of his Werft job. Is that the collision you're talking about?

Copy link
Contributor

Choose a reason for hiding this comment

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

The range core-dev uses for registry-facade is 30000-31000. @aledbf is that the potential collision you were referring? If yes, I think we'll be okay, because this is referring registry-facade.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Are we doing the same post-processing on user clusters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we're not. The general principle that we're working on is one deployment of Gitpod per cluster. It's not explicitly noted at the moment, but it's one that will be documented when we do the public introductory docs

@mrsimonemms
Copy link
Contributor Author

/hold

@kylos101
Copy link
Contributor

kylos101 commented Dec 7, 2021

/lgtm

@mrsimonemms mrsimonemms changed the base branch from main to kyleb/installer-werft December 7, 2021 16:58
@roboquat roboquat added size/XXL and removed size/L labels Dec 7, 2021
@mrsimonemms mrsimonemms changed the base branch from kyleb/installer-werft to main December 7, 2021 17:09
@roboquat roboquat removed the size/XXL label Dec 7, 2021
@mrsimonemms
Copy link
Contributor Author

mrsimonemms commented Dec 7, 2021

/werft run

👍 started the job as gitpod-build-sje-installer-psps.5

@roboquat roboquat added the size/L label Dec 7, 2021
@mrsimonemms
Copy link
Contributor Author

/unhold

@mrsimonemms
Copy link
Contributor Author

/approve

@roboquat
Copy link
Contributor

roboquat commented Dec 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geropl, kylos101, MrSimonEmms

Associated issue: #7107

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 2631195 into main Dec 7, 2021
@roboquat roboquat deleted the sje/installer-psps branch December 7, 2021 17:15
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note size/L team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Installer]: incorrect pod security policies
5 participants