Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

[codegen] Add Webhook Conversion Strategy to Generated CRDs #481

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+68 −10
Open

[codegen] Add Webhook Conversion Strategy to Generated CRDs #481

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e2d872d
Add extra optional webhook conversion params to apiResource in CUE ki…
IfSentient Nov 14, 2024
9826758
Use URL instead of service name/namespace when configuring coversion …
IfSentient Nov 15, 2024
46bd5cf
Update URL to be in ClientConfig instead of Service.
IfSentient Nov 26, 2024
5a5a750
Merge branch 'main' into crd-gen-include-conversion
IfSentient Nov 26, 2024
9ac60e9
Fix go.work.sum from merge.
IfSentient Nov 26, 2024
0574f6c
Merge branch 'main' into crd-gen-include-conversion
IfSentient Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions codegen/cuekind/def.cue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,13 @@ Kind: S={
}
// conversion determines whether there is code-based conversion for this kind. Used for generating the manifest.
conversion: bool | *false
// conversionWebhookProps is a temporary way of specifying the service webhook information
// which will be migrated away from once manifests are used in the codegen pipeline
conversionWebhookProps: {
serviceName: string | *""
serviceNamespace: string | *""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to support url (https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#webhookclientconfig-v1-apiextensions-k8s-io) here, either as an option, or as the main & only way – we don't use service configs at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to the main & only way, as this is something we'll likely want to address better in the manifest and this is a temporary patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM!

There's an issue with the code at the moment, since it doesn't comply with CRD schema – the config should be either a Service or an URL.

I suggest that instead of hand-rolling custom types, we instead use the canonical ones from upstream, e.g. something like this:

diff --git a/codegen/jennies/crd.go b/codegen/jennies/crd.go
index 17c5f91..32ad450 100644
--- a/codegen/jennies/crd.go
+++ b/codegen/jennies/crd.go
@@ -3,11 +3,13 @@ package jennies
 
 import (
 	"fmt"
+	"net/url"
 	"strings"
 
 	"cuelang.org/go/cue"
 	"github.com/grafana/codejen"
 	goyaml "gopkg.in/yaml.v3"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
 	"github.com/grafana/grafana-app-sdk/codegen"
 	"github.com/grafana/grafana-app-sdk/k8s"
@@ -53,15 +55,18 @@ func (c *crdGenerator) Generate(kind codegen.Kind) (*codejen.File, error) {
 	}
 
 	if kind.Properties().APIResource.Conversion {
-		resource.Spec.Conversion = &k8s.CustomResourceDefinitionSpecConversion{
-			Strategy: "webhook",
-			Webhook: &k8s.CustomResourceDefinitionSpecConversionWebhook{
+		u := kind.Properties().APIResource.ConversionWebhookProps.URL
+
+		if _, err := url.Parse(u); err != nil {
+			return nil, err
+		}
+
+		resource.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
+			Strategy: apiextensionsv1.WebhookConverter,
+			Webhook: &apiextensionsv1.WebhookConversion{
 				ConversionReviewVersions: []string{"v1"},
-				ClientConfig: k8s.CustomResourceDefinitionClientConfig{
-					Service: k8s.CustomResourceDefinitionClientConfigService{
-						Path: kind.Properties().APIResource.ConversionWebhookProps.Path,
-						URL:  kind.Properties().APIResource.ConversionWebhookProps.URL,
-					},
+				ClientConfig: &apiextensionsv1.WebhookClientConfig{
+					URL: &u,
 				},
 			},
 		}
diff --git a/k8s/manager.go b/k8s/manager.go
index 4cabd17..cf6a4c3 100644
--- a/k8s/manager.go
+++ b/k8s/manager.go
@@ -10,6 +10,7 @@ import (
 	"strings"
 	"time"
 
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	kschema "k8s.io/apimachinery/pkg/runtime/schema"
@@ -324,32 +325,11 @@ func (crd *CustomResourceDefinition) DeepCopyObject() runtime.Object {
 
 // CustomResourceDefinitionSpec is the body or spec of a kubernetes Custom Resource Definition
 type CustomResourceDefinitionSpec struct {
-	Group      string                                  `json:"group" yaml:"group"`
-	Versions   []CustomResourceDefinitionSpecVersion   `json:"versions" yaml:"versions"`
-	Names      CustomResourceDefinitionSpecNames       `json:"names" yaml:"names"`
-	Conversion *CustomResourceDefinitionSpecConversion `json:"conversion,omitempty" yaml:"conversion,omitempty"`
-	Scope      string                                  `json:"scope" yaml:"scope"`
-}
-
-type CustomResourceDefinitionSpecConversion struct {
-	Strategy string                                         `json:"strategy" yaml:"strategy"`
-	Webhook  *CustomResourceDefinitionSpecConversionWebhook `json:"webhook,omitempty" yaml:"webhook,omitempty"`
-}
-
-type CustomResourceDefinitionSpecConversionWebhook struct {
-	ConversionReviewVersions []string                             `json:"conversionReviewVersions" yaml:"conversionReviewVersions"`
-	ClientConfig             CustomResourceDefinitionClientConfig `json:"clientConfig" yaml:"clientConfig"`
-}
-
-type CustomResourceDefinitionClientConfig struct {
-	Service CustomResourceDefinitionClientConfigService `json:"service" yaml:"service"`
-}
-
-type CustomResourceDefinitionClientConfigService struct {
-	URL       string `json:"url,omitempty" yaml:"url,omitempty"`
-	Name      string `json:"name,omitempty" yaml:"name,omitempty"`
-	Namespace string `json:"namespace,omitempty" yaml:"namespace,omitempty"`
-	Path      string `json:"path" yaml:"path"`
+	Group      string                                    `json:"group" yaml:"group"`
+	Versions   []CustomResourceDefinitionSpecVersion     `json:"versions" yaml:"versions"`
+	Names      CustomResourceDefinitionSpecNames         `json:"names" yaml:"names"`
+	Conversion *apiextensionsv1.CustomResourceConversion `json:"conversion,omitempty" yaml:"conversion,omitempty"`
+	Scope      string                                    `json:"scope" yaml:"scope"`
 }
 
 // CustomResourceDefinitionSpecVersion is the representation of a specific version of a CRD, as part of the overall spec

(I've also thrown in URL validation but feel free to drop it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this comment totally slipped through the cracks for me, just noticed it today. The problem with using the kubernetes types is that they don't have yaml tags, which presented problems in the past for YAML marshal (which is why the SDK has ones which are hand-rolled). I can fix the structure, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@radiohead fixed the URL portion, I think we can move over to using the k8s types if we change our YAML library, I'll try to get that working in a different PR.

path: string | *"/convert"
}
}
// isCRD is true if the `crd` trait is present in the kind.
isAPIResource: apiResource != _|_
Expand Down
2 changes: 2 additions & 0 deletions codegen/cuekind/testing/testkind.cue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ testKind: {
apiResource: {
validation: operations: ["create","update"]
conversion: true
conversionWebhookProps: serviceName: "foo-operator"
conversionWebhookProps: serviceNamespace: "foo-namespace"
}
current: "v1"
codegen: frontend: false
Expand Down
16 changes: 16 additions & 0 deletions codegen/jennies/crd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,22 @@ func (c *crdGenerator) Generate(kind codegen.Kind) (*codejen.File, error) {
},
}

if kind.Properties().APIResource.Conversion {
resource.Spec.Conversion = &k8s.CustomResourceDefinitionSpecConversion{
Strategy: "webhook",
Webhook: &k8s.CustomResourceDefinitionSpecConversionWebhook{
ConversionReviewVersions: []string{"v1"},
ClientConfig: k8s.CustomResourceDefinitionClientConfig{
Service: k8s.CustomResourceDefinitionClientConfigService{
Path: kind.Properties().APIResource.ConversionWebhookProps.Path,
Name: kind.Properties().APIResource.ConversionWebhookProps.ServiceName,
Namespace: kind.Properties().APIResource.ConversionWebhookProps.ServiceNamespace,
},
},
},
}
}

for _, ver := range kind.Versions() {
v, err := KindVersionToCRDSpecVersion(ver, kind.Properties().Kind, ver.Version == kind.Properties().Current)
if err != nil {
Expand Down
17 changes: 12 additions & 5 deletions codegen/kind.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,18 @@ type KindProperties struct {

// APIResourceProperties contains information about a Kind expressible as a kubernetes API resource
type APIResourceProperties struct {
Group string `json:"group"`
Scope string `json:"scope"`
Validation KindAdmissionCapability `json:"validation"`
Mutation KindAdmissionCapability `json:"mutation"`
Conversion bool `json:"conversion"`
Group string `json:"group"`
Scope string `json:"scope"`
Validation KindAdmissionCapability `json:"validation"`
Mutation KindAdmissionCapability `json:"mutation"`
Conversion bool `json:"conversion"`
ConversionWebhookProps ConversionWebhookProperties `json:"conversionWebhookProps"`
}

type ConversionWebhookProperties struct {
ServiceName string `json:"serviceName"`
ServiceNamespace string `json:"serviceNamespace"`
Path string `json:"path"`
}

type KindAdmissionCapabilityOperation string
Expand Down
2 changes: 1 addition & 1 deletion codegen/testing/golden_generated/crd/testkind.test.ext.grafana.com.json.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"kind":"CustomResourceDefinition","apiVersion":"apiextensions.k8s.io/v1","metadata":{"name":"testkinds.test.ext.grafana.com"},"spec":{"group":"test.ext.grafana.com","versions":[{"name":"v1","served":true,"storage":true,"schema":{"openAPIV3Schema":{"properties":{"spec":{"properties":{"stringField":{"type":"string"}},"required":["stringField"],"type":"object"},"status":{"properties":{"additionalFields":{"description":"additionalFields is reserved for future use","type":"object","x-kubernetes-preserve-unknown-fields":true},"operatorStates":{"additionalProperties":{"properties":{"descriptiveState":{"description":"descriptiveState is an optional more descriptive state field which has no requirements on format","type":"string"},"details":{"description":"details contains any extra information that is operator-specific","type":"object","x-kubernetes-preserve-unknown-fields":true},"lastEvaluation":{"description":"lastEvaluation is the ResourceVersion last evaluated","type":"string"},"state":{"description":"state describes the state of the lastEvaluation.\nIt is limited to three possible states for machine evaluation.","enum":["success","in_progress","failed"],"type":"string"}},"required":["lastEvaluation","state"],"type":"object"},"description":"operatorStates is a map of operator ID to operator state evaluations.\nAny operator which consumes this kind SHOULD add its state evaluation information to this field.","type":"object"}},"type":"object","x-kubernetes-preserve-unknown-fields":true}},"required":["spec"],"type":"object"}},"subresources":{"status":{}}},{"name":"v2","served":true,"storage":false,"schema":{"openAPIV3Schema":{"properties":{"spec":{"properties":{"intField":{"format":"int64","type":"integer"},"stringField":{"type":"string"},"timeField":{"format":"date-time","type":"string"}},"required":["stringField","intField","timeField"],"type":"object"},"status":{"properties":{"additionalFields":{"description":"additionalFields is reserved for future use","type":"object","x-kubernetes-preserve-unknown-fields":true},"operatorStates":{"additionalProperties":{"properties":{"descriptiveState":{"description":"descriptiveState is an optional more descriptive state field which has no requirements on format","type":"string"},"details":{"description":"details contains any extra information that is operator-specific","type":"object","x-kubernetes-preserve-unknown-fields":true},"lastEvaluation":{"description":"lastEvaluation is the ResourceVersion last evaluated","type":"string"},"state":{"description":"state describes the state of the lastEvaluation.\nIt is limited to three possible states for machine evaluation.","enum":["success","in_progress","failed"],"type":"string"}},"required":["lastEvaluation","state"],"type":"object"},"description":"operatorStates is a map of operator ID to operator state evaluations.\nAny operator which consumes this kind SHOULD add its state evaluation information to this field.","type":"object"}},"type":"object","x-kubernetes-preserve-unknown-fields":true}},"required":["spec"],"type":"object"}},"subresources":{"status":{}},"additionalPrinterColumns":[{"name":"STRING FIELD","type":"string","jsonPath":".spec.stringField"}]}],"names":{"kind":"TestKind","plural":"testkinds"},"scope":"Namespaced"}}
{"kind":"CustomResourceDefinition","apiVersion":"apiextensions.k8s.io/v1","metadata":{"name":"testkinds.test.ext.grafana.com"},"spec":{"group":"test.ext.grafana.com","versions":[{"name":"v1","served":true,"storage":true,"schema":{"openAPIV3Schema":{"properties":{"spec":{"properties":{"stringField":{"type":"string"}},"required":["stringField"],"type":"object"},"status":{"properties":{"additionalFields":{"description":"additionalFields is reserved for future use","type":"object","x-kubernetes-preserve-unknown-fields":true},"operatorStates":{"additionalProperties":{"properties":{"descriptiveState":{"description":"descriptiveState is an optional more descriptive state field which has no requirements on format","type":"string"},"details":{"description":"details contains any extra information that is operator-specific","type":"object","x-kubernetes-preserve-unknown-fields":true},"lastEvaluation":{"description":"lastEvaluation is the ResourceVersion last evaluated","type":"string"},"state":{"description":"state describes the state of the lastEvaluation.\nIt is limited to three possible states for machine evaluation.","enum":["success","in_progress","failed"],"type":"string"}},"required":["lastEvaluation","state"],"type":"object"},"description":"operatorStates is a map of operator ID to operator state evaluations.\nAny operator which consumes this kind SHOULD add its state evaluation information to this field.","type":"object"}},"type":"object","x-kubernetes-preserve-unknown-fields":true}},"required":["spec"],"type":"object"}},"subresources":{"status":{}}},{"name":"v2","served":true,"storage":false,"schema":{"openAPIV3Schema":{"properties":{"spec":{"properties":{"intField":{"format":"int64","type":"integer"},"stringField":{"type":"string"},"timeField":{"format":"date-time","type":"string"}},"required":["stringField","intField","timeField"],"type":"object"},"status":{"properties":{"additionalFields":{"description":"additionalFields is reserved for future use","type":"object","x-kubernetes-preserve-unknown-fields":true},"operatorStates":{"additionalProperties":{"properties":{"descriptiveState":{"description":"descriptiveState is an optional more descriptive state field which has no requirements on format","type":"string"},"details":{"description":"details contains any extra information that is operator-specific","type":"object","x-kubernetes-preserve-unknown-fields":true},"lastEvaluation":{"description":"lastEvaluation is the ResourceVersion last evaluated","type":"string"},"state":{"description":"state describes the state of the lastEvaluation.\nIt is limited to three possible states for machine evaluation.","enum":["success","in_progress","failed"],"type":"string"}},"required":["lastEvaluation","state"],"type":"object"},"description":"operatorStates is a map of operator ID to operator state evaluations.\nAny operator which consumes this kind SHOULD add its state evaluation information to this field.","type":"object"}},"type":"object","x-kubernetes-preserve-unknown-fields":true}},"required":["spec"],"type":"object"}},"subresources":{"status":{}},"additionalPrinterColumns":[{"name":"STRING FIELD","type":"string","jsonPath":".spec.stringField"}]}],"names":{"kind":"TestKind","plural":"testkinds"},"conversion":{"strategy":"webhook","webhook":{"conversionReviewVersions":["v1"],"clientConfig":{"service":{"name":"foo-operator","namespace":"foo-namespace","path":"/convert"}}}},"scope":"Namespaced"}}
10 changes: 10 additions & 0 deletions codegen/testing/golden_generated/crd/testkind.test.ext.grafana.com.yaml.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,14 @@ spec:
names:
kind: TestKind
plural: testkinds
conversion:
strategy: webhook
webhook:
conversionReviewVersions:
- v1
clientConfig:
service:
name: foo-operator
namespace: foo-namespace
path: /convert
scope: Namespaced
Loading
Loading