kptdev / kpt

Automate Kubernetes Configuration Editing
https://kpt.dev
Apache License 2.0
1.68k stars 226 forks source link

`apply-setters` will not apply setters to otherwise valid `kustomize` yaml (kpt v1) #2538

Open Pitta opened 2 years ago

Pitta commented 2 years ago

Expected behavior

Setters get applied to any valid *.yaml that is valid inside of the k8s tooling space, like kustomize

Actual behavior

Stand alone kustomize patches seem to be invalid according kpt fn render and does not apply setters as expected.

Information

I have a very basic kustomize directory structure that I'm trying to make a kpt pkg....

.
|-- Kptfile
|-- README.md
|-- kustomization.yaml
|-- patches
|   `-- nodegroup.yaml
`-- setters.yaml

This pkg is just a kustomize component. The only "resource" is a yaml based json patch because it is meant to be a reusable pkg that results is an array item. (EKS Nodegroups). An example from the kustomize docs detailing this particular use case/pattern can be found here: https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/patches/#patch-using-path-json6902

The yaml for patches/nodegroup.yaml starts like this:

---
- op: add
  path: /managedNodeGroups/-
  value:
    <the nodegroup definition>

This renders in kustomize perfectly when used with a base ClusterConfig, even when I have multiple copies of this pkg.

The last step is to make sure KPT setters work as expected. I have a setters.yaml that worked great when the nodegroup was a proper stand alone resource, but when defined in this patch yaml style for kustomize, KPT throws this error when running kpt fn render :

Package "nodegroup-a":
[RUNNING] "gcr.io/kpt-fn/apply-setters:v0.2"
[FAIL] "gcr.io/kpt-fn/apply-setters:v0.2" in 0s
Error: input resource list must contain only KRM resources: patches/nodegroup.yaml: resource must have `apiVersion`

Steps to reproduce the behavior

  1. Create a valid kustomize stand alone "Json6902" defined in yaml. (refer to the kustomize docs here
  2. add some setter comments to the file
  3. attempt to apply a value to a setter

Edit

These stand alone yaml patches existing in a pkg repo also seem to be preventing kpt from pulling/updating the package at all with a similar error.

droot commented 2 years ago

@Pitta kpt assumes all resources and inputs to have a valid GVK and enforces that invariant before running fn render. In this case, we are running into an edge case here where patches/nodegroup.yaml (the json6902 patch) is not a valid KRM resource even though it is a valid input for apply-setters.

cc: @phanimarupaka This is related to our discussion on supporting bare-sequence-nodes.

Pitta commented 2 years ago

Yeah. It causes problems with v1 when these files are even included in a pkg and trying to kpt pkg get.

Id say fine, except that this is totally valid for kustomize, and its part of eksctl. Feels like this should be supported.

We can work around it by doing the patch inline in the kustomization.yaml file itself, but its uglier and can get messy with larger patches. I find it cleaner to separate into files and reference to them. --- This is now false. kpt seems to just ignore kustomization.yaml files alltogether.

The inline path looks like this, and is not seen by kpt it seems:

apiVersion: kustomize.config.k8s.io/v1alpha1
kind: Component
patches:
- patch: |-
    - op: add
      path: /managedNodeGroups/-
      value:
        name: system # kpt-set: ${eks.nodegroup.name}
droot commented 2 years ago

@Pitta catching up on the issue:

The inline path looks like this, and is not seen by kpt it seems:

The main issue here is that apply-setters doesn't work with a multi-line values. There are some technical issues in supporting that. @phanimarupaka probably knows better about the issue with that.

if you can share more details about the parent package where this pkg is being used, I might be able to suggest kpt idiomatic way of doing it.

droot commented 2 years ago

I think in your parent package, you should have a kubernetes cluster type (where a nodegroup needs to be added/deleted etc.), so having a function such as add-nodegroup(additive version) or ensure-nodegroups (idempotent version) and calling it in the parent package for the corresponding cluster might be more idiomatic.

I will be happy to explain it in a call or something if it's not clear here.

Pitta commented 2 years ago

The main issue here is that apply-setters doesn't work with a multi-line values.

I don't think this assertion in this case is correct. I can have the same multiline value in the kustomize file directly, and it works "fine". Setters are still ignored, but the package doesn't fail to kpt kpt get like it does with the files stand alone.

Pitta commented 2 years ago

These stand alone yaml patches existing in a pkg repo also seem to be preventing kpt from pulling/updating the package at all with a similar error. I am not sure if making a separate issue is appropriate since the seems to have the same root cause.

Edit: If i change the extention to .patch the error goes away, and seems to cause kpt ignore it

Pitta commented 2 years ago

I think in your parent package, you should have a kubernetes cluster type (where a nodegroup needs to be added/deleted etc.), so having a function such as add-nodegroup(additive version) or ensure-nodegroups (idempotent version) and calling it in the parent package for the corresponding cluster might be more idiomatic.

I will be happy to explain it in a call or something if it's not clear here.

For this specific scenario, I am working with an EKS cluster parent pkg, and using this pkg to add additional nodegroups to the cluster, as needed. They have to be done this way because EKS decided to use an array for all the nodegroups in the cluster vs a separate resource type, and this is the best way to handle that with kustomize that I have found.

droot commented 2 years ago

Edit: If i change the extention to .patch the error goes away, and seems to cause kpt ignore it

Yes, files with non (yaml or json) extensions are ignored by kpt.

For this specific scenario, I am working with an EKS cluster parent pkg, and using this pkg to add additional nodegroups to the cluster, as needed. They have to be done this way because EKS decided to use an array for all the nodegroups in the cluster vs a separate resource type, and this is the best way to handle that with kustomize that I have found.

Ack.

Pitta commented 2 years ago

It is unclear on what the triaged tag is for on this issuie.