Support for conditionally enabling of alpha-level CRDs #462

dmitri-d · 2023-07-15T03:39:02Z

If CRD doesn't contain any alpha version of resources, produced CRD templates are almost identical to CRD manifests in crds/ directory.

A given alpha-version CRD can be enabled by adding its GVK to experimental.api helm chart variable.
If there's a single alpha version resource in a CRD, the whole thing is rendered conditionally:

{{- if has "waypointlifecyclemanagers.admin.gloo.solo.io/v2alpha1" $.Values.enabledExperimentalApi }}
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  annotations:
    crd.solo.io/version: 2.4.0
  labels:
    app: gloo-mesh-apis
    app.kubernetes.io/name: gloo-mesh-apis
  name: waypointlifecyclemanagers.admin.gloo.solo.io
spec:
  group: admin.gloo.solo.io
  names:
    categories:
    - solo-io
    kind: WaypointLifecycleManager
    listKind: WaypointLifecycleManagerList
    plural: waypointlifecyclemanagers
    singular: waypointlifecyclemanager
  scope: Namespaced
  versions:
  - name: v2alpha1
    schema:
      openAPIV3Schema:
        properties:
          spec:
...
    served: true
    storage: true
    subresources:
      status: {}
{{- end  }}

If a CRD contains multiple versions and one of them is alpha, then only that version can be rendered conditionally:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  annotations:
    crd.solo.io/version: 2.4.0
  labels:
    app: gloo-mesh-apis
    app.kubernetes.io/name: gloo-mesh-apis
  name: workspaces.admin.gloo.solo.io
spec:
  group: admin.gloo.solo.io
  names:
    categories:
    - solo-io
    kind: Workspace
    listKind: WorkspaceList
    plural: workspaces
    shortNames:
    - ws
    singular: workspace
  scope: Namespaced
  versions:
  - name: v2
    schema:
      openAPIV3Schema:
...
    served: true
    storage: true
    subresources:
      status: {}
  {{- if has "workspaces.admin.gloo.solo.io/v2alpha1" $.Values.enabledExperimentalApi }}
  - name: v2alpha1
    schema:
      openAPIV3Schema:
        properties:
          spec:
...
    served: true
    storage: false
    subresources:
      status: {}
  {{- end }}
---

This partially resolves https://github.com/solo-io/gloo-mesh-enterprise/issues/9019
BOT NOTES:
resolves https://github.com/solo-io/gloo-mesh-enterprise/issues/9019

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

solo-changelog-bot · 2023-07-27T04:09:03Z

Issues linked to changelog:
https://github.com/solo-io/gloo-mesh-enterprise/issues/9019

dmitri-d · 2023-07-27T04:15:37Z

Most changes here to support rendering of codegen/templates/manifests/crd.yamltmpl.

A couple of small questions (maybe) to resolve: https://github.com/solo-io/skv2/pull/462/files#diff-0edd4bd3f13b4db965a1337273d05e142d5c93e63559722098c88c9873458997R148

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

…hered groups Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

josh-pritchard

Looks really good, a few small nits but this is exactly what we needed. Nice work.

codegen/kuberesource/crd.go

codegen/render/manifests_renderer.go

josh-pritchard · 2023-07-28T14:52:19Z

codegen/templates/manifests/crd.yamltmpl

+  [[- if and (and (gt (len $crd.Spec.Versions) 1) (string_contains $version.Name "alpha")) (should_not_skip $crd.Spec.Group $version.Name $.ShouldSkip) ]]
+  {{- end }}
+  [[- end ]]
+  [[- end ]]
+[[- if and (and (lt (len $crd.Spec.Versions) 2) (string_contains (index $crd.Spec.Versions 0).Name "alpha")) (should_not_skip $crd.Spec.Group (index $crd.Spec.Versions 0).Name $.ShouldSkip) ]]


Are these just to make sure new-lines are created when needed?

Not sure what exactly you are referring to. If it's the conditional on the line above, it's to conditionally render {{- end }} that corresponds to the conditional here: https://github.com/solo-io/skv2/pull/462/files/acf693e11fd37032d1e9813a85d5e6888a1c0a4d#diff-033b9d4512e2683aff9f19928ec94eaaa06e46cbfcede987461da2079459436dR3

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

ilackarms · 2023-07-28T19:33:31Z

codegen/cmd.go

 	}

-	manifests, err := render.RenderManifests(c.AppName, c.ManifestRoot, c.ProtoDir, protoOpts, groupOptions, grp)
+	manifests, err := render.RenderManifests(c.AppName, c.ManifestRoot, c.ProtoDir, protoOpts, groupOptions, grps)


we sohuld just move RenderManifests out rather than changing the whole function signature for this one line

Not exactly sure what you mean. RenderManifests needs all groups now, as it combines multiple versions of the same group into one manifest (and multiple versions of the same resource into a single crd).

Sodman

Overall LGTM, couple questions and nits.

codegen/cmd.go

Sodman · 2023-07-28T18:31:06Z

codegen/kuberesource/crd.go

-			resource,
-			group.OpenApiSchemas,
-		)
+	groups []*model.Group,


Implementation looks fine to me, but this function is getting a little complex, we might want to break into a couple smaller simpler funcs?

cleaned it up a little bit.

codegen/kuberesource/crd.go

codegen/model/resource.go

Sodman · 2023-07-28T19:28:17Z

codegen/render/manifests_renderer.go

+	if err != nil {
+		return OutFile{}, err
+	}
+	return files[0], nil


Files always len 1? Should we check?

we only give defaultManifestRenderer.renderCoreTemplates one template to render; if there's an error it will return before this line. So, yes, if it got here, there is one item in []OutFile. I'll add a comment.

Sodman · 2023-07-28T19:33:05Z

codegen/test/name_override_chart/crds/things.test.io_crds.yaml

  labels:
    app: ""
    app.kubernetes.io/name: ""
-  name: paints.things.test.io
+  name: clusterresources.things.test.io


Did these changes result in the ordering of CRDs change?

Yes. There are a few interim maps: for grouping of groups, which also affected ordering of versions in CRDs. To make those stable, I added sorting in both places:

https://github.com/solo-io/skv2/pull/462/files#diff-0edd4bd3f13b4db965a1337273d05e142d5c93e63559722098c88c9873458997R37

https://github.com/solo-io/skv2/pull/462/files#diff-0edd4bd3f13b4db965a1337273d05e142d5c93e63559722098c88c9873458997R42

Sodman · 2023-07-28T19:34:05Z

codegen/render/manifests_renderer.go

+	renderer.left = "[["
+	renderer.right = "]]"


To confirm, this is because the template we are rendering will itself contain template syntax? (i.e. codegen/templates/manifests/crd.yamltmpl)?

yep. [[ and ]] are for the template code that executes during code-generation pass, while {{ and }} for when template is rendered by helm. We do the same in chart_renderer (which is where I got the idea): https://github.com/solo-io/skv2/blob/master/codegen/render/chart_renderer.go#L47

changelog/v0.31.3/conditional_crd_rendering.yaml

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

codegen/templates/manifests/crd.yamltmpl

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

…bled alpha API Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

…d-crd-install

This reverts commit ac641d9.

Support for enabling/disabling of alpha-level CRDs

407decd

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

dmitri-d marked this pull request as draft July 15, 2023 03:39

dmitri-d requested a review from josh-pritchard July 15, 2023 03:40

dmitri-d added 8 commits July 17, 2023 17:07

Fixed crd template

aba416a

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Groups are matched with correct file descriptors now

ba8158d

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Added 'Grandfathered' field to Group model

804ea88

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Merge remote-tracking branch 'origin/master' into dmitrid-crd-install

322e66c

Fixing test failures

16c6794

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Tests are passing

fd01b15

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Added more tests

ff3b4c3

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Test fixes

2ad44af

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

dmitri-d marked this pull request as ready for review July 27, 2023 03:49

dmitri-d added 3 commits July 26, 2023 20:53

Added a changelog entry

b947347

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Respect 'RenderManifests' Group field

08dfd42

Moved changelog to v0.31.3

e6a958c

dmitri-d requested a review from Sodman July 27, 2023 04:09

dmitri-d added 2 commits July 26, 2023 21:35

Regenerated code

77ee7d2

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

De-flaked a test

13d41d9

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

dmitri-d self-assigned this Jul 27, 2023

dmitri-d added 3 commits July 27, 2023 19:08

Added support for skipping conditional rendering of CRDs for grandfat…

ce5b6e4

…hered groups Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

De-flaked a test

8ff87ad

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Made ordering of crds in a group deterministic

acf693e

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

josh-pritchard reviewed Jul 28, 2023

View reviewed changes

codegen/kuberesource/crd.go Show resolved Hide resolved

codegen/render/manifests_renderer.go Outdated Show resolved Hide resolved

josh-pritchard reviewed Jul 28, 2023

View reviewed changes

dmitri-d added 2 commits July 28, 2023 09:06

Responded to feedback

697c2af

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Fixed an issue in template rendering

49c4f85

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

ilackarms reviewed Jul 28, 2023

View reviewed changes

Sodman reviewed Jul 28, 2023

View reviewed changes

jenshu reviewed Jul 28, 2023

View reviewed changes

changelog/v0.31.3/conditional_crd_rendering.yaml Outdated Show resolved Hide resolved

dmitri-d added 2 commits July 28, 2023 13:11

Responded to feedback

bf17709

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

changed changelog entry to 'BREAKING_CHANGE'

7c87686

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

ilackarms reviewed Jul 28, 2023

View reviewed changes

codegen/templates/manifests/crd.yamltmpl Outdated Show resolved Hide resolved

dmitri-d and others added 9 commits July 28, 2023 13:29

cleaned up crd.CustomResourceDefinitions

774e479

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Made configurable the name for the flag that contains the list of ena…

458d999

…bled alpha API Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Added validation that CRD templates can be rendered

9892a1a

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Fixed an issue with crd template rendering

628ec34

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Adding changelog file to new location

32b91d0

Deleting changelog file from old location

9bc78c7

Merge remote-tracking branch 'origin/master' into dmitrid-crd-install

fb84047

Updated version to v0.33.0

c5ad04b

Signed-off-by: Dmitri Dolguikh <dmitri@appliedlogic.ca>

Merge remote-tracking branch 'origin/dmitrid-crd-install' into dmitri…

51e0190

…d-crd-install

ilackarms approved these changes Jul 31, 2023

View reviewed changes

josh-pritchard approved these changes Aug 1, 2023

View reviewed changes

soloio-bulldozer bot merged commit ac641d9 into master Aug 1, 2023
2 checks passed

soloio-bulldozer bot deleted the dmitrid-crd-install branch August 1, 2023 13:34

chunter0 added a commit that referenced this pull request Aug 7, 2023

Revert "Support for conditionally enabling of alpha-level CRDs (#462)"

21353c8

This reverts commit ac641d9.

jenshu mentioned this pull request Nov 19, 2023

SkipCRDManifest and SkipTemplatedCRDManifest are not respected #514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for conditionally enabling of alpha-level CRDs #462

Support for conditionally enabling of alpha-level CRDs #462

dmitri-d commented Jul 15, 2023 •

edited by solo-changelog-bot bot

Loading

solo-changelog-bot bot commented Jul 27, 2023

dmitri-d commented Jul 27, 2023

josh-pritchard left a comment •

edited

Loading

josh-pritchard Jul 28, 2023

dmitri-d Jul 28, 2023

ilackarms Jul 28, 2023

dmitri-d Jul 28, 2023

Sodman left a comment

Sodman Jul 28, 2023

dmitri-d Jul 28, 2023

Sodman Jul 28, 2023

dmitri-d Jul 28, 2023

Sodman Jul 28, 2023

dmitri-d Jul 28, 2023

Sodman Jul 28, 2023

dmitri-d Jul 28, 2023

Support for conditionally enabling of alpha-level CRDs #462

Support for conditionally enabling of alpha-level CRDs #462

Conversation

dmitri-d commented Jul 15, 2023 • edited by solo-changelog-bot bot Loading

solo-changelog-bot bot commented Jul 27, 2023

dmitri-d commented Jul 27, 2023

josh-pritchard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sodman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented Jul 15, 2023 •

edited by solo-changelog-bot bot

Loading

josh-pritchard left a comment •

edited

Loading