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

OCPBUGS-24005: webhookcontroller: report when a webhook resource is missing a caBundle provided by the service-ca-operator #1587

Merged
merged 2 commits into from
Dec 19, 2023
Merged

OCPBUGS-24005: webhookcontroller: report when a webhook resource is missing a caBundle provided by the service-ca-operator #1587

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
119a16f
webhookcontroller: update degraded webhook condition types to errors
p0lyn0mial Nov 27, 2023
aa5d5ed
webhookcontroller: report when a webhook resource is missing a caBund…
p0lyn0mial Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions pkg/operator/webhooksupportabilitycontroller/conditions.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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