Skip to content

[installer] Support configuration of the public API HTTP port #11832

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

Merged
merged 1 commit into from
Aug 4, 2022

Conversation

andrew-farries
Copy link
Contributor

Description

As of #11806, the public api exposes an HTTP port for a Stripe webhook handler. This handler should be present on Gitpod SaaS but absent on self hosted installations.

This PR adds experimental config for the public api component to allow an HTTP port to be configured. If the port is not present in the configuration, baseserver ensures that HTTP is not served.

Related Issue(s)

Part of #10937 and #9036 .

How to test

Installer unit tests to check the rendered public api configmap.

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@andrew-farries andrew-farries requested review from a team August 3, 2022 08:04
@github-actions github-actions bot added team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team labels Aug 3, 2022
@@ -205,7 +205,8 @@ type ProxyConfig struct {
}

type PublicAPIConfig struct {
Enabled bool `json:"enabled"`
Enabled bool `json:"enabled"`
HttpPort int32 `json:"httpPort"`
Copy link
Member

Choose a reason for hiding this comment

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

Why should it be configurable? I think we want it on 9002 always

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want the public API to serve HTTP in Gitpod SaaS but not in self-hosted installations. If the httpPort setting is omitted, then the public API won't start the HTTP server (because baseserver won't start listening without a configured port).

Would you prefer the setting to be a boolean (serveHTTP)? Then we can hardcode the port to 9002 if it's set.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think we should instead run the server and just have a No-OP implementation of the endpoint. Turning a whole HTTP server off in self-hosted is bound to bite us when we go to roll out. In practice. you'd need the same config on the proxy and other places and it just gets too complicated. It's better if it runs, just returns "Unvailable in Self-hosted".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not starting the HTTP server in self-hosted is cleaner as then there is less redundant, SaaS-only stuff floating around in a self-hosted instance.

I think the config we need is:

  • This PR.
  • Make the proxy config that we need to add to expose this HTTP endpoint optional (with another installer PR very similar to this one but for proxy).

I'm not sure it's inevitably going to go wrong at rollout, as we have good test coverage here with regards to the yaml rendered by the installer.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, the Public API server is in experimental and not deployed in Self-Hosted anyway. I'd avoid adding extra complexity at this stage which further changes which parts of the server are running and are not - it's already hard to understand what is running and what is not.

Basically, I'd drop the config for the HTTP port and always run the HTTP server on 9002

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, I'd drop the config for the HTTP port and always run the HTTP server on 9002

Was just typing the same comment - 💯 agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so I'll drop the new config surface here and always have the public api open for HTTP on port 9002, even in self-hosted. That means closing https://github.com/gitpod-io/ops/pull/3637 and #11833.

We can think about returning a no-op response for self hosted separately.

Do you agree @easyCZ ?

@andrew-farries andrew-farries force-pushed the af/configure-public-api-http-port branch from 8874a39 to 7c7ac42 Compare August 4, 2022 06:17
@roboquat roboquat added size/S and removed size/M labels Aug 4, 2022
@andrew-farries andrew-farries requested a review from easyCZ August 4, 2022 06:17
@roboquat roboquat merged commit d0b7ffc into main Aug 4, 2022
@roboquat roboquat deleted the af/configure-public-api-http-port branch August 4, 2022 06:36
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production release-note-none size/S team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants