Skip to content

[installer] Fix server GitHub app cert mount #9799

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.

Sign up for GitHub

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
May 6, 2022
Merged

[installer] Fix server GitHub app cert mount #9799

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion install/installer/pkg/components/server/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package server
import (
"encoding/base64"
"fmt"
"path"

"github.com/gitpod-io/gitpod/installer/pkg/cluster"

Expand Down Expand Up @@ -201,7 +202,7 @@ func deployment(ctx *common.RenderContext) ([]runtime.Object, error) {

volumeMounts = append(volumeMounts, corev1.VolumeMount{
Name: githubAppCertSecret,
MountPath: cfg.WebApp.Server.GithubApp.CertPath,
MountPath: path.Dir(cfg.WebApp.Server.GithubApp.CertPath),
Copy link
Member

Choose a reason for hiding this comment

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

Do we validate the path somewhere? If the path is empty, path.Dir() returns . which is unlikely what we want here. https://go.dev/play/p/P9N0NNBdfdq

Copy link
Member Author

@geropl geropl May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thought about that as well, but I could not come up with anything sensible here.

As a reminder, we have a single client for this anyway, and we need to ensure config stability in general at a higher level anyway, so: I don't think it make sense to add tests for properties on the strings we pipe through. 🤷

Copy link
Member

Choose a reason for hiding this comment

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

For SaaS yes, but if this is also used by self-hosted, chances are someone will misconfigure it. Perhaps just checking that the path is not empty and erroring when it is makes sense here to avoid unexpected behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's in the experimental section, so by definition only to be used by us.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense for now then. Thanks for the clarification.

ReadOnly: true,
})
}
Expand Down