-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[installer] set the ServiceType as LB for ws-proxy #8759
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,9 @@ package wsproxy | |
|
||
import ( | ||
"github.com/gitpod-io/gitpod/installer/pkg/common" | ||
"github.com/gitpod-io/gitpod/installer/pkg/config/v1" | ||
corev1 "k8s.io/api/core/v1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
) | ||
|
||
var Objects = common.CompositeRenderFunc( | ||
|
@@ -14,23 +17,33 @@ var Objects = common.CompositeRenderFunc( | |
networkpolicy, | ||
rolebinding, | ||
role, | ||
func(cfg *common.RenderContext) ([]runtime.Object, error) { | ||
ports := map[string]common.ServicePort{ | ||
HTTPProxyPortName: { | ||
ContainerPort: HTTPProxyPort, | ||
ServicePort: HTTPProxyPort, | ||
}, | ||
HTTPSProxyPortName: { | ||
ContainerPort: HTTPSProxyPort, | ||
ServicePort: HTTPSProxyPort, | ||
}, | ||
MetricsPortName: { | ||
ContainerPort: MetricsPort, | ||
ServicePort: MetricsPort, | ||
}, | ||
SSHPortName: { | ||
ContainerPort: SSHTargetPort, | ||
ServicePort: SSHServicePort, | ||
}, | ||
} | ||
return common.GenerateService(Component, ports, func(service *corev1.Service) { | ||
// In the case of Workspace only setup, `ws-proxy` service is the entrypoint | ||
// Hence we use LoadBalancer type for the service | ||
if cfg.Config.Kind == config.InstallationWorkspace { | ||
service.Spec.Type = corev1.ServiceTypeLoadBalancer | ||
service.Annotations["cloud.google.com/neg"] = `{"exposed_ports": {"80":{},"443": {}}}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to hardcode google specific annotations here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These annotations have been hardcoded for some time on the basis that a) they're ignored if not used and b) we don't really have an effective (and non-verbose) way of adding custom annotations to services. Elsewhere, there's also an annotation for |
||
} | ||
})(cfg) | ||
}, | ||
common.DefaultServiceAccount(Component), | ||
common.GenerateService(Component, map[string]common.ServicePort{ | ||
HTTPProxyPortName: { | ||
ContainerPort: HTTPProxyPort, | ||
ServicePort: HTTPProxyPort, | ||
}, | ||
HTTPSProxyPortName: { | ||
ContainerPort: HTTPSProxyPort, | ||
ServicePort: HTTPSProxyPort, | ||
}, | ||
MetricsPortName: { | ||
ContainerPort: MetricsPort, | ||
ServicePort: MetricsPort, | ||
}, | ||
SSHPortName: { | ||
ContainerPort: SSHTargetPort, | ||
ServicePort: SSHServicePort, | ||
}, | ||
}), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off-topic: Do we have already an issue to get rid of this post-processing step and having a config value for the service type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corneliusludmann there is no such issue open specifically for making the service type configurable. I guess the question is do we plan on making this configurable, since our thought process was to not make it configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad you brought that up. 🚀 Indeed, we decided like this in the past. However, 2 things have changed since then:
experimental
section. Everything that we won't support for some reason but need internally for SaaS should go there. We shouldn't need to rely on post-processing.What do you think, @mrsimonemms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, I'm happy for this to go into the
experimental
section. I think that should be a separate change though, as it will require discussion/agreement on the config surface changeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the inactivity on this issue. I have created this issue to track this requirement. Shall we merge this PR and continue the work there?