Include default template rules for packaging resources #609

mamachanko · 2022-09-18T14:54:41Z

What this PR does / why we need it:

This PR include default templateRules for kapp-controller's packaging resources; PackageRepository, PackageInstall and App. Since kapp and kapp-controller are companions it seems helpful.

In particular, the following templateRules are added to the defaults:

PackageRepository (according to spec)
- .spec.fetch.inline.pathsFrom[].secretRef
- .spec.fetch.inline.pathsFrom[].configMapRef
PackageInstall (according to spec)
- .spec.values[].secretRef
- .spec.cluster.kubeconfigSecretRef
App (according to spec)
- .spec.cluster.kubeconfigSecretRef
- .spec.fetch[].inline.pathsFrom[].secretRef
- .spec.fetch[].inline.pathsFrom[].configMapRef
- .spec.fetch[].imgpkgBundle.secretRef
- .spec.fetch[].http.secretRef
- .spec.fetch[].git.secretRef
- .spec.fetch[].helmChart.repository.secretRef
- .spec.template[].ytt.inline.pathsFrom[].secretRef
- .spec.template[].ytt.inline.pathsFrom[].configMapRef
- .spec.template[].ytt.valuesFrom[].secretRef
- .spec.template[].ytt.valuesFrom[].configMapRef
- .spec.template[].helmTemplate.valuesFrom[].secretRef
- .spec.template[].helmTemplate.valuesFrom[].configMapRef
- .spec.template[].cue.valuesFrom[].secretRef
- .spec.template[].sops.pgp.privateKeySecretRef

Which issue(s) this PR fixes:

Fixes #596

Does this PR introduce a user-facing change?

Include default template rules for _kapp-controller_'s `PackageRepository`, `PackageInstall` and `App` resources.

Questions for the maintainers:

Is this change wanted at all? From what I can tell, additional template rules are a non-invasive / non-breaking change. However, I can't intuit if this could have unwanted effects on kapp-controller.
After studying the code base, I think to understand that there are no (focused) tests for the default config in general and its template rules in particular. However, this change needs to be tested. Before I invest, I would love to know the maintainers recommendation for covering this. There are unit tests for versioned resources and e2e tests. I imagine tests for the default config to sit in between them, e.g. maybe a default_test.go which contains TestDefaultTemplateRules_{PackageRepository, PackageInstall, App}.

In the meantime, I have manually tested the new rules. See details in comment.

Additional Notes for your reviewer:

Review Checklist:

Follows the developer guidelines
Relevant tests are added or updated
Relevant docs in this repo added or updated
Relevant carvel.dev docs added or updated in a separate PR and there's
a link to that PR
Code is at least as readable and maintainable as it was before this
change

Additional documentation e.g., Proposal, usage docs, etc.:

mamachanko · 2022-09-18T14:57:14Z

(see PR description for Q about testing)

In the meantime, here's how I've manually tested this change so far.

#! test.yaml
---
apiVersion: packaging.carvel.dev/v1alpha1
kind: PackageRepository
metadata:
  name: test
spec:
  fetch:
    inline:
      pathsFrom:
        - secretRef:
            name: test-secret
        - configMapRef:
            name: test-configmap

---
apiVersion: packaging.carvel.dev/v1alpha1
kind: PackageInstall
metadata:
  name: test
spec:
  values:
    - secretRef:
        name: test-secret
  cluster:
    kubeconfigSecretRef:
      name: test-secret

---
apiVersion: kappctrl.k14s.io/v1alpha1
kind: App
metadata:
  name: test
spec:
  cluster:
    kubeconfigSecretRef:
      name: test-secret
  fetch:
    - inline:
        pathsFrom:
          - secretRef:
              name: test-secret
          - configMapRef:
              name: test-configmap
    - imgpkgBundle:
        secretRef:
          name: test-secret
    - http:
        secretRef:
          name: test-secret
    - git:
        secretRef:
          name: test-secret
    - helmChart:
        repository:
          secretRef:
            name: test-secret
  template:
    - ytt:
        inline:
          pathsFrom:
            - secretRef:
                name: test-secret
            - configMapRef:
                name: test-configmap
        valuesFrom:
          - secretRef:
              name: test-secret
          - configMapRef:
              name: test-configmap
    - helmTemplate:
        valuesFrom:
          - secretRef:
              name: test-secret
          - configMapRef:
              name: test-configmap
    - cue:
        valuesFrom:
          - secretRef:
              name: test-secret
          - configMapRef:
              name: test-configmap
    - sops:
        pgp:
          privateKeySecretRef:
            name: test-secret

---
apiVersion: v1
kind: Secret
metadata:
  name: test-secret
  annotations:
    kapp.k14s.io/versioned: ""
data:
  config.yml: ""

---
apiVersion: v1
kind: ConfigMap
metadata:
  name: test-configmap
  annotations:
    kapp.k14s.io/versioned: ""
data:
  config.yml: ""

go run cmd/kapp/kapp.go deploy --app test --yes --diff-changes --diff-run --file test.yaml

Target cluster 'https://127.0.0.1:56481' (nodes: kind-control-plane)

@@ create secret/test-secret-ver-1 (v1) namespace: default @@
      0 + apiVersion: v1
      1 + data:
      2 +   config.yml: <-- value not shown (#1)
      3 + kind: Secret
      4 + metadata:
      5 +   annotations:
      6 +     kapp.k14s.io/versioned: ""
      7 +   labels:
      8 +     kapp.k14s.io/app: "1663512989648583000"
      9 +     kapp.k14s.io/association: v1.2af780e8ffab3a405b9c89686418b58a
     10 +   name: test-secret-ver-1
     11 +   namespace: default
     12 +
@@ create configmap/test-configmap-ver-1 (v1) namespace: default @@
      0 + apiVersion: v1
      1 + data:
      2 +   config.yml: ""
      3 + kind: ConfigMap
      4 + metadata:
      5 +   annotations:
      6 +     kapp.k14s.io/versioned: ""
      7 +   labels:
      8 +     kapp.k14s.io/app: "1663512989648583000"
      9 +     kapp.k14s.io/association: v1.3b489d168caf3f033eb1d98e482fc052
     10 +   name: test-configmap-ver-1
     11 +   namespace: default
     12 +
@@ create packagerepository/test (packaging.carvel.dev/v1alpha1) namespace: default @@
      0 + apiVersion: packaging.carvel.dev/v1alpha1
      1 + kind: PackageRepository
      2 + metadata:
      3 +   labels:
      4 +     kapp.k14s.io/app: "1663512989648583000"
      5 +     kapp.k14s.io/association: v1.1d9d3c5a5fa754c7715d4707b9774382
      6 +   name: test
      7 +   namespace: default
      8 + spec:
      9 +   fetch:
     10 +     inline:
     11 +       pathsFrom:
     12 +       - secretRef:
     13 +           name: test-secret-ver-1
     14 +       - configMapRef:
     15 +           name: test-configmap-ver-1
     16 +
@@ create packageinstall/test (packaging.carvel.dev/v1alpha1) namespace: default @@
      0 + apiVersion: packaging.carvel.dev/v1alpha1
      1 + kind: PackageInstall
      2 + metadata:
      3 +   labels:
      4 +     kapp.k14s.io/app: "1663512989648583000"
      5 +     kapp.k14s.io/association: v1.8da39a73cfc4136f8ac556df26e520ea
      6 +   name: test
      7 +   namespace: default
      8 + spec:
      9 +   cluster:
     10 +     kubeconfigSecretRef:
     11 +       name: test-secret-ver-1
     12 +   values:
     13 +   - secretRef:
     14 +       name: test-secret-ver-1
     15 +
@@ create app/test (kappctrl.k14s.io/v1alpha1) namespace: default @@
      0 + apiVersion: kappctrl.k14s.io/v1alpha1
      1 + kind: App
      2 + metadata:
      3 +   labels:
      4 +     kapp.k14s.io/app: "1663512989648583000"
      5 +     kapp.k14s.io/association: v1.93248f12c712bc5436fdd56b8be5641d
      6 +   name: test
      7 +   namespace: default
      8 + spec:
      9 +   cluster:
     10 +     kubeconfigSecretRef:
     11 +       name: test-secret-ver-1
     12 +   fetch:
     13 +   - inline:
     14 +       pathsFrom:
     15 +       - secretRef:
     16 +           name: test-secret-ver-1
     17 +       - configMapRef:
     18 +           name: test-configmap-ver-1
     19 +   - imgpkgBundle:
     20 +       secretRef:
     21 +         name: test-secret-ver-1
     22 +   - http:
     23 +       secretRef:
     24 +         name: test-secret-ver-1
     25 +   - git:
     26 +       secretRef:
     27 +         name: test-secret-ver-1
     28 +   - helmChart:
     29 +       repository:
     30 +         secretRef:
     31 +           name: test-secret-ver-1
     32 +   template:
     33 +   - ytt:
     34 +       inline:
     35 +         pathsFrom:
     36 +         - secretRef:
     37 +             name: test-secret-ver-1
     38 +         - configMapRef:
     39 +             name: test-configmap-ver-1
     40 +       valuesFrom:
     41 +       - secretRef:
     42 +           name: test-secret-ver-1
     43 +       - configMapRef:
     44 +           name: test-configmap-ver-1
     45 +   - helmTemplate:
     46 +       valuesFrom:
     47 +       - secretRef:
     48 +           name: test-secret-ver-1
     49 +       - configMapRef:
     50 +           name: test-configmap-ver-1
     51 +   - cue:
     52 +       valuesFrom:
     53 +       - secretRef:
     54 +           name: test-secret-ver-1
     55 +       - configMapRef:
     56 +           name: test-configmap-ver-1
     57 +   - sops:
     58 +       pgp:
     59 +         privateKeySecretRef:
     60 +           name: test-secret-ver-1
     61 +

Changes

Namespace  Name                  Kind               Age  Op      Op st.  Wait to    Rs  Ri
default    test                  App                -    create  -       reconcile  -   -
^          test                  PackageInstall     -    create  -       reconcile  -   -
^          test                  PackageRepository  -    create  -       reconcile  -   -
^          test-configmap-ver-1  ConfigMap          -    create  -       reconcile  -   -
^          test-secret-ver-1     Secret             -    create  -       reconcile  -   -

Op:      5 create, 0 delete, 0 update, 0 noop, 0 exists
Wait to: 5 reconcile, 0 delete, 0 noop

Succeeded

100mik · 2022-09-18T17:47:13Z

Thanks for the PR! Gonna take a closer look at this first thing Monday ❤️

100mik · 2022-09-22T15:04:23Z

Hey @mamachanko ! So sorry I totally responded to this in my head. I verified that all Secret and ConfigMap references.
Would you be open to adding a test for this? I will look up and drop links to similar tests if that helps!

mamachanko · 2022-09-23T06:07:01Z

I verified that all Secret and ConfigMap references.

Nice. Thanks for going over them!

Would you be open to adding a test for this?

Yes, I'd be happy to. I will test a few limited cases first and check in with you to vet the approach before going through the full lot.

mamachanko · 2022-09-25T09:56:02Z

@100mik I won't be able to add tests right away, because time and obligations. There's no rush here, is that right?

In the meantime, if you are able to point me at existing tests, that'd be helpful. However, as far as I can tell, no tests exercise the default configuration's templating rules yet.

100mik · 2022-09-30T10:56:52Z

Hey! Absolutely no rush from our side.

Today, the only test we have for template rules is an E2E test. I think the example you shared above is perfect ! It can be captured in an E2E test in a similar manner (like the existing one).

It might be cool to figure out if we can have tabular unit tests for template rules. But that would definitely be a separate PR altogether 🤔

kumaritanushree · 2022-09-30T11:25:43Z

@100mik

I think the example you shared above is perfect ! It can be captured in an E2E test in a similar manner (like the existing one)

To have an e2e test with example shared by @mamachanko will require installation and deletion of kapp-controller as well, as CRDs of PackageInstall, App and PackageRepository will require to deploy these resources. I think installation of kapp-controller will be bit heavy in e2e test.
Alternative of this could be adding of CRDs of these resources in the manifest instead of using kapp-controller might be helpful.

100mik · 2022-09-30T11:28:26Z

Yep that makes sense, I think deploying the latest kapp-controller release while applying only CRDs using filters.

And we just need to check the diff, so we can set --wait=false while testing the template rules 🤔

mamachanko · 2022-10-10T15:00:02Z

@kumaritanushree @100mik Before committing to an E2E test scenario, I wanted to explore a more unit-style approach. After discovering all the required types and APIs I drafted a table-driven default_template_rules_test.go.

Currently, it only contains a single entry in its test table for demonstration. However, it can be easily extended with all remaining packaging-related - or in fact any - template rule test.

Before I invest and fill in the missing cases, would you be able to have a look and see if this makes sense to you? Do you like the level of test? Are the APIs and types used idiomatically? Is there are simpler way?

Ty! 🙇🏻

kumaritanushree · 2022-10-12T13:09:29Z

@kumaritanushree @100mik Before committing to an E2E test scenario, I wanted to explore a more unit-style approach. After discovering all the required types and APIs I drafted a table-driven default_template_rules_test.go.

@mamachanko I was also thinking to have unit test instead of E2E for this scenario. For template rules we already have E2E test via versioned resources. I am fine with this. 👍

mamachanko · 2022-10-13T07:39:18Z

@kumaritanushree @100mik Before committing to an E2E test scenario, I wanted to explore a more unit-style approach. After discovering all the required types and APIs I drafted a table-driven default_template_rules_test.go.

@mamachanko I was also thinking to have unit test instead of E2E for this scenario. For template rules we already have E2E test via versioned resources. I am fine with this. 👍

Thank you!

default_template_rules_test.go now tests the newly added template rules for kapp-controller's resources.

The PR is ready for review. 🔍

kumaritanushree · 2022-10-18T07:33:44Z

default_template_rules_test.go now tests the newly added template rules for kapp-controller's resources.

@mamachanko I think file name should be default_test.go so that if in future if required we can add test for other rules as well in the same file.
default_template_rules_test.go sounds like this will have only test for template rules.

mamachanko · 2022-10-19T12:42:49Z

default_template_rules_test.go now tests the newly added template rules for kapp-controller's resources.

@mamachanko I think file name should be default_test.go so that if in future if required we can add test for other rules as well in the same file. default_template_rules_test.go sounds like this will have only test for template rules.

@kumaritanushree Great point. Renamed the file. ✔️

kumaritanushree

LGTM
@mamachanko just one unresolved comment.

pkg/kapp/config/default_template_rules_test.go

In particular this includes template rules for kapp-controller's PackageRepository, PackageInstall and App CRs. A table-driven test for default template rules is added. It covers the newly added template rules.

praveenrewar

Thank you for the contribution @mamachanko!

vmwclabot added the cla-not-required label Sep 18, 2022

renuy requested review from 100mik, praveenrewar and cppforlife September 19, 2022 05:18

mamachanko force-pushed the packaging-template-rules branch from 7933755 to 4198e36 Compare October 10, 2022 14:53

mamachanko force-pushed the packaging-template-rules branch from 4198e36 to 47f175d Compare October 10, 2022 15:01

mamachanko force-pushed the packaging-template-rules branch from 47f175d to 67404d4 Compare October 13, 2022 07:16

praveenrewar requested a review from kumaritanushree October 17, 2022 16:05

mamachanko force-pushed the packaging-template-rules branch from 67404d4 to a680c62 Compare October 19, 2022 12:42

kumaritanushree approved these changes Oct 28, 2022

View reviewed changes

pkg/kapp/config/default_template_rules_test.go Outdated Show resolved Hide resolved

Include default template rules for packaging resources

54ec9e6

In particular this includes template rules for kapp-controller's PackageRepository, PackageInstall and App CRs. A table-driven test for default template rules is added. It covers the newly added template rules.

mamachanko force-pushed the packaging-template-rules branch from a680c62 to 54ec9e6 Compare October 28, 2022 07:26

praveenrewar approved these changes Oct 28, 2022

View reviewed changes

praveenrewar merged commit 3f3bc48 into carvel-dev:develop Oct 28, 2022

mamachanko deleted the packaging-template-rules branch October 28, 2022 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include default template rules for packaging resources #609

Include default template rules for packaging resources #609

mamachanko commented Sep 18, 2022 •

edited

Loading

mamachanko commented Sep 18, 2022

100mik commented Sep 18, 2022

100mik commented Sep 22, 2022 •

edited

Loading

mamachanko commented Sep 23, 2022

mamachanko commented Sep 25, 2022

100mik commented Sep 30, 2022

kumaritanushree commented Sep 30, 2022

100mik commented Sep 30, 2022

mamachanko commented Oct 10, 2022

kumaritanushree commented Oct 12, 2022 •

edited

Loading

mamachanko commented Oct 13, 2022

kumaritanushree commented Oct 18, 2022

mamachanko commented Oct 19, 2022

kumaritanushree left a comment

praveenrewar left a comment

Include default template rules for packaging resources #609

Include default template rules for packaging resources #609

Conversation

mamachanko commented Sep 18, 2022 • edited Loading

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Questions for the maintainers:

Additional Notes for your reviewer:

Review Checklist:

Additional documentation e.g., Proposal, usage docs, etc.:

mamachanko commented Sep 18, 2022

100mik commented Sep 18, 2022

100mik commented Sep 22, 2022 • edited Loading

mamachanko commented Sep 23, 2022

mamachanko commented Sep 25, 2022

100mik commented Sep 30, 2022

kumaritanushree commented Sep 30, 2022

100mik commented Sep 30, 2022

mamachanko commented Oct 10, 2022

kumaritanushree commented Oct 12, 2022 • edited Loading

mamachanko commented Oct 13, 2022

kumaritanushree commented Oct 18, 2022

mamachanko commented Oct 19, 2022

kumaritanushree left a comment

Choose a reason for hiding this comment

praveenrewar left a comment

Choose a reason for hiding this comment

mamachanko commented Sep 18, 2022 •

edited

Loading

100mik commented Sep 22, 2022 •

edited

Loading

kumaritanushree commented Oct 12, 2022 •

edited

Loading