Skip to content

[installer] support service type ClusterIP for proxy #10537

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
Jun 15, 2022
Merged

Conversation

nandajavarma
Copy link
Contributor

@nandajavarma nandajavarma commented Jun 8, 2022

Description

Currently, there is no way to do a post-processing in KOTS like we suggest to do for replacing service type of proxy to ClusterIP. This PR introduces a new experimental field to the Config so users can provide a patch via the KOTS UI. The patch file can have the following structure:

experimental:
  webapp:
    proxy:
      serviceType: "ClusterIP" # allowed values are "ClusterIP", "LoadBalancer", "NodePort", "ExternalName" consistent with https://pkg.go.dev/k8s.io/api/core/v1#ServiceType

There is slight redundancy in the validation procedure at the moment(in the validation.go and service.go we check the allowed types). This is because we do not validate the config in KOTS as of now. We will remove this redundancy when we have better validation in place.

Related Issue(s)

Fixes #9981

How to test

Follow the instructions of this PR to setup a self-hosted installation with this change. Go to the Config tab of the KOTS UI. In the bottom you will see an Enable additional options section. Enable it, and copy paste the aforementioned YAML snippet to a file and upload it. Update the installment, and if you do a kubectl get svc proxy -n gitpod from the CLI you will see that it now has ClusterIP type.

Release Notes

NONE

Documentation

if cfg.Common != nil && cfg.Common.ServiceConfig != nil {
st, ok := cfg.Common.ServiceConfig["proxy"]
if ok {
if strings.ToLower(st.ServiceType) == "clusterip" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this conditional doesn't have an else clause; if ServiceType is misspelled (eg cluterip) will this this code default to ServiceTypeLoadBalancer? Is there validation occurring in this pipeline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great point Adrien! I agree that we can add a validation process. As of this implementation, it just defaults to LoadBalancer as set in this line.

About the validation, it is a great tip! We can leverage the existing validate cmd! I will do a follow-up PR for this!

@adrienthebo
Copy link
Contributor

Copying my comment from Slack;

My philosophy on pull request review is to provide constructive feedback and make notes where I see potential changes/improvements/risks, but to grant approval as quickly so the author can land the work when they're ready and adopt feedback at their discretion. I think this is ready to ship but there might be a case where an additional conditional clause (or documentation) could be useful.

Since there are no blocking issues on this PR, is it following team policy to grant an approval here? And if we grant that approval, is that sufficient to merge the PR?

Copy link
Contributor

@adrienthebo adrienthebo 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
Copy link
Contributor

corneliusludmann commented Jun 9, 2022

@adrienthebo:

My philosophy on pull request review is to provide constructive feedback and make notes where I see potential changes/improvements/risks, but to grant approval as quickly so the author can land the work when they're ready and adopt feedback at their discretion. I think this is ready to ship but there might be a case where an additional conditional clause (or documentation) could be useful.

100 % agree with this philosophy!

Since there are no blocking issues on this PR, is it following team policy to grant an approval here? And if we grant that approval, is that sufficient to merge the PR?

Approving PR is fine! ✔️ However, since we automatically merge PRs by a bot once all required approvals are given, you should add the hold label on this PR so that the author has the opportunity to address your comments. You can do this with this command in a comment:

/hold

The author just needs to remove the hold label or comment with /unhold and the PR will be merged.

(In this case, the PR still needs approval from Team WebApp. That's because this PR has not been merged yet automatically.)

@nandajavarma
Copy link
Contributor Author

/unhold

@geropl geropl self-assigned this Jun 13, 2022
@nandajavarma nandajavarma force-pushed the nvn/fix-9981 branch 2 times, most recently from c563eee to f59c23b Compare June 13, 2022 14:44
@nandajavarma nandajavarma requested a review from geropl June 13, 2022 14:45
@nandajavarma nandajavarma marked this pull request as draft June 14, 2022 16:46
@nandajavarma nandajavarma marked this pull request as ready for review June 15, 2022 07:46
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

✔️

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/M 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.

Add Option to Set Proxy Service as ClusterIP Type in KOTS Admin UI
5 participants