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.

Sign up for GitHub

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

Ensure file sync works in multi-namespace environments #5464

Closed
wants to merge 1 commit into from
Closed

Ensure file sync works in multi-namespace environments #5464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions pkg/skaffold/runner/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@omninonsense omninonsense Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking more into this, we provide an UpdateNamespace method which updates the namespaces with deployed resources.

skaffold/pkg/skaffold/runner/runcontext/context.go

Line 244 in 5bf0323

func (rc *RunContext) UpdateNamespaces(ns []string) {

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?


context, ok := config.Contexts[config.CurrentContext]
if ok {
nsMap[context.Namespace] = true
} else {
nsMap[""] = true
}
} else {
nsMap[configNamespace] = true
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/runner/util/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func TestGetAllPodNamespaces(t *testing.T) {
{
description: "kube context's namespace",
currentContext: "prod-context",
expected: []string{"prod"},
expected: []string{"", "prod"},
},
{
description: "default namespace",
Expand Down