Skip to content
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

Change Connect default service.type and other service generation items #478

Merged
merged 8 commits into from
Mar 7, 2024

Conversation

tnederlof
Copy link
Contributor

This PR contains the following changes, following the changes made to RSPM by @atheriel in 2021 (PR here: #122) This change to RSPM was made over 2 years ago and went pretty smoothly, now bringing it to Connect.

  • Use a ClusterIP service by default instead of a NodePort.
  • Add support for controlling loadBalancerIP and clusterIP, which are commonly-used service features.
  • Ignore nodePort settings when the service type is not NodePort. Otherwise, this will generate server-side validation errors.
  • Simplify handling of Service annotations by removing the existing YAML helper is not actually necessary and has a confusing name.
  • Tweak documentation for service.nodePort and service.annotations.

Since the first of these is a breaking change, I bumped the minor version for the chart.

Moving to ClusterIP

ClusterIP is the Kubernetes default, exposing the service in-cluster only. This is a secure, well-understood default. For external users, there are a few options:

  • Use an Ingress. We support this already and it works with ClusterIP.
  • Use a LoadBalancer. This can be expensive so it makes a poor default.
  • Use a NodePort and some kind of external load balancing solution.

It seems like the last route (which is the current default) is much more uncommon than the first two, so I'd advocate for this change.

In addition:

  • NodePorts are a very limited cluster resource, so we should avoid consuming them if we can avoid it.
  • Defaulting to NodePort is a poor security choice, especially if we have users that don't understand the consequences.

Copy link
Contributor

@dbkegley dbkegley left a comment

Choose a reason for hiding this comment

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

Long overdue change, thanks @tnederlof !

Copy link
Contributor

@aronatkins aronatkins left a comment

Choose a reason for hiding this comment

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

I'd like @dbkegley or @colearendt to +1, as well.

@dbkegley
Copy link
Contributor

dbkegley commented Mar 6, 2024

I installed the helm chart from this branch using our local development setup for Connect and everything worked as expected 👍

david@Davids-MacBook-Pro:connect [main] » k get svc
NAME                      TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)           AGE
connect-rstudio-connect   ClusterIP   10.109.240.7   <none>        80/TCP,9108/TCP   6s

Before:

david@Davids-MacBook-Pro:connect [main] » k get svc
NAME                      TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)                       AGE
connect-rstudio-connect   NodePort    10.103.146.3   <none>        80:30590/TCP,9108:32590/TCP   10s

@tnederlof
Copy link
Contributor Author

@dbkegley thanks, works on my side and I verified annotations still make it through on all types. I am unable to merge bc I think CI doesn't like the latest commit being an auto one. If you are able to when the appropriate people have reviewed please do!

@atheriel atheriel merged commit 7bb32e0 into main Mar 7, 2024
5 checks passed
@atheriel atheriel deleted the tn-change-connect-service branch March 7, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants