Skip to content

Commit

Permalink
Merge pull request #1587 from p0lyn0mial/webhook-controller-checks-se…
Browse files Browse the repository at this point in the history 
…rvice-ca-annotation

OCPBUGS-24005: webhookcontroller: report when a webhook resource is missing a caBundle provided by the service-ca-operator
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] committed Dec 19, 2023
2 parents 420a0e9 + aa5d5ed commit d6f4bca
Show file tree
Hide file tree
Showing 9 changed files with 170 additions and 53 deletions.
16 changes: 8 additions & 8 deletions pkg/operator/webhooksupportabilitycontroller/conditions.go
View file
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
package webhooksupportabilitycontroller

const (
// MutatingAdmissionWebhookConfigurationDegradedType is true when there
// MutatingAdmissionWebhookConfigurationErrorType is true when there
// is a problem with a mutating admission webhook service.
MutatingAdmissionWebhookConfigurationDegradedType = "MutatingAdmissionWebhookConfigurationError"
MutatingAdmissionWebhookConfigurationErrorType = "MutatingAdmissionWebhookConfigurationError"

// ValidatingAdmissionWebhookConfigurationDegradedType is true when there
// ValidatingAdmissionWebhookConfigurationErrorType is true when there
// is a problem with a validating admission webhook service.
ValidatingAdmissionWebhookConfigurationDegradedType = "ValidatingAdmissionWebhookConfigurationError"
ValidatingAdmissionWebhookConfigurationErrorType = "ValidatingAdmissionWebhookConfigurationError"

// CRDConversionWebhookConfigurationDegradedType is true when there
// CRDConversionWebhookConfigurationErrorType is true when there
// is a problem with a custom resource definition conversion webhook service.
CRDConversionWebhookConfigurationDegradedType = "CRDConversionWebhookConfigurationError"
CRDConversionWebhookConfigurationErrorType = "CRDConversionWebhookConfigurationError"

// VirtualResourceAdmissionDegradedType is true when a dynamic admission webhook matches
// VirtualResourceAdmissionErrorType is true when a dynamic admission webhook matches
// a virtual resource.
VirtualResourceAdmissionDegradedType = "VirtualResourceAdmissionError"
VirtualResourceAdmissionErrorType = "VirtualResourceAdmissionError"
)

const (
Expand Down
23 changes: 21 additions & 2 deletions pkg/operator/webhooksupportabilitycontroller/degraded_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ import (
"k8s.io/klog/v2"
)

const (
// injectCABundleAnnotationName is the annotation used by the
// service-ca-operator to indicate which resources it should inject the CA into.
injectCABundleAnnotationName = "service.beta.openshift.io/inject-cabundle"
)

// webhookInfo generically represents a webhook
type webhookInfo struct {
Name string
Expand All @@ -25,6 +31,9 @@ type webhookInfo struct {
// TimeoutSeconds specifies the timeout for a webhook.
// After the timeout passes, the webhook call will be ignored or the API call will fail
TimeoutSeconds *int32
// HasServiceCaAnnotation indicates whether a webhook
// resource has been annotated for CABundle injection by the service-ca-operator
HasServiceCaAnnotation bool
}

// serviceReference generically represents a service reference
Expand Down Expand Up @@ -52,7 +61,7 @@ func (c *webhookSupportabilityController) updateWebhookConfigurationDegraded(ctx
serviceMsgs = append(serviceMsgs, msg)
continue
}
err = c.assertConnect(ctx, webhook.Name, webhook.Service, webhook.CABundle, webhook.TimeoutSeconds)
err = c.assertConnect(ctx, webhook.Name, webhook.Service, webhook.CABundle, webhook.HasServiceCaAnnotation, webhook.TimeoutSeconds)
if err != nil {
msg := fmt.Sprintf("%s: %s", webhook.Name, err)
if webhook.FailurePolicyIsIgnore {
Expand Down Expand Up @@ -97,7 +106,7 @@ func (c *webhookSupportabilityController) assertService(reference *serviceRefere
}

// assertConnect performs a dns lookup of service, opens a tcp connection, and performs a tls handshake.
func (c *webhookSupportabilityController) assertConnect(ctx context.Context, webhookName string, reference *serviceReference, caBundle []byte, webhookTimeoutSeconds *int32) error {
func (c *webhookSupportabilityController) assertConnect(ctx context.Context, webhookName string, reference *serviceReference, caBundle []byte, caBundleProvidedByServiceCA bool, webhookTimeoutSeconds *int32) error {
host := reference.Name + "." + reference.Namespace + ".svc"
port := "443"
if reference.Port != nil {
Expand All @@ -106,6 +115,9 @@ func (c *webhookSupportabilityController) assertConnect(ctx context.Context, web
rootCAs := x509.NewCertPool()
if len(caBundle) > 0 {
rootCAs.AppendCertsFromPEM(caBundle)
} else if caBundleProvidedByServiceCA {
err := fmt.Errorf("skipping checking the webhook via %q service because the caBundle (provided by the service-ca-operator) is empty. Please check the service-ca's logs if the issue persists", net.JoinHostPort(host, port))
return err
}
timeout := 10 * time.Second
if webhookTimeoutSeconds != nil {
Expand Down Expand Up @@ -142,3 +154,10 @@ func (c *webhookSupportabilityController) assertConnect(ctx context.Context, web
}
return err
}

func hasServiceCaAnnotation(annotations map[string]string) bool {
if annotations == nil {
return false
}
return strings.EqualFold(annotations[injectCABundleAnnotationName], "true")
}
22 changes: 12 additions & 10 deletions pkg/operator/webhooksupportabilitycontroller/degraded_webhook_admission.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

func (c *webhookSupportabilityController) updateMutatingAdmissionWebhookConfigurationDegraded(ctx context.Context) v1helpers.UpdateStatusFunc {
condition := operatorv1.OperatorCondition{
Type: MutatingAdmissionWebhookConfigurationDegradedType,
Type: MutatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionUnknown,
}
webhookConfigurations, err := c.mutatingWebhookLister.List(labels.Everything())
Expand All @@ -24,10 +24,11 @@ func (c *webhookSupportabilityController) updateMutatingAdmissionWebhookConfigur
for _, webhookConfiguration := range webhookConfigurations {
for _, webhook := range webhookConfiguration.Webhooks {
info := webhookInfo{
Name: webhook.Name,
CABundle: webhook.ClientConfig.CABundle,
FailurePolicyIsIgnore: webhook.FailurePolicy != nil && *webhook.FailurePolicy == admissionregistrationv1.Ignore,
TimeoutSeconds: webhook.TimeoutSeconds,
Name: webhook.Name,
CABundle: webhook.ClientConfig.CABundle,
HasServiceCaAnnotation: hasServiceCaAnnotation(webhookConfiguration.Annotations),
FailurePolicyIsIgnore: webhook.FailurePolicy != nil && *webhook.FailurePolicy == admissionregistrationv1.Ignore,
TimeoutSeconds: webhook.TimeoutSeconds,
}
if webhook.ClientConfig.Service != nil {
info.Service = &serviceReference{
Expand All @@ -44,7 +45,7 @@ func (c *webhookSupportabilityController) updateMutatingAdmissionWebhookConfigur

func (c *webhookSupportabilityController) updateValidatingAdmissionWebhookConfigurationDegradedStatus(ctx context.Context) v1helpers.UpdateStatusFunc {
condition := operatorv1.OperatorCondition{
Type: ValidatingAdmissionWebhookConfigurationDegradedType,
Type: ValidatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionUnknown,
}
webhookConfigurations, err := c.validatingWebhookLister.List(labels.Everything())
Expand All @@ -56,10 +57,11 @@ func (c *webhookSupportabilityController) updateValidatingAdmissionWebhookConfig
for _, webhookConfiguration := range webhookConfigurations {
for _, webhook := range webhookConfiguration.Webhooks {
info := webhookInfo{
Name: webhook.Name,
CABundle: webhook.ClientConfig.CABundle,
FailurePolicyIsIgnore: webhook.FailurePolicy != nil && (*webhook.FailurePolicy == v1.Ignore),
TimeoutSeconds: webhook.TimeoutSeconds,
Name: webhook.Name,
CABundle: webhook.ClientConfig.CABundle,
HasServiceCaAnnotation: hasServiceCaAnnotation(webhookConfiguration.Annotations),
FailurePolicyIsIgnore: webhook.FailurePolicy != nil && (*webhook.FailurePolicy == v1.Ignore),
TimeoutSeconds: webhook.TimeoutSeconds,
}

if webhook.ClientConfig.Service != nil {
Expand Down
82 changes: 69 additions & 13 deletions pkg/operator/webhooksupportabilitycontroller/degraded_webhook_admission_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func TestUpdateMutatingAdmissionWebhookConfigurationDegraded(t *testing.T) {
{
name: "None",
expected: operatorv1.OperatorCondition{
Type: MutatingAdmissionWebhookConfigurationDegradedType,
Type: MutatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionFalse,
},
},
Expand Down Expand Up @@ -65,7 +65,7 @@ func TestUpdateMutatingAdmissionWebhookConfigurationDegraded(t *testing.T) {
webhookServer("mwc30", "ns30", "svc30"),
},
expected: operatorv1.OperatorCondition{
Type: MutatingAdmissionWebhookConfigurationDegradedType,
Type: MutatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionFalse,
},
},
Expand All @@ -90,7 +90,7 @@ func TestUpdateMutatingAdmissionWebhookConfigurationDegraded(t *testing.T) {
},
webhookServers: []*mockWebhookServer{},
expected: operatorv1.OperatorCondition{
Type: MutatingAdmissionWebhookConfigurationDegradedType,
Type: MutatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionFalse,
},
},
Expand All @@ -115,7 +115,7 @@ func TestUpdateMutatingAdmissionWebhookConfigurationDegraded(t *testing.T) {
webhookServer("mwc30", "ns30", "svc30"),
},
expected: operatorv1.OperatorCondition{
Type: MutatingAdmissionWebhookConfigurationDegradedType,
Type: MutatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceNotReadyReason,
Message: `mw10: unable to find service svc10.ns10: service "svc10" not found\nmw20: (?:.*)?dial tcp: lookup svc20.ns20.svc on .+: no such host`,
Expand All @@ -133,7 +133,7 @@ func TestUpdateMutatingAdmissionWebhookConfigurationDegraded(t *testing.T) {
},
webhookServers: nil,
expected: operatorv1.OperatorCondition{
Type: MutatingAdmissionWebhookConfigurationDegradedType,
Type: MutatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceConnectionErrorReason,
Message: `mw10: (?:.*)?dial tcp: lookup svc10.ns10.svc on .+: no such host`,
Expand All @@ -153,7 +153,7 @@ func TestUpdateMutatingAdmissionWebhookConfigurationDegraded(t *testing.T) {
webhookServer("mwc10", "ns10", "svc10", doNotStart()),
},
expected: operatorv1.OperatorCondition{
Type: MutatingAdmissionWebhookConfigurationDegradedType,
Type: MutatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceConnectionErrorReason,
Message: `mw10: (?:.*)?dial tcp 127.0.0.1:[0-9]+: connect: connection refused`,
Expand All @@ -173,12 +173,33 @@ func TestUpdateMutatingAdmissionWebhookConfigurationDegraded(t *testing.T) {
webhookServer("mwc10", "ns10", "svc10", withWrongCABundle(t)),
},
expected: operatorv1.OperatorCondition{
Type: MutatingAdmissionWebhookConfigurationDegradedType,
Type: MutatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceConnectionErrorReason,
Message: `mw10: (?:.*)?x509: certificate signed by unknown authority`,
},
},
{
name: "CABundleNotYetProvided",
webhookConfigs: []*admissionregistrationv1.MutatingWebhookConfiguration{
mutatingWebhookConfiguration("mwc10",
withMutatingWebhook("mw10", withMutatingServiceReference("ns10", "svc10")),
withMutatingWebhookAnnotatedWithServiceCABundleInjection,
),
},
services: []*corev1.Service{
service("ns10", "svc10"),
},
webhookServers: []*mockWebhookServer{
webhookServer("mwc10", "ns10", "svc10", withEmptyCABundle),
},
expected: operatorv1.OperatorCondition{
Type: MutatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceConnectionErrorReason,
Message: `mw10: skipping checking the webhook via \"([^"]+)\" service because the caBundle \(provided by the service-ca-operator\) is empty. Please check the service-ca's logs if the issue persists`,
},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
Expand Down Expand Up @@ -262,7 +283,7 @@ func TestUpdateValidatingAdmissionWebhookConfigurationDegradedStatus(t *testing.
{
name: "None",
expected: operatorv1.OperatorCondition{
Type: ValidatingAdmissionWebhookConfigurationDegradedType,
Type: ValidatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionFalse,
},
},
Expand Down Expand Up @@ -296,7 +317,7 @@ func TestUpdateValidatingAdmissionWebhookConfigurationDegradedStatus(t *testing.
webhookServer("mwc30", "ns30", "svc30"),
},
expected: operatorv1.OperatorCondition{
Type: ValidatingAdmissionWebhookConfigurationDegradedType,
Type: ValidatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionFalse,
},
},
Expand All @@ -321,7 +342,7 @@ func TestUpdateValidatingAdmissionWebhookConfigurationDegradedStatus(t *testing.
webhookServer("mwc30", "ns30", "svc30"),
},
expected: operatorv1.OperatorCondition{
Type: ValidatingAdmissionWebhookConfigurationDegradedType,
Type: ValidatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceNotReadyReason,
Message: `mw10: unable to find service svc10.ns10: service \"svc10\" not found\nmw20: (?:.*)?dial tcp: lookup svc20.ns20.svc on .+: no such host`,
Expand All @@ -339,7 +360,7 @@ func TestUpdateValidatingAdmissionWebhookConfigurationDegradedStatus(t *testing.
},
webhookServers: nil,
expected: operatorv1.OperatorCondition{
Type: ValidatingAdmissionWebhookConfigurationDegradedType,
Type: ValidatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceConnectionErrorReason,
Message: `mw10: (?:.*)?dial tcp: lookup svc10.ns10.svc on .+: no such host`,
Expand All @@ -359,7 +380,7 @@ func TestUpdateValidatingAdmissionWebhookConfigurationDegradedStatus(t *testing.
webhookServer("mwc10", "ns10", "svc10", doNotStart()),
},
expected: operatorv1.OperatorCondition{
Type: ValidatingAdmissionWebhookConfigurationDegradedType,
Type: ValidatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceConnectionErrorReason,
Message: `mw10: (?:.*)?dial tcp 127.0.0.1:[0-9]+: connect: connection refused`,
Expand All @@ -379,12 +400,33 @@ func TestUpdateValidatingAdmissionWebhookConfigurationDegradedStatus(t *testing.
webhookServer("mwc10", "ns10", "svc10", withWrongCABundle(t)),
},
expected: operatorv1.OperatorCondition{
Type: ValidatingAdmissionWebhookConfigurationDegradedType,
Type: ValidatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceConnectionErrorReason,
Message: `mw10: (?:.*)?x509: certificate signed by unknown authority`,
},
},
{
name: "CABundleNotYetProvided",
webhookConfigs: []*admissionregistrationv1.ValidatingWebhookConfiguration{
validatingWebhookConfiguration("mwc10",
withValidatingWebhook("mw10", withValidatingServiceReference("ns10", "svc10")),
withValidatingWebhookAnnotatedWithServiceCABundleInjection,
),
},
services: []*corev1.Service{
service("ns10", "svc10"),
},
webhookServers: []*mockWebhookServer{
webhookServer("mwc10", "ns10", "svc10", withEmptyCABundle),
},
expected: operatorv1.OperatorCondition{
Type: ValidatingAdmissionWebhookConfigurationErrorType,
Status: operatorv1.ConditionTrue,
Reason: WebhookServiceConnectionErrorReason,
Message: `mw10: skipping checking the webhook via \"([^"]+)\" service because the caBundle \(provided by the service-ca-operator\) is empty. Please check the service-ca's logs if the issue persists`,
},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
Expand Down Expand Up @@ -466,6 +508,13 @@ func mutatingWebhookConfiguration(n string, options ...func(*admissionregistrati
return c
}

func withMutatingWebhookAnnotatedWithServiceCABundleInjection(c *admissionregistrationv1.MutatingWebhookConfiguration) {
if c.Annotations == nil {
c.Annotations = map[string]string{}
}
c.Annotations[injectCABundleAnnotationName] = "true"
}

func withMutatingWebhook(n string, options ...func(*admissionregistrationv1.MutatingWebhook)) func(*admissionregistrationv1.MutatingWebhookConfiguration) {
return func(c *admissionregistrationv1.MutatingWebhookConfiguration) {
w := &admissionregistrationv1.MutatingWebhook{
Expand Down Expand Up @@ -554,6 +603,13 @@ func withValidatingServiceReference(ns, n string) func(*admissionregistrationv1.
}
}

func withValidatingWebhookAnnotatedWithServiceCABundleInjection(c *admissionregistrationv1.ValidatingWebhookConfiguration) {
if c.Annotations == nil {
c.Annotations = map[string]string{}
}
c.Annotations[injectCABundleAnnotationName] = "true"
}

func service(ns, n string) *corev1.Service {
s := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: n},
Expand Down
7 changes: 4 additions & 3 deletions pkg/operator/webhooksupportabilitycontroller/degraded_webhook_conversion.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

func (c *webhookSupportabilityController) updateCRDConversionWebhookConfigurationDegraded(ctx context.Context) v1helpers.UpdateStatusFunc {
condition := operatorv1.OperatorCondition{
Type: CRDConversionWebhookConfigurationDegradedType,
Type: CRDConversionWebhookConfigurationErrorType,
Status: operatorv1.ConditionUnknown,
}
crds, err := c.crdLister.List(labels.Everything())
Expand All @@ -26,8 +26,9 @@ func (c *webhookSupportabilityController) updateCRDConversionWebhookConfiguratio
continue
}
info := webhookInfo{
Name: crd.Name,
CABundle: crd.Spec.Conversion.Webhook.ClientConfig.CABundle,
Name: crd.Name,
CABundle: crd.Spec.Conversion.Webhook.ClientConfig.CABundle,
HasServiceCaAnnotation: hasServiceCaAnnotation(crd.Annotations),
Service: &serviceReference{
Namespace: crd.Spec.Conversion.Webhook.ClientConfig.Service.Namespace,
Name: crd.Spec.Conversion.Webhook.ClientConfig.Service.Name,
Expand Down

0 comments on commit d6f4bca

Please sign in to comment.