Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feature: Automatic Creation of 'vars:' and 'varReferences:' sections #1217

Closed
wants to merge 2 commits into from

Conversation

jbrette
Copy link
Contributor

@jbrette jbrette commented Jun 20, 2019

The description of the goal for this PR is available at: issue

The main idea is to scan the ResMap
in order to create the varand varreference

Those var and varrefence get then merge into the accumulator as if a user had written those my hand in the kustomization.yaml and kustomizeconfig.yaml: merge

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 20, 2019
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 20, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 20, 2019
@jbrette jbrette changed the title Automatic Creation of vars: and varRerences: section Automatic Creation of 'vars:' and 'varReferences:' sections Jun 20, 2019
@jbrette jbrette force-pushed the autov branch 2 times, most recently from 00c60f5 to 7eb747a Compare June 20, 2019 18:44
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 20, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 20, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 20, 2019
@jbrette
Copy link
Contributor Author

jbrette commented Jun 20, 2019

/assign @monopole

@k8s-ci-robot
Copy link
Contributor

[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:

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

@jbrette
Copy link
Contributor Author

jbrette commented Sep 30, 2019

@jbrette Out of curiosity, is there a reason you've not expanded this feature to support namespaces?

Can you give us an example. The autovar feature works across namespaces.

@tkellen
Copy link
Contributor

tkellen commented Sep 30, 2019

@jbrette Out of curiosity, is there a reason you've not expanded this feature to support namespaces?

Can you give us an example. The autovar feature works across namespaces.

Yes of course! We spoke about this a few months back. A minimal test case of the failure is here:
#1316 (comment)

For more context, I have one file that contains all environment specific identifiers that is generated at the time the cluster is created (by Terraform). Here is a sample:

# GENERATED BY TERRAFORM
apiVersion: v1
kind: ConfigMap
metadata:
  name: environment
data:
  hostname: <OMITTED>
  rds_ci_hostname: <OMITTED>
  rds_statestore_hostname: <OMITTED>
  kiam_server_sudosu_role: <OMITTED>
  secret_prefix: <OMITTED>
  ci_secret_prefix: <OMITTED>
  app_secret_prefix: <OMITTED>
  rds_secret_prefix: <OMITTED>
  aws_region: us-east-1

Sibling to this file I have the following:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - environment.yml

...every single kustomization file in my configuration references this common base.

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: monitoring
resources:
  - ../../config/
  - ../../../../../../../shared/kubernetes/monitoring/loki
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: kube-system
resources:
  - ../../config
  - ../../../../../../shared/kubernetes/kiam/

If I apply these individually everything is fine. However, I do have one master kustomization.yml I'd like to be able to apply that I cannot (due to the failure shared in the link above).

# This file is here for the convenience of building multiple configurations with
# a single command. No customizations should be placed here as they will not be
# available if/when projects are configured independently.
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - bootstrap.yml
  - manifests/cert-manager
  - manifests/concourse
  - manifests/dns-autoscaler
  - manifests/ingress-nginx
  - manifests/kiam
  - manifests/monitoring
  - manifests/service-catalog

It makes sense to me that references to $(ConfigMap.environment.data.<field>) would fail because it is unable to disambiguate between each that appear, but each is exactly the same save for the namespace field. It seems like this sort of conflict should be self-resolving?

@jbrette jbrette force-pushed the autov branch 2 times, most recently from 63e1b6f to 9125aae Compare October 2, 2019 22:26
@jbrette jbrette force-pushed the autov branch 4 times, most recently from a6a6d4c to 89a50b7 Compare October 11, 2019 00:31
@jbrette jbrette force-pushed the autov branch 6 times, most recently from 34cba32 to 762bd6d Compare October 13, 2019 02:39
- Makefile needs to be updated after change in kustomize organization
- Remove mdrip, blackfriday from kustomize dependencies
- Improve resmapscanner.go
- Update unit tests associated with feature.
- Harden variable pattern detection
- Sort FieldSpec using Path instead of Gvk.
- Adapt to new VarSet structure
- Adapt to v3 code

AutoVar: Add Test demonstrating Mixin and Autovar
AutoVar: Add AbsorbRefVarName to optimize lookup of variable during MergeVars
AutoVar: Various cleanup around the autovar feature
AutoVar: Manual vs Automatic Variables conflict detection still needs improvments
NameValue: Allow name=value to look for specific var field
@jbrette jbrette changed the title Automatic Creation of 'vars:' and 'varReferences:' sections Feature: Automatic Creation of 'vars:' and 'varReferences:' sections Oct 14, 2019
@jbrette
Copy link
Contributor Author

jbrette commented Oct 15, 2019

This PR was only doing automatically what user have to painfully do by hand. Since it was specified in will never be merged, closing it and keeping private.

@jbrette jbrette closed this Oct 15, 2019
@jonathanunderwood
Copy link

This is a really disappointing outcome.

@oboukili
Copy link

So much work and energy had been put into this, that's really sad.

@jbrette
Copy link
Contributor Author

jbrette commented Oct 15, 2019

The code is not lost for our project and will stay in our fork. The design of this PR is to create whatever complex structure kustomize uses at the time. (currently varRef and var). Once the Replacement PR is merged, we will only have to adapt the autovar feature. Indeed that Replacement PR still forces the user to manually create complex fieldspecs in the kustomization.yaml.

Actually from a selfish point of view, it means that our project does not have to keep the PR small anymore or rebase to allow easier merging. With the four true features PR (diamond, inline, autovar and transformers), we unfortunatly end up with a fork of kustomize, but a least that fork gets everything much more simplier for our users (and still uses the same kustomization.yaml as the official kustomize). The real game changer in the fork/nofork debat was the handling of SMP instead JMP for CRD by kustomize (CRD are everywhere in the new applications: Istio, Argo, Prometheus, OpenShift, Cluster-API,.) .

Still fork is able to address 90% of the issues raised, some of them having been opened more than a year check here

@jonathanunderwood
Copy link

@jbrette what is the "Replacement PR" you refer to?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants