Ensure file sync works in multi-namespace environments #5464
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5464 +/- ##
==========================================
- Coverage 71.45% 71.07% -0.39%
==========================================
Files 397 402 +5
Lines 14569 15055 +486
==========================================
+ Hits 10411 10700 +289
- Misses 3388 3563 +175
- Partials 770 792 +22
Continue to review full report at Codecov.
|
|
I'm not sure if I need to do something about the "kokoro" thing. 👀 |
omninonsense
changed the title
|
hey @omninonsense , thanks for opening this, and sorry for the delayed response! It looks good to me, however I'm asking the other skaffold team members to take a look as well as I'm not completely sure if there are potential issues that could come from this. I'll try to have an update soon :) |
| @@ -38,11 +38,13 @@ func GetAllPodNamespaces(configNamespace string, pipelines []latest.Pipeline) ([ | |||
| return nil, fmt.Errorf("getting k8s configuration: %w", err) | |||
| } | |||
|
|
|||
| // The empty string is here to ensure that the File Sync feature | |||
| // works with multi-namespace environments | |||
| nsMap[""] = true | |||
There was a problem hiding this comment.
the reason why we we are against "" in nsMap is because not everyone has permissions to fetch pods and objects in all namespaces.
With empty namespace, k8s api will try to fetch pods, monitors etc in all namespaces.
To avoid getting a permission issue, we only add "" when no namespace is present.
There was a problem hiding this comment.
Ah right, I see. My assumption was that skaffold is only used with local clusters (minikube and similar), but now your comment implies people use it with shared clusters too. So, in that case I see how this would cause problems.
This is mostly an issue for kustomize and kubectl deployments, since they're simply "glide-through" skaffold without any processing. (Edit: assuming I remember the code correctly).
Could I instead peek at .metadata.namespace of the kustomize-compiled resources? I know could be made possible, but I'm not sure if that's desirable? In that case it would work in a similar fashion to the Helm namespace heuristic.
There was a problem hiding this comment.
Looking more into this, we provide an UpdateNamespace method which updates the namespaces with deployed resources.
I submitted a PR recently where PodWatcher.Start() uses updated namespace.
I am wondering if that fixed this issue.
#5487
Can you verify this on the latest version of skaffold?
|
@omninonsense this PR has gotten stale, but I believe this issue may have been fixed through #5487. going to close this out for now, but will reopen if we get reports that the issue still exists and we can discuss alternative approaches for inferring the namespace. |
|
Hi! @nkubala! The workaround mentioned in #4246 (comment) works, so this wasn't a priority for us at the company any longer.And... I'll be honest: I forgot about this PR. Sorry about that! 😅 I realise creating a repro of this is a chore, so I'll check if #5487 fixes it for us (at a glance it seems like it should). |
Fixes: #4246
Description
Some kubernetes contexts set a default namespace. This breaks the file sync feature in a way that it prevented files being synced to any other namespace (Helm was probably working, but the heuristic didn't work for
deploy.kustomizeanddeploy.kubectldeployment methods).I am not sure if this causes some unexpected behaviour. The tests seem happy with this, so I'm somewhat confident nothing broke. I've only added the "empty" namespace when no
--namespaceis passed (ie the default value is"").I'm not fully convinced the heuristic of parsing kubeconfig to get the default namespace is needed at all, though. Everything seems content with just
"". So, I'm happy to remove the heuristic altogether, but since I'm not familiar with why it was added, I didn't want to do it prematurely.