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

Skaffold by default currently adds a namespace to all rendered manifests (--offline=true must be used to change this). We should change the default not to add a namespace` #8318

Closed
aaron-prindle opened this issue Jan 12, 2023 · 13 comments · Fixed by #8561
Assignees
Labels
area/namespaces area/render kind/todo implementation task/epic for the skaffold team
Milestone

Comments

@aaron-prindle
Copy link
Contributor

aaron-prindle commented Jan 12, 2023

Currently when rendering a manifest with skaffold (skaffold render ...) by default skaffold adds a namespace to all rendered objects:

aprindle@aprindle-ssd ~/skaffold/examples/getting-started  [main]$ skaffold render -a artifacts.json
apiVersion: v1
kind: Pod
metadata:
  name: getting-started
  namespace: default # <--- namespace is added
spec:
  containers:
  - image: gcr.io/aprindle-test-cluster/skaffold-example:v2.0.2-79-g8d655cdb2@sha256:f45a3b94c62ea0a2f3375745809601af3cbb1aa64ebe36f388927096947f26c1
    name: getting-started

This is likely not the best experience for skaffold users as having a hard coded namespace on a manifest when it is not explicity requested in some form can lead to issues where users might want to render and then deploy to a different namespace but the manifests won't be deployable to that namespace as it has a namespace set likely unintentionally.

Currently to get skaffold not to add a namespace when doing skaffold render, the offline=true flag must be added:

aprindle@aprindle-ssd ~/skaffold/examples/getting-started  [main]$ skaffold render -a artifacts.json --offline=true
apiVersion: v1
kind: Pod
metadata:
  name: getting-started
spec:
  containers:
  - image: gcr.io/aprindle-test-cluster/skaffold-example:v2.0.2-79-g8d655cdb2@sha256:f45a3b94c62ea0a2f3375745809601af3cbb1aa64ebe36f388927096947f26c1
    name: getting-started

This is not obvious (offline controls namespacing) and the default here should likely mimic this case vs what it does currently

NOTE:
We should do a git-bisect and identify what PRs or what skaffold versions introduced this behavior and if this is a regression or how it has always worked

@aaron-prindle
Copy link
Contributor Author

Likely the approach with the best UX would be something like:

  • change the default skaffold render to not add namespaces
  • add a new flag -set-namespace that is used to get skaffold to set the namespace (the old default)
  • change --offline=true message to not mention namespace as it is a bit confusing and the related conditions checking this flag related to setting the namespace. The new default should mean users using this flag even without these special cases have their workflows still working. This means offline=true in the future won't have any mention/functionality related to rendering w/ or w/o a namespace.

@jleonar
Copy link

jleonar commented Jan 12, 2023

Does this mean default doesn't get added to all manifests where the namespace isn't specified in the yamls? My use case is sorta what you mention, we build in one environment and publish artifacts. Then use render -> deploy in our prod environment. This default getting added will break for us since we use manifests that don't specify a namespace.

@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Jan 12, 2023

@jleonar to clarify, the PR here is suggesting that skaffold render by default should NOT add a namespace: <defaultNamespace> field (which it does currently UNLESS --offline=true is added). From your comment I'm a bit confused as it seems you are using the default behavior (skaffold render w/o --offline=true) which does add a namespace but it sounds like you don't want this to be done. Currently skaffold does add a namespace: <defaultNamespace> value to rendered manifests and the issue here is tracking to make skaffold not do that (which I think sounds like what you want)

TLDR; If I understand correctly, I think the change in this issue would do what you what, I'm a bit confused how the default behavior is working for your use case currently given what you stated. Are you setting --offline=true?

@jleonar
Copy link

jleonar commented Jan 12, 2023

We have stayed on skaffold v1. Our manifests don't specify namespaces. We use profiles for what namespace we want stuff deployed in. When I tried to upgrade from skaffold v1 to v2 and switch to using render -> deploy, I found that default was getting added and I would get namespace mismatches. I.e. kubectl was saying put in dev namespace but the kustomize yamls had default added to them. So I have been paying attention to the previous issue.

@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Jan 12, 2023

What version of skaffold are you using currently (v1.X.Y)? We are hoping to change skaffold so in our v2.2.0 release the behavior aligns with what you would want from the info above as I understand it

NOTE: unless there are other regressions, friction - you could also try skaffold v2.0.X with --offline=true in the short term. Does that meet your needs or is there other pieces as well that are different/have-regressions?

@jleonar
Copy link

jleonar commented Jan 12, 2023

v1.39.1. I did not know about the offline=true changing the behavior so I will give that a try.

@aaron-prindle aaron-prindle changed the title Skaffold by default currently adds a namespace to all rendered manifests (--offline=true must be used to change this) Skaffold by default currently adds a namespace to all rendered manifests (--offline=true must be used to change this). We should change this Jan 12, 2023
@aaron-prindle aaron-prindle changed the title Skaffold by default currently adds a namespace to all rendered manifests (--offline=true must be used to change this). We should change this Skaffold by default currently adds a namespace to all rendered manifests (--offline=true must be used to change this). We should change this to not be the case Jan 12, 2023
@aaron-prindle aaron-prindle changed the title Skaffold by default currently adds a namespace to all rendered manifests (--offline=true must be used to change this). We should change this to not be the case Skaffold by default currently adds a namespace to all rendered manifests (--offline=true must be used to change this). We should change the default not to add a namespace` Jan 12, 2023
@wvh
Copy link

wvh commented Jan 27, 2023

After a recent upgrade of Skaffold we're running into the same problem, notably kustomize adding the default namespace to manifests that are deliberately without one, and kubectl then complaining about having two different namespaces. I don't think there's currently a way to add --offline=true to the configuration file or just force the kubectl section to use the -k variant of apply, so we're stuck at the moment.

@renzodavid9
Copy link
Contributor

Hey @jleonar, could you share which deployer/renderer are you using in your skaffold.yaml file? Thanks!

@renzodavid9
Copy link
Contributor

@wvh, what version of skaffold v1 (v1.X.Y) are you currently using? From your comment I understand you are using the kustomize deployer, right? Thanks!

@wvh
Copy link

wvh commented Feb 9, 2023

@renzodavid9, I'm using Skaffold v2.0.1, and mostly the skaffold dev sub-command.

I am indeed using the kustomize manifest deployer. Kustomize adds a default namespace, and kubectl then sets the right one with defaultNamespace, which generates an error.

My manifests themselves don't have namespaces, as they can be deployed in different ones and either a kustomization file or a kubectl parameter will determine the right namespace on when applying.

@renzodavid9
Copy link
Contributor

Thanks @wvh. We opened the 8403 issue to track the regression for Kustomize. Follow-up work is to standardized the behavior for all the renderers. Thanks for the info!

@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Feb 27, 2023

Work here is currently prioritized and in progress

@gsquared94
Copy link
Contributor

/triage-action-comment

@aaron-prindle aaron-prindle modified the milestones: v2.3.0, v2.4.0 Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/namespaces area/render kind/todo implementation task/epic for the skaffold team
Projects
None yet
5 participants