Skip to content

[public-api] Ensure no objects are rendered by installer without experimental config #9324

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
Apr 20, 2022

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Apr 14, 2022

Description

Ensures that all config related to public-api is only rendered when the experimental config is specified. Currently, only the deployment resource is conditionally rendered but the ServiceAccount and RoleBinding still get rendered.

Before
---
# v1/ServiceAccount public-api-server
apiVersion: v1
automountServiceAccountToken: true
kind: ServiceAccount
metadata:
creationTimestamp: null
labels:
  app: gitpod
  component: public-api-server
name: public-api-server
namespace: default
---
# rbac.authorization.k8s.io/v1/RoleBinding public-api-server
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
creationTimestamp: null
labels:
  app: gitpod
  component: public-api-server
name: public-api-server
namespace: default
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: default-ns-psp:restricted-root-user
subjects:
- kind: ServiceAccount
name: public-api-server
---

With this change, this config above is not rendered.

Related Issue(s)

Part of #9229

How to test

  1. cd install/installer
  2. Generate config with go run main.go init > installer.yaml
  3. Edit the config and set domain anything non-empty
  4. Fetch versions docker run -it --rm "eu.gcr.io/gitpod-core-dev/build/versions:main.2872" cat versions.yaml > versions.yaml
  5. Extend the config from step 2. to add
experimental:
  webapp:
    publicApi:
      enabled: true
  1. Run
export LOG_LEVEL=debug
go run main.go render --config "./installer.yaml" --use-experimental-config --debug-version-file versions.yaml
  1. Search the file for public-api and see no occurences

Release Notes

NONE

Documentation

NONE

/hold

@easyCZ easyCZ changed the title [public-api] Define a k8s service [public-api] Ensure no objects are rendered by installer without experimental config Apr 16, 2022
@@ -18,21 +16,6 @@ import (
)

func deployment(ctx *common.RenderContext) ([]runtime.Object, error) {
var experimentalCfg *experimental.Config

_ = ctx.WithExperimental(func(ucfg *experimental.Config) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

This check has been effectively hoisted higher to the Objects function (the main entry point for rendering anything for public-api). This ensures that future objects are also protected with the same config.

@easyCZ easyCZ marked this pull request as ready for review April 16, 2022 12:56
@easyCZ easyCZ requested a review from a team April 16, 2022 12:56
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Apr 16, 2022
@andrew-farries
Copy link
Contributor

Running the steps to test, I hit:

rendering using experimental config
{
  "valid": false,
  "fatal": [
    "Field 'Config.Domain' is required"
  ]
}
configuration is invalid
exit status 1

after step 5.

Where is the eu.gcr.io/gitpod-core-dev/build/versions image documented? I haven't come across that before.

@easyCZ
Copy link
Member Author

easyCZ commented Apr 19, 2022

Ah, I've forgotten a step. Set the domain to anything non-empty.

The image instructions used to be in the old installer readme, and I got them from Simon over Slack. Will raise a PR to add it to a readme.

@andrew-farries
Copy link
Contributor

With this config:

...
experimental:
  webapp:
    publicApi:
      enabled: true

I see resources (deployments etc) relating to the public API; setting enabled to false doesn't change that.

@easyCZ
Copy link
Member Author

easyCZ commented Apr 20, 2022

Right. Enabled is poorly named, it's more-less so that there is something. For now, we don't parametrize the public API config. To not render it, the epxerimental.webapp.publicApi must not be set at all.

I'll raise a follow-up PR to parametrize it, but the existence of the config should in itself indicate it's enabled, so we shouldn't need an extra property down the line.

@easyCZ
Copy link
Member Author

easyCZ commented Apr 20, 2022

/unhold

Thanks for the review!

@roboquat roboquat merged commit f6d2783 into main Apr 20, 2022
@roboquat roboquat deleted the mp/papi-service branch April 20, 2022 09:32
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Apr 20, 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 deployed Change is completely running in production release-note-none size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants