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.

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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

IfSentient
Copy link
Contributor

Add extra optional webhook conversion params to apiResource in CUE kind, use those parameters to add webhook conversion strategy to generated CRD's when apiResource.conversion = true.

The extra parameters are a temporary measure as we move toward the codegen having a manifest to look at which would include the necessary information.

…nd, use those parameters to add webhook conversion strategy to generated CRD's when apiResource.conversion = true.
@IfSentient IfSentient requested a review from a team as a code owner November 14, 2024 19:58
Comment on lines 148 to 149
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants