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

remoteManifests no longer works in v2.0.0 #7990

Closed
rishka opened this issue Oct 26, 2022 · 3 comments · Fixed by #8036
Closed

remoteManifests no longer works in v2.0.0 #7990

rishka opened this issue Oct 26, 2022 · 3 comments · Fixed by #8036
Assignees
Labels
deploy/remote-manifests kind/bug Something isn't working priority/p1 High impact feature/bug.
Milestone

Comments

@rishka
Copy link

rishka commented Oct 26, 2022

Expected behavior

Remote manifests loads manifest from specified kubernetes resource

Actual behavior

Manifests are looked for locally

Information

  • Skaffold version: working: v1.37.2, not working: v2.0.0
  • Operating system: Ubuntu 20.04
  • Installed via: skaffold.dev
  • Contents of skaffold.yaml:
apiVersion: skaffold/v2beta8
kind: Config
build:
  artifacts:
  - image: test
deploy:
  kubectl:
    remoteManifests:
      - deploy/test

Steps to reproduce the behavior

  1. clone https://github.com/rishka/skaffold-manifests-bug
  2. run kubectl apply -f deploy.yaml
  3. run skaffold run
  4. results in nothing to deploy in v2, correctly applies in v1
@aaron-prindle aaron-prindle added deploy/remote-manifests kind/bug Something isn't working priority/p1 High impact feature/bug. labels Oct 31, 2022
@aaron-prindle aaron-prindle added this to the v2.1.0 milestone Nov 1, 2022
@aaron-prindle
Copy link
Contributor

aaron-prindle commented Nov 2, 2022

This is actually an error with our automatic schema updating, remote manifests still work but Skaffold currently does not properly add them to the new location they should be in for skaffold v2.0.0 when migrating old schemas. The field deploy.kubectl.remoteManifests is not actually used in Skaffold v2.0.0 (apiVersion: skaffold/v3) and should be removed as the correct location for remote manifests is manifests.rawYaml. This will work in skaffold v2.0.0 if you manually configure this to be:

apiVersion: skaffold/v3
kind: Config
build:
  artifacts:
  - image: test
manifests:
  rawYaml:
  - deploy/test
  - https://k8s.io/examples/controllers/nginx-deployment.yaml# <-- added as an example remote manifest

In skaffold v2.0.0 whether a manifest is remote is inferred by string (no specific remoteManifests field):
https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/render/generate/generate.go#L73-L116

The issue currently is that our automatic update code for the skaffold.yaml does not actually move deploy.kubectl.remoteManifests to manifests.rawYaml, instead leaving the stanza there and breaking the skaffold.yaml as you encountered. I will submit a fix for skaffold to properly update such skaffold.yaml files but in the meantime you can manually edit your skaffold.yaml file to move the deploy.kubectl.remoteManifests values to be in the list at manifests.rawYaml

EDITED: The above suggestion incorrectly assumed remoteManifests were the same as URL manifests. See below comments for correct analysis of the issue

@aaron-prindle
Copy link
Contributor

aaron-prindle commented Nov 2, 2022

@rishka let me know if you are able to try to make the manual changes suggested above. Want to make sure that is a potential workaround for your skaffold.yaml as if not more investigation needs to be done here. Thanks!

@aaron-prindle
Copy link
Contributor

aaron-prindle commented Nov 3, 2022

Ignore the above comments, this information was incorrect. I did not properly understand the remoteManifests field. I will add some additional information which was posted in my initial PR from @ericzzzzzzz below:


We have some logic to deduce url manifests, not remoteManifests.
In v1 remoteManifests are resources in cluster, not urls where store manifests. implementation is here https://github.com/GoogleContainerTools/skaffold/blob/v1/pkg/skaffold/deploy/kubectl/kubectl.go#L379-L397

we call kubectl get --namespace ${namespace} ${resource name} -o=yaml to retrieve the manifests from clusters.

So I don't think the current change is the right fix, we're currently just not supporting remoteManifests in v2, this change may help supporting reading get manifests from kubenetes resources in remote clusters, but the implementation is hard.

like the reproduction project : https://github.com/rishka/skaffold-manifests-bug/blob/main/skaffold.yaml

the path can just be deploy/test, it's hard to tell if is a local path or some remote k8s resource.

Also, we will need to specify cluster context for this kind of kubectl call, in v1 this readRemoteManifests is a function for deployer, we have a place to specify that kube context in config, but in v2 we cannot specify this kube context under rawK8, we may need a separate config to support reading remote manifests from clusters. Then it's clear that the path is for k8s resources, maybe we don't even have to name it as path, also we're able to specify context. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy/remote-manifests kind/bug Something isn't working priority/p1 High impact feature/bug.
Projects
None yet
2 participants