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

[supervisor] Make ports configured in .gitpod.yml private by default #4548

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

corneliusludmann
Copy link
Contributor

Fixes #4159

How to test

@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #4548 (4454cde) into main (299b1b9) will increase coverage by 36.44%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           main    #4548       +/-   ##
=========================================
+ Coverage      0   36.44%   +36.44%     
=========================================
  Files         0       14       +14     
  Lines         0     3880     +3880     
=========================================
+ Hits          0     1414     +1414     
- Misses        0     2349     +2349     
- Partials      0      117      +117     
Flag Coverage Δ
components-ws-manager-app 36.44% <50.00%> (?)

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

Impacted Files Coverage Δ
components/ws-manager/pkg/manager/manager.go 24.45% <50.00%> (ø)
components/ws-manager/pkg/manager/manager_ee.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/config.go 32.50% <0.00%> (ø)
components/ws-manager/pkg/manager/create.go 79.40% <0.00%> (ø)
components/ws-manager/pkg/manager/monitor.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/headless.go 44.00% <0.00%> (ø)
components/ws-manager/pkg/manager/imagespec.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/status.go 72.14% <0.00%> (ø)
components/ws-manager/pkg/manager/probe.go 0.00% <0.00%> (ø)
... and 5 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 299b1b9...4454cde. Read the comment docs.

@corneliusludmann corneliusludmann force-pushed the corneliusludmann/never-make-ports-4159 branch from d08f5b6 to 4ddeb4c Compare June 18, 2021 15:44
@csweichel
Copy link
Contributor

You'll also have to adapt this for the ports that are exposed on workspace start where ws-manager creates the port service: https://github.com/gitpod-io/gitpod/blob/main/components/server/src/workspace/workspace-starter.ts#L609

@corneliusludmann
Copy link
Contributor Author

@csweichel: Thanks for the hint.

What about these 2 places? Do we need to change the default values here as well?

protected portVisibilityToProto(visibility: PortVisibility | undefined): ProtoPortVisibility {
switch (visibility) {
case 'private':
return ProtoPortVisibility.PORT_VISIBILITY_PRIVATE;
default: // the default for requests is: public
case 'public':
return ProtoPortVisibility.PORT_VISIBILITY_PUBLIC;
}
}

func portNameToVisibility(s string) api.PortVisibility {
parts := strings.Split(s, "-")
if len(parts) != 2 {
// old or wrong port name: return default
return api.PortVisibility_PORT_VISIBILITY_PUBLIC
}
// parse (or public as fallback: important for backwards compatibility during rollout)
visibilitStr := fmt.Sprintf("PORT_VISIBILITY_%s", strings.ToUpper(parts[1]))
i32Value, present := api.PortVisibility_value[visibilitStr]
if !present {
return api.PortVisibility_PORT_VISIBILITY_PUBLIC
}
return api.PortVisibility(i32Value)
}

@corneliusludmann corneliusludmann marked this pull request as draft June 21, 2021 08:01
@corneliusludmann corneliusludmann force-pushed the corneliusludmann/never-make-ports-4159 branch from 4ddeb4c to cd840d6 Compare June 21, 2021 08:03
@corneliusludmann corneliusludmann marked this pull request as ready for review June 21, 2021 08:04
@csweichel
Copy link
Contributor

@csweichel: Thanks for the hint.

What about these 2 places? Do we need to change the default values here as well?

Yes

@corneliusludmann corneliusludmann force-pushed the corneliusludmann/never-make-ports-4159 branch from cd840d6 to c959e4f Compare June 21, 2021 09:17
@corneliusludmann corneliusludmann force-pushed the corneliusludmann/never-make-ports-4159 branch from c959e4f to 4454cde Compare June 21, 2021 11:54
Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

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

LGTM

@corneliusludmann corneliusludmann merged commit caa322c into main Jun 21, 2021
@corneliusludmann corneliusludmann deleted the corneliusludmann/never-make-ports-4159 branch June 21, 2021 14:22
@mikenikles mikenikles added the changelog worth adding to www.gitpod.io/changelog label Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog worth adding to www.gitpod.io/changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Never make ports public unless explicitly configured
4 participants