fix: explicitly set namespace on KubeClient #8785
Conversation
helm-bot
added
the
size/XS
|
Hi @object88. We ask for contributors to provide tests along with their changes to ensure the fix actually ensures it fixes the issue. It also self-documents what you are trying to fix, and ensures regressions won’t be introduced in the future. Thanks! |
|
Hi, @object88 - in order for us to merge if/when the time comes, we do need a DCO signature; thanks! https://github.com/helm/helm/pull/8785/checks?check_run_id=1147227884 |
pkg/action/action.go
Outdated
| @@ -365,6 +365,7 @@ func (c *Configuration) recordRelease(r *release.Release) { | |||
| func (c *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string, log DebugLog) error { | |||
| kc := kube.New(getter) | |||
| kc.Log = log | |||
| kc.Namespace = namespace | |||
There was a problem hiding this comment.
This is a place to set the namespace but it causes a problem.
The Release secrets can be installed into a different namespace from the release manifests. The Helm client doesn't do that. But, because the namespace is passed in one place for the action config and a separate place for the actions this is an option for users of the API.
If it is set here than the namespace for the two things will be the same and the namespace passed to the actions themselves will be ignored. So, we should set the namespace elsewhere.
We need to find a different place to set it and, while doing so, make it concurrency safe.
There was a problem hiding this comment.
Ah, thank you! I had a feeling that there were some wrinkles I was not aware of. We have some discussion going both here and in the related issue -- where is the best place to continue? I have an idea of how to address it, but I don't know if it would feel right to the helm team.
There was a problem hiding this comment.
@object88 I have an idea how to fix it but I dunno if the rest of the client team would agree. Gimmie a little bit to confer with some of them.
"I think" a new KubeClient needs to be created inside of each action (i.e., the Run or New function). There is can be created in a similar manner to actionconfig Init but have the correct namespace set. This would apply to each of the actions.
This way the namespace setting is separate and it's concurrency safe.
There was a problem hiding this comment.
@object88 The thought on KubeClients in each action for that action and kept private to it looks like a good way to go. Did you want to take a shot at writing that? If not, I can.
There was a problem hiding this comment.
I would like to. I'm enjoying digging into this, and it's relevant to a project I'm working on, so I'm motivated. If I find that I am dropping my focus, I will let you know.
There was a problem hiding this comment.
I took a shot at implementing the idea. Can't say if I landed on target, so any feedback is appreciated. PR currently only changes the Install action, and there's documentation & testing to do, and, well, maybe Do isn't a very good name, etc., etc.
The thought is that the Configuration can provide a kube.Interface to use within the scope of a Run, with the namespace established at that time. The original KubeConfig is still there unchanged, but it's not used... so, why is it there (aside from the fact that removing it would be a breaking API change)?
If this looks like it's the right direction, I will make the same change across other actions, fix up any unit tests (unsure if any need to be added, aside from a concurrency test?), and document the changes. If not, I am more than happy to take it in another direction.
There was a problem hiding this comment.
Hello @mattfarina , I took a shot at implementing the rest of the changes. Please let me know if there are dangerous edges or anything that I've missed. Thanks!
Signed-off-by: Paul Brousseau <object88@gmail.com>
Signed-off-by: Paul Brousseau <object88@gmail.com>
object88
force-pushed
the
deploy-k8s-resources-to-right-namespace
branch
from
45b972d
to
ed0da9f
Compare
helm-bot
added
size/M
Signed-off-by: Paul Brousseau <object88@gmail.com>
object88
force-pushed
the
deploy-k8s-resources-to-right-namespace
branch
from
eb9bd88
to
9ac7d9e
Compare
Signed-off-by: Paul Brousseau <object88@gmail.com>
helm-bot
added
size/L
Signed-off-by: Paul Brousseau <object88@gmail.com>
|
does #10230 solve your issue? |
What this PR does / why we need it:
There seems to be a problem in the Go SDK that deploys k8s resources to the namespace of the kube config, instead of a namespace specified by the action. See the linked issues below.
Special notes for your reviewer:
This requires testing, etc., The PR is submitted to get a quick feedback, i.e., "this is not the right idea at all", or "yeah, seems like it could be legit". Will write tests, etc.
If applicable:
closes #8780
closes #8685