Feature: Diamond import of base folders

[jbrette]

This PR provides more possibilities to the way the user organize his files and folders.

First commit is for the code change itself:

• This situation happens in project which try to aggregate multiple
  components in different folders sharing the same base folder.
• This commit demonstrates how a common resource can be included
  across different components.
• Address conflicting bases by converting resources into patches
• Improve variables diamond conflicts detection

Second commit is for associated tests:

1. Test simple import of common resources.
2. Test simple import of common variables.
3. Test simple import of variables without their target
   resources in the parent folders.
4. Test that the algorithm is reporting proper errors
   in case of not supported diamond import of resources.
5. Test that the algorithm is reporting proper errors
   in case of not supported diamond import of variables.
6. Update errors messages in existing tests.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jbrette
To complete the pull request process, please assign monopole
You can assign the PR to them by writing /assign @monopole in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jbrette]

/assign @monopole

[Liujingfang1]

@jbrette Thank you for coming up with this change and many tests. I took a look at it, when detecting conflicts, it creates patches and applied to the target object. By default, this change makes Kustomize smart on resolving conflicts. However, it may not always be the expected behavior. For some use case, your want to compose the resources; for other use cases, you have multiple resources and don't want to merge them. Due to some user errors, those resources happen to be the same ID. kustomize build gives the output, but not the expected one.
So it's better to let user choose and set what behavior they want kustomize to be when dealing with ID conflicts. There are several choices in #1292, which allow users to specify it. For example, you can add a new field called resourcesToMerge or add more flexible source struct, which supports three different behavior create, replace, merge. No matter which choice we choose, the implementation would have a lot in common with this PR. Would you like to contribute to @1292?

[jbrette]

@monopole @Liujingfang1

I'm always interested in contributing but I'm not sure that 1292 is addressing the real concern which is simplification and maintenance of the kustomization.yaml.

It seems that their is a need to use folders to simplify the kustomization.yaml, have smaller ones... The patches section is already, at least in our case too long to be maintained properly. We have to generate the kustomization.yaml. Using folders and kustomization.yaml without changing the name prefix is the simplifying the process. This is what this PR achieves. Also this PR goes a real long way to not do the wrong thing, by forcing the algorithm to go through the conflictdetector instead of just failing.

Still this PR is not incompatible with 1292, I think they complements each other. If we get the conflictdetector to read the new section of the kustomization.yaml, then the user would always have a way to drive the merge behavior if he wants to or has to (because of merge conflict).

Please have a look at the autovar PR is has been done in the same spirit: This allows a really big simplification of the kustomization.yaml (we were able to reduce thousand of line) but the user is still in charge. Whatever he added to the kustomizeconfig.yaml varReference and kustomization.yaml is used. If an conflict happen between the automatic algorithm and the configuration added by the auto, the automatic stuff is trashed to preserve the user input.

[Liujingfang1]

It seems that their is a need to use folders to simplify the kustomization.yaml, have smaller ones... The patches section is already, at least in our case too long to be maintained properly. We have to generate the kustomization.yaml. Using folders and kustomization.yaml without changing the name prefix is the simplifying the process. This is what this PR achieves. Also this PR goes a real long way to not do the wrong thing, by forcing the algorithm to go through the conflictdetector instead of just failing.

We recommend to use small patches, e.g. one patch for changing one thing. The kustomization.yaml is not necessary to be small. If users would like to keep it small, they need to use many number of kustomization.yaml. In all, it may not do better than allowing non small size of kustomization.yaml. On the other side, if you put all patch files inside one kustomization.yaml, it is straightforward which patches are applied.

To me it is a need of reusing patches for different overlays and allowing singleton from different overlays. Allowing reusing of patches is solved by relaxing the loader restriction. #1292 is to solve the other need, merging resources from different overlays so that kustomize makes single objects.

Still this PR is not incompatible with 1292, I think they complements each other. If we get the conflictdetector to read the new section of the kustomization.yaml, then the user would always have a way to drive the merge behavior if he wants to or has to (because of merge conflict).

Let's not change the current behavior of Kustomize. We can make the merging behavior go into a new field.

Please have a look at the autovar PR is has been done in the same spirit: This allows a really big simplification of the kustomization.yaml (we were able to reduce thousand of line) but the user is still in charge. Whatever he added to the kustomizeconfig.yaml varReference and kustomization.yaml is used. If an conflict happen between the automatic algorithm and the configuration added by the auto, the automatic stuff is trashed to preserve the user input.

Will take a look.

[tkellen]

Thank you @jbrette! I am using your branch to great effect in my project. I think I have run into an edge case that perhaps hasn't been considered. Can you weigh in on if this config should be expected to work?

mkdir test
cd test
mkdir -p projects/foo/manifests projects/bar/manifests environment
printf domain.com > environment/domain
printf dev > environment/name
printf -branch > environment/branch
cat <<EOF > environment/kustomization.yml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
configMapGenerator:
  - name: environment
    files:
      - name
      - domain
      - branch
vars:
  - name: ENV
    objref:
      apiVersion: v1
      kind: ConfigMap
      name: environment
    fieldref:
      fieldpath: data.name
  - name: DOMAIN
    objref:
      apiVersion: v1
      kind: ConfigMap
      name: environment
    fieldref:
      fieldpath: data.domain
  - name: BRANCH
    objref:
      apiVersion: v1
      kind: ConfigMap
      name: environment
    fieldref:
      fieldpath: data.branch
generatorOptions:
  disableNameSuffixHash: true
