Conversation
colearendt
requested review from
aronatkins,
atheriel,
christierney,
dbkegley and
zackverham
as code owners
|
Thank you @colearendt ! I started to do this like 3 separate times but got lost in the sauce on the template merge each time. If you have a minute, could you write up a summary of the commands you use somewhere to handle the diff? |
…ting logic bug Also fix up NEWS.md for the rstudio-connect chart
| {{- if .Job.host }} | ||
| nodeName: {{ toYaml .Job.host }} | ||
| {{- end }} | ||
| enableServiceLinks: {{ if hasKey $templateData.pod "enableServiceLinks" }}{{ $templateData.pod.enableServiceLinks }}{{ else }}false{{ end }} |
There was a problem hiding this comment.
@jspiewak What do you think about this?
"Proper" backwards compatibility would mean leaving this unset if it is not specified, but this is a better security posture so you do not expose the endpoints of your other services on Kubernetes to arbitrary Connect content.
There was a problem hiding this comment.
I can't speak for workbench but for connect I think it's fair to say that these content pods are an implementation detail and shouldn't be considered a part of our public API. I'm fine with changing the default in Connect
There was a problem hiding this comment.
What is the behavior when unset?
What would a customer experience with this unexpectedly defaulting to false?
There was a problem hiding this comment.
When unset, the default is to enable service links. Which means you get all the environment variables for every service in your kubernetes cluster.
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.24/#podspec-v1-core
There was a problem hiding this comment.
Interesting discussion on the Kubernetes project: kubernetes/kubernetes#121787
There was a problem hiding this comment.
So, you could roll this out in two steps:
- Enable control over the setting, but maintain existing behavior
- Move to defaulting to
false
Two steps is more work, but gives consumers additional control.
Not clear to me if it is worth it. What is the likelihood that anyone is actually relying upon it being true?
Yes, very interesting discussion. Might be worth linking to in the NEWS item you have.
There was a problem hiding this comment.
I think it's extremely unliklely that anyone is relying upon it being true. And I like the idea of just flipping the switch so (1) maybe we get some data if they are, (2) we ensure everyone has a "more secure" / more stable environment.
It's also relatively low risk:
- if you see it ahead of time in
NEWS.mdwhen upgrading, you can just set the toggle - if you break your environment, you then go to debug the upgrade and set the toggle
- setting the toggle ensures you are aware of these environment variables and how many there are 😅 opting into this behavior rather than tacitly getting it
I'll definitely add to NEWS!
|
@jforest do you want to get this landed soon while it is relatively fresh? Was it something you covered with Cole? |
It was something we covered, and added his latest commits while doing so. It appears there are new conflicts we need to fix first, I'll look at that now |
|
|
5 participants
Launcher now ships template versions 2.4.0. The helm charts are now updated accordingly