[Proposal] Add Helm support to skaffold init

[tejal29]

In issue #1651, @soullivaneuh tried running skaffold init in his code base which uses helm deployer.
He discovered a bug, where skaffold init ran into an error.
See details here

This Proposal is to support all the deploy tools that skaffold supports which are helm, kubectl and kustomize in skaffold init flow.
Currently, we only support kubectl deployer and generate skaffold KubeCtlDeployConfig.

This Proposal is to add a new Initializer Interface,

package initializer
// Initializer is the Init API of skaffold and responsible for generating
// skaffold configuration file.
type Initializer interface {
  // GenerateDeployConfig generates Deploy Config for skaffold configuration.
  func GenerateDeployConfig() latest.DeployConfig
  // GetBuildImages returns all the images defined in manifest files for the corresponding tool.
  func GetBuildImages() []string
}

The interface will provide 2 methods.

1. GenerateDeployConfig:
   This method will generate the deploy config for skaffold configuration for each supported deploy tool.

Tool	Deploy Config
kubectl	deploy:   kubectl:     manifests:     - k8-*
helm	deploy:   helm::     releases:     - name: release-name       chartPath: .
kustomize	deploy:   kustomize: {}

2. GetBuildImages:
   This method will return a list of images used in corresponding manifest files for the deploy tool.
   In case of kubectl, these will a list of images defined in image tag defined in k8 yaml.
   For helm, this would be the images defined in values.yaml.
   (in case of helm, i haven't really thought about how to grab those yet. But we could use a norms like,
3. field name has suffix has Image
4. or use some util or pattern recognition to see if a field value is an fully qualified image
   e.g. gocontainer registry name.ParseReference
   These images are required to generate BuildConfig.

build:
  artifacts:
  - image: gcr.io/k8s-skaffold/example

Right now, all skaffold initialization happens in cmd/skaffold/app/cmd/init.go
I plan to refactor this as follows:

1. Add package: pkg/skaffold/initializer
2. Add sub package kubectl, helm and kustomize for each deploy tool supported.
   e.g.: pkg/skaffold/initializer/kubectl, pkg/skaffold/initializer/helm, pkg/skaffold/initializer/kustomize,
3. All the kubectl specific functionality will now reside in pkg/skaffold/initializer/kubect.

[nkubala]

if I'm understanding correctly there would be different implementations of the Initializer interface, e.g. HelmInitializer, KubectlInitializer, etc. how would we know which one to invoke for a given project? we'd need some heuristic on how to determine if we're in a helm project, kustomize project, etc. maybe it's as easy as checking to see if there's a kustomization.yaml or a Chart.yaml, but this might not cover all the bases. I suppose we could also support a flag to specify to skaffold what kind of project we're initializing, e.g. skaffold init --helm.

also, returning the list of images to build isn't the full story. we also need the list of Dockerfiles (or .bzl files or whatever) to build those images, and then take input from the user to match them up.

[tejal29]

how would we know which one to invoke for a given project? we'd need some heuristic on how to determine if we're in a helm project, kustomize project, etc. maybe it's as easy as checking to see if there's a kustomization.yaml or a Chart.yaml, but this might not cover all the bases.

That is correct, i was going to rely on files present.

I suppose we could also support a flag to specify to skaffold what kind of project we're initializing, e.g. skaffold init --helm.

I was thinking of that too. I do like that idea. Adding a hint on command like would help resolve conflicts.
But it should not be necessary.

also, returning the list of images to build isn't the full story. we also need the list of Dockerfiles (or .bzl files or whatever) to build those images, and then take input from the user to match them up.

That is correct, however determining the list Dockerfiles logic should remain the same irrespective of the Deploy Tool unless i am missing something. Hence it is not part of the Initializer Interface.
The common functionality which is

1. Finding docker files
2. Pairing the dockerfiles to images
3. processCliArtifacts and
4. processBuildArtifacts
   would remain same and still be stand alone functions. They would live as functions in initializer module.

// NewCmdInit describes the CLI command to generate a Skaffold configuration.
func NewCmdInit(out io.Writer) *cobra.Command {
	cmd := &cobra.Command{
		Use:   "init",
		Short: "Automatically generate Skaffold configuration for deploying an application",
		Args:  cobra.NoArgs,
		RunE: func(cmd *cobra.Command, args []string) error {
			c := initializer.Config{
				ComposeFile:  composeFile,
				CliArtifacts: cliArtifacts,
				SkipBuild:    skipBuild,
				Force:        force,
				SkaffoldOpts: opts,
			}
			return initializer.DoInit(out, c)
		},
	}
	cmd.Flags().StringVarP(&opts.ConfigurationFile, "filename", "f", "skaffold.yaml", "Filename or URL to the pipeline file")
	cmd.Flags().BoolVar(&skipBuild, "skip-build", false, "Skip generating build artifacts in Skaffold config")
	cmd.Flags().BoolVar(&force, "force", false, "Force the generation of the Skaffold config")
	cmd.Flags().StringVar(&composeFile, "compose-file", "", "Initialize from a docker-compose file")
	cmd.Flags().StringArrayVarP(&cliArtifacts, "artifact", "a", nil, "'='-delimited dockerfile/image pair to generate build artifact\n(example: --artifact=/web/Dockerfile.web=gcr.io/web-project/image)")
	return cmd
}

and then in skaffold/pkg/skaffold/initializer/init.go

package initializer

func DoInit(out io.Writer, c Config) error {
  // Determine which Initializer to use based on command line flag or manifest files present.
  //e.g. for kubectl, 
  kInitializer, err := kubectl.New(potentialConfigs, c.Force)
	if err != nil {
		return err
  }
  ...
  dockerfilePairs = resolveDockerfileImages(dockerfiles, kInitializer.Images())
  pipeline := &latest.SkaffoldPipeline{
		APIVersion: latest.Version,
		Kind:       "Config",
	}
 ...
 pipeline.Build = processBuildArtifacts(dockerfilePairs)
 pipeline.Deploy = kInitializer.GenerateDeployConfig()

}

Does that make sense?

[balopat]

Did you consider adding the Initializer as part of the Deployer interface?

[tejal29]

I did not consider as from the code it felt like the Deployer Interface was very specific to how to deploy.
The constructor had details on command line args etc which were populated from the skaffold config.
e.g.

// KubectlDeployer deploys workflows using kubectl CLI.
type KubectlDeployer struct {
	*latest.KubectlDeploy

	workingDir  string
	kubectl     kubectl.CLI
	defaultRepo string
}
// NewKubectlDeployer returns a new KubectlDeployer for a DeployConfig filled
// with the needed configuration for `kubectl apply`
func NewKubectlDeployer(workingDir string, cfg *latest.KubectlDeploy, kubeContext string, namespace string, defaultRepo string) *KubectlDeployer {
	return &KubectlDeployer{
		KubectlDeploy: cfg,
		workingDir:    workingDir,
		kubectl: kubectl.CLI{
			Namespace:   namespace,
			KubeContext: kubeContext,
			Flags:       cfg.Flags,
		},
		defaultRepo: defaultRepo,
	}
}

Most of this information like workingDir, Kubectl context and default are not part of the latest.DeployConfig and determined in pkg/skaffold/runner/runner.go.

If, i use the same Deployer interface, it will look something like this for KubectlDeployer.

1. Add a new field manifests which will a list of k8sconfigs.

// KubectlDeployer deploys workflows using kubectl CLI.
type KubectlDeployer struct {
	*latest.KubectlDeploy

	workingDir  string
	kubectl     kubectl.CLI
	defaultRepo string
	manifests   []string
}

2. We will need 2 constructors
   1. for the init flow where all you have is list of potential config files relevant to the deploy config.
   2. the existing one which contains a lot for context for executing the kubectl deploy

// NewKubectlDeployer returns a new KubectlDeployer for a DeployConfig filled
// with the needed configuration for `kubectl apply`
func NewKubectlDeployer(workingDir string, cfg *latest.KubectlDeploy, kubeContext string, namespace string, defaultRepo string) *KubectlDeployer {
	return &KubectlDeployer{
		KubectlDeploy: cfg,
		workingDir:    workingDir,
		kubectl: kubectl.CLI{
			Namespace:   namespace,
			KubeContext: kubeContext,
			Flags:       cfg.Flags,
		},
		defaultRepo: defaultRepo,
	}
}

// A new method here do want to add a bunch of nil arguments in init flow when creating a new deployer for kubectl.
// NewKubectlDeployerWithManifests returns a new KubectlDeployer for a DeployConfig filled
// with the manifests file for intializing skaffold config.
func NewKubectlDeployerWithManifests(potentialConfigs []string) *KubectlDeployer {
	return &KubectlDeployer{
		manifests: potentialConfigs,
	}
}

The Deployer interface will be

// Deployer is the Deploy API of skaffold and responsible for deploying
// the build results to a Kubernetes cluster and generate skaffold deployment
// config for a repo.
type Deployer interface {
	Labels() map[string]string

	// Deploy should ensure that the build results are deployed to the Kubernetes
	// cluster.
	Deploy(context.Context, io.Writer, []build.Artifact, []Labeller) error

	// Init generates Init Config for skaffold configuration.
	Init() (*latest.DeployConfig, []string, error)
.... 
}

The init flow will now be

doInit() :
 ...
  deployer, err := getDeployer(potentialConfigs)
  ...
  deployConfig, images, err := deployer.Init()
}

func getDeployer(potentialConfigs []string) (deploy.Deployer, error) {
	// TODO(tejal29): this should return appropriate deployers.
	return deploy.NewKubectlDeployerWithManifests(potentialConfigs), nil
}

This looks like a smaller change and i like it better now.

[balopat]

Thank you so much for digging into this. I agree that keeping the first approach (separate Initializer interface and implementation) is cleaner.

[balopat]

I'm not sure why we closed this :)

[reddybhavaniprasad]

I am trying to run "skaffold init" for a docker-compose.yml file using the below command and it fails with "Invalid k8s yaml" error.

skaffold init --compose-file docker-compose.yml --verbosity='info'

Please find below the error:

time="2019-11-26T08:57:19Z" level=info msg="invalid k8s yaml charts/user-interface/Chart.yaml: decoding kubernetes yaml: Object 'Kind' is missing in 'apiVersion: v1\nappVersion: 0.0.84\ndescription: A Helm chart for Kubernetes\nname: user-interface\nversion: 0.0.84\n'"
time="2019-11-26T08:57:19Z" level=info msg="invalid k8s yaml charts/user-interface/templates/config-map.yaml: decoding kubernetes yaml: couldn't get version/kind; json parse error: invalid character '{' looking for beginning of object key string"
time="2019-11-26T08:57:19Z" level=info msg="invalid k8s yaml charts/user-interface/templates/deployment.yaml: decoding kubernetes yaml: couldn't get version/kind; json parse error: invalid character '{' looking for beginning of object key string"
time="2019-11-26T08:57:19Z" level=info msg="invalid k8s yaml charts/user-interface/templates/ingress.yaml: decoding kubernetes yaml: couldn't get version/kind; json parse error: invalid character '{' looking for beginning of object key string"
time="2019-11-26T08:57:19Z" level=info msg="invalid k8s yaml charts/user-interface/templates/ksvc.yaml: decoding kubernetes yaml: couldn't get version/kind; json parse error: invalid character '{' looking for beginning of object key string"
time="2019-11-26T08:57:19Z" level=info msg="invalid k8s yaml charts/user-interface/templates/service.yaml: decoding kubernetes yaml: yaml: line 11: could not find expected ':'"
time="2019-11-26T08:57:19Z" level=info msg="invalid k8s yaml charts/user-interface/templates/volume.yaml: decoding kubernetes yaml: couldn't get version/kind; json parse error: invalid character '{' looking for beginning of object key string"
time="2019-11-26T08:57:19Z" level=info msg="invalid k8s yaml charts/user-interface/templates/volumeclaim.yaml: decoding kubernetes yaml: couldn't get version/kind; json parse error: invalid character '{' looking for beginning of object key string"
time="2019-11-26T08:57:19Z" level=info msg="invalid k8s yaml charts/user-interface/values.yaml: decoding kubernetes yaml: Object 'Kind' is missing in '# Default values for helm.

As @tejal29 mentioned in the issue #1651 (comment), please share the skaffold init command with deploy type as helm if available or any alternative to resolve this issue.

[tstromberg]

I'm still not convinced that this is necessary, but would love to hear some feedback.

Narrowing the issue down to a single deployer.

[tejal29]

Closing this in favor of #3768