Skip to content

🌱 Bundle renderer refactor webhook service related function and attribute naming #1999

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

Merged
merged 2 commits into from
May 28, 2025
Merged

🌱 Bundle renderer refactor webhook service related function and attribute naming #1999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
351b64d
Refactor certprovider WebhookServiceName to ServiceName
May 23, 2025
29c5046
Refactor BundleWebhookServiceResourceGenerator to BundleDeploymentSer…
May 23, 2025
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
16 changes: 8 additions & 8 deletions internal/operator-controller/rukpak/render/certprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ type CertSecretInfo struct {
// CertificateProvisionerConfig contains the necessary information for a CertificateProvider
// to correctly generate and modify object for certificate injection and automation
type CertificateProvisionerConfig struct {
WebhookServiceName string
CertName string
Namespace string
CertProvider CertificateProvider
ServiceName string
CertName string
Namespace string
CertProvider CertificateProvider
}

// CertificateProvisioner uses a CertificateProvider to modify and generate objects based on its
Expand Down Expand Up @@ -70,9 +70,9 @@ func CertProvisionerFor(deploymentName string, opts Options) CertificateProvisio
certName := util.ObjectNameForBaseAndSuffix(webhookServiceName, "cert")

return CertificateProvisioner{
CertProvider: opts.CertificateProvider,
WebhookServiceName: webhookServiceName,
Namespace: opts.InstallNamespace,
CertName: certName,
CertProvider: opts.CertificateProvider,
ServiceName: webhookServiceName,
Namespace: opts.InstallNamespace,
CertName: certName,
}
}
30 changes: 15 additions & 15 deletions internal/operator-controller/rukpak/render/certprovider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ import (

func Test_CertificateProvisioner_WithoutCertProvider(t *testing.T) {
provisioner := &render.CertificateProvisioner{
WebhookServiceName: "webhook",
CertName: "cert",
Namespace: "namespace",
CertProvider: nil,
ServiceName: "webhook",
Copy link
Contributor

@camilamacedo86 camilamacedo86 May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

👍
Make sense we could use it for metrics:
See for example:

https://github.com/kubernetes-sigs/kubebuilder/blob/71fc857095565a48980cdfc323126c0cb1c98e7b/testdata/project-v4/config/default/kustomization.yaml#L44-L49

Which will patch: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/config/default/cert_metrics_manager_patch.yaml

To pass the cert for the metrics server like:

CertName: "cert",
Namespace: "namespace",
CertProvider: nil,
}

require.NoError(t, provisioner.InjectCABundle(&corev1.Secret{}))
Expand Down Expand Up @@ -50,10 +50,10 @@ func Test_CertificateProvisioner_WithCertProvider(t *testing.T) {
},
}
provisioner := &render.CertificateProvisioner{
WebhookServiceName: "webhook",
CertName: "cert",
Namespace: "namespace",
CertProvider: fakeProvider,
ServiceName: "webhook",
CertName: "cert",
Namespace: "namespace",
CertProvider: fakeProvider,
}

svc := &corev1.Service{}
Expand Down Expand Up @@ -83,10 +83,10 @@ func Test_CertificateProvisioner_Errors(t *testing.T) {
},
}
provisioner := &render.CertificateProvisioner{
WebhookServiceName: "webhook",
CertName: "cert",
Namespace: "namespace",
CertProvider: fakeProvider,
ServiceName: "webhook",
CertName: "cert",
Namespace: "namespace",
CertProvider: fakeProvider,
}

err := provisioner.InjectCABundle(&corev1.Service{})
Expand All @@ -107,16 +107,16 @@ func Test_CertProvisionerFor(t *testing.T) {
})

require.Equal(t, prov.CertProvider, fakeProvider)
require.Equal(t, "my-deployment-thing-service", prov.WebhookServiceName)
require.Equal(t, "my-deployment-thing-service", prov.ServiceName)
require.Equal(t, "my-deployment-thing-service-cert", prov.CertName)
require.Equal(t, "my-namespace", prov.Namespace)
}

func Test_CertProvisionerFor_ExtraLargeName_MoreThan63Chars(t *testing.T) {
prov := render.CertProvisionerFor("my.object.thing.has.a.really.really.really.really.really.long.name", render.Options{})

require.Len(t, prov.WebhookServiceName, 63)
require.Len(t, prov.ServiceName, 63)
require.Len(t, prov.CertName, 63)
require.Equal(t, "my-object-thing-has-a-really-really-really-really-reall-service", prov.WebhookServiceName)
require.Equal(t, "my-object-thing-has-a-really-really-really-really-reall-service", prov.ServiceName)
require.Equal(t, "my-object-thing-has-a-really-really-really-really-reall-se-cert", prov.CertName)
}
8 changes: 4 additions & 4 deletions internal/operator-controller/rukpak/render/certproviders/certmanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,13 @@ func (p CertManagerCertificateProvider) AdditionalObjects(cfg render.Certificate
},
Spec: certmanagerv1.CertificateSpec{
SecretName: cfg.CertName,
CommonName: fmt.Sprintf("%s.%s", cfg.WebhookServiceName, cfg.Namespace),
CommonName: fmt.Sprintf("%s.%s", cfg.ServiceName, cfg.Namespace),
Usages: []certmanagerv1.KeyUsage{certmanagerv1.UsageServerAuth},
IsCA: false,
DNSNames: []string{
fmt.Sprintf("%s.%s", cfg.WebhookServiceName, cfg.Namespace),
fmt.Sprintf("%s.%s.svc", cfg.WebhookServiceName, cfg.Namespace),
fmt.Sprintf("%s.%s.svc.cluster.local", cfg.WebhookServiceName, cfg.Namespace),
fmt.Sprintf("%s.%s", cfg.ServiceName, cfg.Namespace),
fmt.Sprintf("%s.%s.svc", cfg.ServiceName, cfg.Namespace),
fmt.Sprintf("%s.%s.svc.cluster.local", cfg.ServiceName, cfg.Namespace),
},
IssuerRef: certmanagermetav1.ObjectReference{
Name: issuer.GetName(),
Expand Down
36 changes: 18 additions & 18 deletions internal/operator-controller/rukpak/render/certproviders/certmanager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ func Test_CertManagerProvider_InjectCABundle(t *testing.T) {
name: "injects certificate annotation in validating webhook configuration",
obj: &admissionregistrationv1.ValidatingWebhookConfiguration{},
cfg: render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
},
expectedObj: &admissionregistrationv1.ValidatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -46,9 +46,9 @@ func Test_CertManagerProvider_InjectCABundle(t *testing.T) {
name: "injects certificate annotation in mutating webhook configuration",
obj: &admissionregistrationv1.MutatingWebhookConfiguration{},
cfg: render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
},
expectedObj: &admissionregistrationv1.MutatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -62,9 +62,9 @@ func Test_CertManagerProvider_InjectCABundle(t *testing.T) {
name: "injects certificate annotation in custom resource definition",
obj: &apiextensionsv1.CustomResourceDefinition{},
cfg: render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
},
expectedObj: &apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -78,9 +78,9 @@ func Test_CertManagerProvider_InjectCABundle(t *testing.T) {
name: "ignores other objects",
obj: &corev1.Service{},
cfg: render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
},
expectedObj: &corev1.Service{},
},
Expand All @@ -96,9 +96,9 @@ func Test_CertManagerProvider_InjectCABundle(t *testing.T) {
func Test_CertManagerProvider_AdditionalObjects(t *testing.T) {
certProvier := certproviders.CertManagerCertificateProvider{}
objs, err := certProvier.AdditionalObjects(render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
})
require.NoError(t, err)
require.Equal(t, []unstructured.Unstructured{
Expand Down Expand Up @@ -151,9 +151,9 @@ func Test_CertManagerProvider_AdditionalObjects(t *testing.T) {
func Test_CertManagerProvider_GetCertSecretInfo(t *testing.T) {
certProvier := certproviders.CertManagerCertificateProvider{}
certInfo := certProvier.GetCertSecretInfo(render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
})
require.Equal(t, render.CertSecretInfo{
SecretName: "cert-name",
Expand Down
42 changes: 21 additions & 21 deletions internal/operator-controller/rukpak/render/certproviders/openshift_serviceca_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ func Test_OpenshiftServiceCAProvider_InjectCABundle(t *testing.T) {
name: "injects inject-cabundle annotation in validating webhook configuration",
obj: &admissionregistrationv1.ValidatingWebhookConfiguration{},
cfg: render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
},
expectedObj: &admissionregistrationv1.ValidatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -41,9 +41,9 @@ func Test_OpenshiftServiceCAProvider_InjectCABundle(t *testing.T) {
name: "injects inject-cabundle annotation in mutating webhook configuration",
obj: &admissionregistrationv1.MutatingWebhookConfiguration{},
cfg: render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
},
expectedObj: &admissionregistrationv1.MutatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -57,9 +57,9 @@ func Test_OpenshiftServiceCAProvider_InjectCABundle(t *testing.T) {
name: "injects inject-cabundle annotation in custom resource definition",
obj: &apiextensionsv1.CustomResourceDefinition{},
cfg: render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
},
expectedObj: &apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -73,9 +73,9 @@ func Test_OpenshiftServiceCAProvider_InjectCABundle(t *testing.T) {
name: "injects serving-cert-secret-name annotation in service resource referencing the certificate name",
obj: &corev1.Service{},
cfg: render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
},
expectedObj: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -89,9 +89,9 @@ func Test_OpenshiftServiceCAProvider_InjectCABundle(t *testing.T) {
name: "ignores other objects",
obj: &corev1.Secret{},
cfg: render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
},
expectedObj: &corev1.Secret{},
},
Expand All @@ -107,9 +107,9 @@ func Test_OpenshiftServiceCAProvider_InjectCABundle(t *testing.T) {
func Test_OpenshiftServiceCAProvider_AdditionalObjects(t *testing.T) {
certProvider := certproviders.OpenshiftServiceCaCertificateProvider{}
objs, err := certProvider.AdditionalObjects(render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
})
require.NoError(t, err)
require.Nil(t, objs)
Expand All @@ -118,9 +118,9 @@ func Test_OpenshiftServiceCAProvider_AdditionalObjects(t *testing.T) {
func Test_OpenshiftServiceCAProvider_GetCertSecretInfo(t *testing.T) {
certProvider := certproviders.OpenshiftServiceCaCertificateProvider{}
certInfo := certProvider.GetCertSecretInfo(render.CertificateProvisionerConfig{
WebhookServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
ServiceName: "webhook-service",
Namespace: "namespace",
CertName: "cert-name",
})
require.Equal(t, render.CertSecretInfo{
SecretName: "cert-name",
Expand Down
14 changes: 7 additions & 7 deletions internal/operator-controller/rukpak/render/registryv1/generators/generators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func BundleCRDGenerator(rv1 *bundle.RegistryV1, opts render.Options) ([]client.O
ClientConfig: &apiextensionsv1.WebhookClientConfig{
Service: &apiextensionsv1.ServiceReference{
Namespace: opts.InstallNamespace,
Name: certProvisioner.WebhookServiceName,
Name: certProvisioner.ServiceName,
Path: &conversionWebhookPath,
Port: &cw.ContainerPort,
},
Expand Down Expand Up @@ -314,7 +314,7 @@ func BundleValidatingWebhookResourceGenerator(rv1 *bundle.RegistryV1, opts rende
ClientConfig: admissionregistrationv1.WebhookClientConfig{
Service: &admissionregistrationv1.ServiceReference{
Namespace: opts.InstallNamespace,
Name: certProvisioner.WebhookServiceName,
Name: certProvisioner.ServiceName,
Path: wh.WebhookPath,
Port: &wh.ContainerPort,
},
Expand Down Expand Up @@ -362,7 +362,7 @@ func BundleMutatingWebhookResourceGenerator(rv1 *bundle.RegistryV1, opts render.
ClientConfig: admissionregistrationv1.WebhookClientConfig{
Service: &admissionregistrationv1.ServiceReference{
Namespace: opts.InstallNamespace,
Name: certProvisioner.WebhookServiceName,
Name: certProvisioner.ServiceName,
Path: wh.WebhookPath,
Port: &wh.ContainerPort,
},
Expand All @@ -379,10 +379,10 @@ func BundleMutatingWebhookResourceGenerator(rv1 *bundle.RegistryV1, opts render.
return objs, nil
}

// BundleWebhookServiceResourceGenerator generates Service resources based that support the webhooks defined in
// the bundle's cluster service version spec. The resource is modified by the CertificateProvider in opts
// BundleDeploymentServiceResourceGenerator generates Service resources that support, e.g. the webhooks,
// defined in the bundle's cluster service version spec. The resource is modified by the CertificateProvider in opts
// to add any annotations or modifications necessary for certificate injection.
func BundleWebhookServiceResourceGenerator(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
func BundleDeploymentServiceResourceGenerator(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
if rv1 == nil {
return nil, fmt.Errorf("bundle cannot be nil")
}
Expand Down Expand Up @@ -415,7 +415,7 @@ func BundleWebhookServiceResourceGenerator(rv1 *bundle.RegistryV1, opts render.O

certProvisioner := render.CertProvisionerFor(deploymentSpec.Name, opts)
serviceResource := CreateServiceResource(
certProvisioner.WebhookServiceName,
certProvisioner.ServiceName,
opts.InstallNamespace,
WithServiceSpec(
corev1.ServiceSpec{
Expand Down
6 changes: 3 additions & 3 deletions internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2000,7 +2000,7 @@ func Test_BundleMutatingWebhookResourceGenerator_FailsOnNil(t *testing.T) {
require.Contains(t, err.Error(), "bundle cannot be nil")
}

func Test_BundleWebhookServiceResourceGenerator_Succeeds(t *testing.T) {
func Test_BundleDeploymentServiceResourceGenerator_Succeeds(t *testing.T) {
fakeProvider := FakeCertProvider{
InjectCABundleFn: func(obj client.Object, cfg render.CertificateProvisionerConfig) error {
obj.SetAnnotations(map[string]string{
Expand Down Expand Up @@ -2414,14 +2414,14 @@ func Test_BundleWebhookServiceResourceGenerator_Succeeds(t *testing.T) {
},
} {
t.Run(tc.name, func(t *testing.T) {
objs, err := generators.BundleWebhookServiceResourceGenerator(tc.bundle, tc.opts)
objs, err := generators.BundleDeploymentServiceResourceGenerator(tc.bundle, tc.opts)
require.NoError(t, err)
require.Equal(t, tc.expectedResources, objs)
})
}
}

func Test_BundleWebhookServiceResourceGenerator_FailsOnNil(t *testing.T) {
func Test_BundleDeploymentServiceResourceGenerator_FailsOnNil(t *testing.T) {
objs, err := generators.BundleMutatingWebhookResourceGenerator(nil, render.Options{})
require.Nil(t, objs)
require.Error(t, err)
Expand Down
2 changes: 1 addition & 1 deletion internal/operator-controller/rukpak/render/registryv1/registryv1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,6 @@ var ResourceGenerators = []render.ResourceGenerator{
generators.BundleCSVDeploymentGenerator,
generators.BundleValidatingWebhookResourceGenerator,
generators.BundleMutatingWebhookResourceGenerator,
generators.BundleWebhookServiceResourceGenerator,
generators.BundleDeploymentServiceResourceGenerator,
generators.CertProviderResourceGenerator,
}
2 changes: 1 addition & 1 deletion internal/operator-controller/rukpak/render/registryv1/registryv1_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func Test_ResourceGeneratorsHasAllGenerators(t *testing.T) {
generators.BundleCSVDeploymentGenerator,
generators.BundleValidatingWebhookResourceGenerator,
generators.BundleMutatingWebhookResourceGenerator,
generators.BundleWebhookServiceResourceGenerator,
generators.BundleDeploymentServiceResourceGenerator,
generators.CertProviderResourceGenerator,
}
actualGenerators := registryv1.ResourceGenerators
Expand Down
Loading