Skip to content

Commit 5b7f265

Browse files
awgreeneawgreene
committed
Allow ConversionWebhooks to define targetPort
Problem: It is currently not possible to specify a conversion webhook without specifying either a validating or mutating admission webhook. Additionally, it is not possible to define the targetPort of the service for webhooks Solution: Update the WebhookDescription.Type Enum to include ConversionWebhook as an option. Then update OLM to just update the owned CRD when this option is specified. Also allow users to specify a targetPort as well.
1 parent 3669367 commit 5b7f265

File tree

4 files changed

+201
-159
lines changed

4 files changed

+201
-159
lines changed

‎pkg/controller/install/certresources.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,18 @@ func (i *webhookDescriptionWithCAPEM) getServicePort() corev1.ServicePort {
125125
if i.webhookDescription.ContainerPort > 0 {
126126
containerPort = int(i.webhookDescription.ContainerPort)
127127
}
128+
129+
// Before users could set TargetPort in the CSV, OLM just set its
130+
// value to the containerPort. This change keeps OLM backwards compatible
131+
// if the TargetPort is not set in the CSV.
132+
targetPort := intstr.FromInt(containerPort)
133+
if i.webhookDescription.TargetPort != nil {
134+
targetPort = *i.webhookDescription.TargetPort
135+
}
128136
return corev1.ServicePort{
129137
Name: strconv.Itoa(containerPort),
130138
Port: int32(containerPort),
131-
TargetPort: intstr.FromInt(containerPort),
139+
TargetPort: targetPort,
132140
}
133141
}
134142

‎pkg/controller/install/webhook.go

Lines changed: 47 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,8 @@ func (i *StrategyDeploymentInstaller) createOrUpdateWebhook(caPEM []byte, desc v
6969
i.createOrUpdateValidatingWebhook(ogNamespacelabelSelector, caPEM, desc)
7070
case v1alpha1.MutatingAdmissionWebhook:
7171
i.createOrUpdateMutatingWebhook(ogNamespacelabelSelector, caPEM, desc)
72-
72+
case v1alpha1.ConversionWebhook:
73+
i.createOrUpdateConversionWebhook(caPEM, desc)
7374
}
7475
return nil
7576
}
@@ -102,14 +103,6 @@ func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacel
102103
log.Errorf("Webhooks: Error creating MutatingWebhookConfiguration: %v", err)
103104
return err
104105
}
105-
106-
// If dealing with a Conversion Webhook, make sure that the operator only supports the AllNamespace installMode.
107-
if len(desc.ConversionCRDs) != 0 && isSingletonOperator(i) {
108-
if err := createOrUpdateConversionCRDs(desc, webhook.Webhooks[0].ClientConfig, i); err != nil {
109-
return err
110-
}
111-
}
112-
return nil
113106
}
114107
for _, webhook := range existingWebhooks.Items {
115108
// Update the list of webhooks
@@ -123,13 +116,6 @@ func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacel
123116
log.Warnf("could not update MutatingWebhookConfiguration %s", webhook.GetName())
124117
return err
125118
}
126-
127-
// If dealing with a Conversion Webhook, make sure that the operator only supports the AllNamespace installMode.
128-
if len(desc.ConversionCRDs) != 0 && isSingletonOperator(i) {
129-
if err := createOrUpdateConversionCRDs(desc, webhook.Webhooks[0].ClientConfig, i); err != nil {
130-
return err
131-
}
132-
}
133119
}
134120

135121
return nil
@@ -164,13 +150,6 @@ func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespac
164150
return err
165151
}
166152

167-
// If dealing with a Conversion Webhook, make sure that the operator only supports the AllNamespace installMode.
168-
if len(desc.ConversionCRDs) != 0 && isSingletonOperator(i) {
169-
if err := createOrUpdateConversionCRDs(desc, webhook.Webhooks[0].ClientConfig, i); err != nil {
170-
return err
171-
}
172-
}
173-
174153
return nil
175154
}
176155
for _, webhook := range existingWebhooks.Items {
@@ -185,25 +164,13 @@ func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespac
185164
log.Warnf("could not update ValidatingWebhookConfiguration %s", webhook.GetName())
186165
return err
187166
}
188-
189-
// If dealing with a Conversion Webhook, make sure that the operator only supports the AllNamespace installMode.
190-
if len(desc.ConversionCRDs) != 0 && isSingletonOperator(i) {
191-
if err := createOrUpdateConversionCRDs(desc, webhook.Webhooks[0].ClientConfig, i); err != nil {
192-
return err
193-
}
194-
}
195167
}
196168

197169
return nil
198170
}
199171

200172
// check if csv supports only AllNamespaces install mode
201-
func isSingletonOperator(i *StrategyDeploymentInstaller) bool {
202-
// get csv
203-
csv, err := i.strategyClient.GetOpLister().OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(i.owner.GetNamespace()).Get(i.owner.GetName())
204-
if err != nil {
205-
log.Infof("CSV not found, error: %s", err.Error())
206-
}
173+
func isSingletonOperator(csv v1alpha1.ClusterServiceVersion) bool {
207174
// check if AllNamespaces is supported and other install modes are not supported
208175
for _, installMode := range csv.Spec.InstallModes {
209176
if installMode.Type == v1alpha1.InstallModeTypeAllNamespaces && !installMode.Supported {
@@ -216,34 +183,39 @@ func isSingletonOperator(i *StrategyDeploymentInstaller) bool {
216183
return true
217184
}
218185

219-
func createOrUpdateConversionCRDs(desc v1alpha1.WebhookDescription, clientConfig admissionregistrationv1.WebhookClientConfig, i *StrategyDeploymentInstaller) error {
186+
func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []byte, desc v1alpha1.WebhookDescription) error {
220187
// get a list of owned CRDs
221-
csv, err := i.strategyClient.GetOpLister().OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(i.owner.GetNamespace()).Get(i.owner.GetName())
222-
if err != nil {
223-
log.Infof("CSV not found, error: %s", err.Error())
188+
csv, ok := i.owner.(*v1alpha1.ClusterServiceVersion)
189+
if !ok {
190+
return fmt.Errorf("ConversionWebhook owner must be a ClusterServiceVersion")
191+
}
192+
193+
if !isSingletonOperator(*csv) {
194+
return fmt.Errorf("CSVs with conversion webhooks must support only AllNamespaces")
195+
}
196+
197+
if len(desc.ConversionCRDs) == 0 {
198+
return fmt.Errorf("Conversion Webhook must have at least one CRD specified")
224199
}
225-
ownedCRDs := csv.Spec.CustomResourceDefinitions.Owned
226200

227201
// iterate over all the ConversionCRDs
228-
for _, ConversionCRD := range desc.ConversionCRDs {
229-
// check if CRD exists on cluster
230-
crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), ConversionCRD, metav1.GetOptions{})
202+
for _, conversionCRD := range desc.ConversionCRDs {
203+
// Get existing CRD on cluster
204+
crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
231205
if err != nil {
232-
log.Infof("CRD not found %s, error: %s", ConversionCRD, err.Error())
206+
return fmt.Errorf("Unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err)
233207
}
234-
log.Infof("Found conversionCRDs %s", ConversionCRD)
235208

236209
// check if this CRD is an owned CRD
237210
foundCRD := false
238-
for _, ownedCRD := range ownedCRDs {
239-
if ownedCRD.Name == crd.GetName() {
240-
log.Infof("CSV %q owns CRD %q", csv.GetName(), crd.GetName())
211+
for _, ownedCRD := range csv.Spec.CustomResourceDefinitions.Owned {
212+
if ownedCRD.Name == conversionCRD {
241213
foundCRD = true
214+
break
242215
}
243216
}
244217
if !foundCRD {
245-
log.Infof("CSV %q does not own CRD %q", csv.GetName(), crd.GetName())
246-
return nil
218+
return fmt.Errorf("CSV %s does not own CRD %s", csv.GetName(), conversionCRD)
247219
}
248220

249221
// crd.Spec.Conversion.Strategy specifies how custom resources are converted between versions.
@@ -256,54 +228,41 @@ func createOrUpdateConversionCRDs(desc v1alpha1.WebhookDescription, clientConfig
256228
// By default the strategy is none
257229
// Reference:
258230
// - https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
259-
if crd.Spec.PreserveUnknownFields == true && crd.Spec.Conversion.Strategy == "Webhook" {
260-
log.Infof("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion.")
261-
return nil
231+
if crd.Spec.PreserveUnknownFields != false {
232+
return fmt.Errorf("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion")
262233
}
263234

264235
// Conversion WebhookClientConfig should not be set when Strategy is None
265236
// https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
266237
// Conversion WebhookClientConfig needs to be set when Strategy is None
267238
// https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks
268-
if crd.Spec.Conversion.Strategy == "Webhook" {
269-
log.Infof("Updating CRD")
270-
271-
// use user defined path for CRD conversion webhook, else set default value
272-
conversionWebhookPath := "/"
273-
if crd.Spec.Conversion.Webhook.ClientConfig.Service.Path != nil {
274-
conversionWebhookPath = *crd.Spec.Conversion.Webhook.ClientConfig.Service.Path
275-
}
276239

277-
// use user defined port, else set it to admission webhook's port
278-
conversionWebhookPort := *clientConfig.Service.Port
279-
if crd.Spec.Conversion.Webhook.ClientConfig.Service.Port != nil {
280-
conversionWebhookPort = *crd.Spec.Conversion.Webhook.ClientConfig.Service.Port
281-
}
240+
// use user defined path for CRD conversion webhook, else set default value
241+
conversionWebhookPath := "/"
242+
if desc.WebhookPath != nil {
243+
conversionWebhookPath = *desc.WebhookPath
244+
}
282245

283-
// use user defined ConversionReviewVersions
284-
conversionWebhookConversionReviewVersions := crd.Spec.Conversion.Webhook.ConversionReviewVersions
285-
286-
// Override Name, Namespace, and CABundle
287-
crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
288-
Strategy: "Webhook",
289-
Webhook: &apiextensionsv1.WebhookConversion{
290-
ClientConfig: &apiextensionsv1.WebhookClientConfig{
291-
Service: &apiextensionsv1.ServiceReference{
292-
Namespace: clientConfig.Service.Namespace,
293-
Name: clientConfig.Service.Name,
294-
Path: &conversionWebhookPath,
295-
Port: &conversionWebhookPort,
296-
},
297-
CABundle: clientConfig.CABundle,
246+
// Override Name, Namespace, and CABundle
247+
crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
248+
Strategy: "Webhook",
249+
Webhook: &apiextensionsv1.WebhookConversion{
250+
ClientConfig: &apiextensionsv1.WebhookClientConfig{
251+
Service: &apiextensionsv1.ServiceReference{
252+
Namespace: i.owner.GetNamespace(),
253+
Name: desc.DomainName() + "-service",
254+
Path: &conversionWebhookPath,
255+
Port: &desc.ContainerPort,
298256
},
299-
ConversionReviewVersions: conversionWebhookConversionReviewVersions,
257+
CABundle: caPEM,
300258
},
301-
}
259+
ConversionReviewVersions: desc.AdmissionReviewVersions,
260+
},
261+
}
302262

303-
// update CRD conversion Specs
304-
if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil {
305-
log.Infof("CRD %s could not be updated, error: %s", ConversionCRD, err.Error())
306-
}
263+
// update CRD conversion Specs
264+
if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil {
265+
return fmt.Errorf("Error updating CRD with Conversion info: %v", err)
307266
}
308267
}
309268

‎pkg/controller/operators/olm/apiservices.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -553,9 +553,22 @@ func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion) (bo
553553
return false, err
554554
}
555555
webhookCount = len(webhookList.Items)
556+
case v1alpha1.ConversionWebhook:
557+
for _, conversionCRD := range desc.ConversionCRDs {
558+
// check if CRD exists on cluster
559+
crd, err := a.opClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
560+
if err != nil {
561+
log.Infof("CRD not found %v, error: %s", desc, err.Error())
562+
return false, err
563+
}
564+
565+
if crd.Spec.Conversion.Strategy != "Webhook" || crd.Spec.Conversion.Webhook == nil || crd.Spec.Conversion.Webhook.ClientConfig == nil && crd.Spec.Conversion.Webhook.ClientConfig.CABundle == nil {
566+
return false, fmt.Errorf("ConversionWebhook not ready")
567+
}
568+
webhookCount++
569+
}
556570
}
557571
if webhookCount == 0 {
558-
a.logger.Info("Expected Webhook does not exist")
559572
return false, nil
560573
}
561574
}

0 commit comments

Comments
 (0)