EOF
cat <<EOF > projects/foo/kustomization.yml
namespace: foo
resources:
  - ../../environment
  - manifests/ingress.yml
EOF
cat <<EOF > projects/bar/kustomization.yml
namespace: bar
resources:
  - ../../environment
  - manifests/ingress.yml
EOF
cat <<'EOF' > projects/bar/manifests/ingress.yml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: bar
spec:
  rules:
    - host: bar$(BRANCH).$(ENV).$(DOMAIN)
      http:
        paths:
        - backend:
            serviceName: bar
            servicePort: http
EOF
cat <<'EOF' > projects/foo/manifests/ingress.yml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: foo
spec:
  rules:
    - host: foo$(BRANCH).$(ENV).$(DOMAIN)
      http:
        paths:
        - backend:
            serviceName: foo
            servicePort: http
EOF
cat <<EOF > kustomization.yml
resources:
  - projects/foo
  - projects/bar
EOF
tree
kustomize build .

Here is the output:

➜ tree
.
├── environment
│   ├── branch
│   ├── domain
│   ├── kustomization.yml
│   └── name
├── kustomization.yml
└── projects
    ├── bar
    │   ├── kustomization.yml
    │   └── manifests
    │       └── ingress.yml
    └── foo
        ├── kustomization.yml
        └── manifests
            └── ingress.yml
➜ kustomize build .
Error: accumulating resources: recursed merging from path 'projects/bar': found 2 resId matches for var {BRANCH ~G_v1_ConfigMap {data.branch}} (unable to disambiguate)

If you adjust both projects to be in the same namespace this works fine.

[jbrette]

@tkellen Your comments have three things in common with what our project found:

• You get the ambiguous variable (I think) because the variable description doesn't/can't contain the namespace [PR)(https://github.com/ResId.Equals usable for VariableRef. #1327)
• Using the diamond import of common object PR
• How to simplify the usage variables. We use a CRD as place holder for our values because it is much easier to filter them out from the output especially if you use kustomize build . -o .
  Combined with autovariable declaration PR

Our kustomization.yaml is simple (no variable declaration)

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- values.yaml

The values.yaml is a simple, whatever syntax you want as long as it respect the CRD structure and kustomize:

apiVersion: v1
kind: Values
metadata:
  name: shared
spec:
  env: dev
  domain: domain.com
  branch: -branch

Your ingress changed to use the autovar feature:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: foo
spec:
  rules:
    - host: foo$(Values.shared.spec.branch).$(Values.shared.spec.env).$(Values.shared.spec.domain)
      http:
        paths:
        - backend:
            serviceName: foo
            servicePort: http

The result of the kustomize build

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: bar
spec:
  rules:
  - host: bar-branch.dev.domain.com
    http:
      paths:
      - backend:
          serviceName: bar
          servicePort: http
---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: foo
spec:
  rules:
  - host: foo-branch.dev.domain.com
    http:
      paths:
      - backend:
          serviceName: foo
          servicePort: http
---
apiVersion: v1
kind: Values
metadata:
  name: shared
spec:
  branch: -branch
  domain: domain.com
  env: dev

Your example has been saved here for right as simple yamls
I'll convert it go like we do in pkg/target. We are still circulating the autovar feature to see what people thinks.
Anyway all those PR are merged together in the allinone branch.

[tkellen]

Thanks for the thorough reply @jbrette! I am running the allinone branch locally now and I am able to confirm that as long as all bases which reference the config are in the same namespace, this works. If they are NOT in the same namespace, this is the output:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: bar
  namespace: bar
spec:
  rules:
  - host: bar$(Values.shared.spec.branch).$(Values.shared.spec.env).$(Values.shared.spec.domain)
    http:
      paths:
      - backend:
          serviceName: bar
          servicePort: http
---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: foo
  namespace: foo
spec:
  rules:
  - host: foo$(Values.shared.spec.branch).$(Values.shared.spec.env).$(Values.shared.spec.domain)
    http:
      paths:
      - backend:
          serviceName: foo
          servicePort: http
---
apiVersion: v1
kind: Values
metadata:
  name: shared
  namespace: bar
spec:
  branch: -branch
  domain: domain.com
  env: dev
---
apiVersion: v1
kind: Values
metadata:
  name: shared
  namespace: foo
spec:
  branch: -branch
  domain: domain.com
  env: dev

I presume this is because the $(Values...) lookup isn't taking the namespace into account?

[jbrette]

@tkellen The name of the variable is not currently the blocking aspect. The autovar feature is leveraging the current var feature but help the user by not having to manually declare the variables and varReference. The real issue your are seeing right now is linked to the declaration of the var. You see that their is currently no namespace field to workaround the "ambiguous" variable. See issue

A possible temporary workaround to that issue is:

mkdir outputdir
kustomize build ./projects/foo -o outputdir
kustomize build ./projects/bar -o outputdir

[jbrette]

@tkellen I did not realized we already fixed the simple version of your use case: Check fixed two months ago and since one is using configmap

So we are able to deal with namespace properly. Will try the more complex version later

[tkellen]

@jbrette I've been using the simple version for some time now very successfully, thank you. Just to say it out loud, I'm not using the Values/crd approach because it relies on a number of k8s/kustomize concepts I would prefer my team not have to understand. Thank you again!

[jbrette]

This feature PR was doing the equivalent of git automatic merge for resources and variables where do not causing any conflicts. Solved quite a lot of actual use cases. But it will never be merged because it does not involve the user manually creating entries in the kustomization.yaml to describe how to perform the merge. Closing it.

[tkellen]

Thank you @jbrette -- even though these features didn't land, I appreciate your work!