diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 562f9c89ed..091ca798fc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -340,7 +340,14 @@ rebase the branch on main, fixing any conflicts along the way before the code ca 1. Replace the names 1. Ensure you've correctly replaced the names in the kubebuilder annotation, ensure the plurality is correct ```go - // +kubebuilder:webhook:verbs=create;update,path=/mutate-v1alpha1-ingressgateway,mutating=true,failurePolicy=fail,groups=consul.hashicorp.com,resources=ingressgateways,versions=v1alpha1,name=mutate-ingressgateway.consul.hashicorp.com,webhookVersions=v1beta1,sideEffects=None + // +kubebuilder:webhook:verbs=create;update,path=/mutate-v1alpha1-ingressgateway,mutating=true,failurePolicy=fail,groups=consul.hashicorp.com,resources=ingressgateways,versions=v1alpha1,name=mutate-ingressgateway.consul.hashicorp.com,sideEffects=None,admissionReviewVersions=v1beta1;v1 + ``` +1. Ensure you update the path to match the annotation in the `SetupWithManager` method: + ```go + func (v *IngressGatewayWebhook) SetupWithManager(mgr ctrl.Manager) { + v.decoder = admission.NewDecoder(mgr.GetScheme()) + mgr.GetWebhookServer().Register("/mutate-v1alpha1-ingressgateway", &admission.Webhook{Handler: v}) +} ``` ### Update command.go @@ -362,16 +369,13 @@ rebase the branch on main, fixing any conflicts along the way before the code ca return 1 } ``` -1. Update `control-plane/subcommand/inject-connect/command.go` and add your webhook (the path should match the kubebuilder annotation): +1. Update `control-plane/subcommand/inject-connect/command.go` and add your webhook ```go - mgr.GetWebhookServer().Register("/mutate-v1alpha1-ingressgateway", - &webhook.Admission{Handler: &v1alpha1.IngressGatewayWebhook{ - Client: mgr.GetClient(), - ConsulClient: consulClient, - Logger: ctrl.Log.WithName("webhooks").WithName(common.IngressGateway), - EnableConsulNamespaces: c.flagEnableNamespaces, - EnableNSMirroring: c.flagEnableNSMirroring, - }}) + (&v1alpha1.IngressGatewayWebhook + Client: mgr.GetClient(), + Logger: ctrl.Log.WithName("webhooks").WithName(common.IngressGateway), + ConsulMeta: consulMeta, + }).SetupWithManager(mgr) ``` ### Generating YAML diff --git a/charts/consul/templates/connect-inject-validatingwebhookconfiguration.yaml b/charts/consul/templates/connect-inject-validatingwebhookconfiguration.yaml index 8d01ace911..92068bbf68 100644 --- a/charts/consul/templates/connect-inject-validatingwebhookconfiguration.yaml +++ b/charts/consul/templates/connect-inject-validatingwebhookconfiguration.yaml @@ -28,4 +28,20 @@ webhooks: name: {{ template "consul.fullname" . }}-connect-injector namespace: {{ .Release.Namespace }} path: /validate-v1alpha1-gatewaypolicy +- name: validate-registration.consul.hashicorp.com + matchPolicy: Equivalent + rules: + - operations: [ "CREATE" , "UPDATE" ] + apiGroups: [ "consul.hashicorp.com" ] + apiVersions: [ "v1alpha1" ] + resources: [ "registrations" ] + failurePolicy: Fail + sideEffects: None + admissionReviewVersions: + - v1 + clientConfig: + service: + name: {{ template "consul.fullname" . }}-connect-injector + namespace: {{ .Release.Namespace }} + path: /validate-v1alpha1-registration {{- end }} diff --git a/charts/consul/templates/crd-registrations.yaml b/charts/consul/templates/crd-registrations.yaml index c693977636..e1e45d3574 100644 --- a/charts/consul/templates/crd-registrations.yaml +++ b/charts/consul/templates/crd-registrations.yaml @@ -86,9 +86,7 @@ spec: udp: type: string required: - - deregisterCriticalServiceAfterDuration - intervalDuration - - timeoutDuration type: object exposedPort: type: integer @@ -116,7 +114,6 @@ spec: - checkId - definition - name - - node - serviceId - serviceName - status diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go index c253f25aa6..739f85e87d 100644 --- a/control-plane/api/v1alpha1/registration_types.go +++ b/control-plane/api/v1alpha1/registration_types.go @@ -111,7 +111,7 @@ type Locality struct { // HealthCheck is used to represent a single check. type HealthCheck struct { - Node string `json:"node"` + Node string `json:"node,omitempty"` CheckID string `json:"checkId"` Name string `json:"name"` Status string `json:"status"` @@ -142,8 +142,8 @@ type HealthCheckDefinition struct { OSService string `json:"osService,omitempty"` GRPCUseTLS bool `json:"grpcUseTLS,omitempty"` IntervalDuration string `json:"intervalDuration"` - TimeoutDuration string `json:"timeoutDuration"` - DeregisterCriticalServiceAfterDuration string `json:"deregisterCriticalServiceAfterDuration"` + TimeoutDuration string `json:"timeoutDuration,omitempty"` + DeregisterCriticalServiceAfterDuration string `json:"deregisterCriticalServiceAfterDuration,omitempty"` } // +kubebuilder:object:root=true @@ -309,3 +309,7 @@ func (r *Registration) SetSyncedCondition(status corev1.ConditionStatus, reason }, } } + +func (r *Registration) KubernetesName() string { + return r.ObjectMeta.Name +} diff --git a/control-plane/api/v1alpha1/registration_webhook.go b/control-plane/api/v1alpha1/registration_webhook.go new file mode 100644 index 0000000000..e2a69f3b0b --- /dev/null +++ b/control-plane/api/v1alpha1/registration_webhook.go @@ -0,0 +1,124 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package v1alpha1 + +import ( + "context" + "errors" + "fmt" + "net/http" + "time" + + "github.com/go-logr/logr" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/hashicorp/consul-k8s/control-plane/api/common" +) + +// +kubebuilder:object:generate=false + +type RegistrationWebhook struct { + Logger logr.Logger + + // ConsulMeta contains metadata specific to the Consul installation. + ConsulMeta common.ConsulMeta + + decoder *admission.Decoder + client.Client +} + +// +kubebuilder:webhook:verbs=create;update,path=/validate-v1alpha1-registration,mutating=false,failurePolicy=fail,groups=consul.hashicorp.com,resources=registrations,versions=v1alpha1,name=validate-registration.consul.hashicorp.com,sideEffects=None,admissionReviewVersions=v1beta1;v1 + +func (v *RegistrationWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { + resource := &Registration{} + decodeErr := v.decoder.Decode(req, resource) + if decodeErr != nil { + return admission.Errored(http.StatusBadRequest, decodeErr) + } + + var err error + + err = errors.Join(err, validateRequiredFields(resource)) + err = errors.Join(err, validateHealthChecks(resource)) + if err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + return admission.Allowed("registration is valid") +} + +func (v *RegistrationWebhook) SetupWithManager(mgr ctrl.Manager) { + v.Logger.Info("setting up registration webhook") + v.decoder = admission.NewDecoder(mgr.GetScheme()) + mgr.GetWebhookServer().Register("/validate-v1alpha1-registration", &admission.Webhook{Handler: v}) +} + +func validateRequiredFields(registration *Registration) error { + var err error + if registration.Spec.Node == "" { + err = errors.Join(err, errors.New("registration.Spec.Node is required")) + } + if registration.Spec.Service.Name == "" { + err = errors.Join(err, errors.New("registration.Spec.Service.Name is required")) + } + if registration.Spec.Address == "" { + err = errors.Join(err, errors.New("registration.Spec.Address is required")) + } + + if err != nil { + return err + } + return nil +} + +var validStatuses = map[string]struct{}{ + "passing": {}, + "warning": {}, + "critical": {}, +} + +func validateHealthChecks(registration *Registration) error { + if registration.Spec.HealthCheck == nil { + return nil + } + + var err error + + if registration.Spec.HealthCheck.Name == "" { + err = errors.Join(err, errors.New("registration.Spec.HealthCheck.Name is required")) + } + + // status must be one "passing", "warning", or "critical" + if _, ok := validStatuses[registration.Spec.HealthCheck.Status]; !ok { + err = errors.Join(err, fmt.Errorf("invalid registration.Spec.HealthCheck.Status value, must be 'passing', 'warning', or 'critical', actual: %q", registration.Spec.HealthCheck.Status)) + } + + // parse all durations and check for any errors + _, parseErr := time.ParseDuration(registration.Spec.HealthCheck.Definition.IntervalDuration) + if parseErr != nil { + err = errors.Join(err, fmt.Errorf("invalid registration.Spec.HealthCheck.Definition.IntervalDuration value: %q", registration.Spec.HealthCheck.Definition.IntervalDuration)) + } + + if registration.Spec.HealthCheck.Definition.TimeoutDuration != "" { + _, timeoutErr := time.ParseDuration(registration.Spec.HealthCheck.Definition.TimeoutDuration) + if timeoutErr != nil { + err = errors.Join(err, fmt.Errorf("invalid registration.Spec.HealthCheck.Definition.TimeoutDuration value: %q", registration.Spec.HealthCheck.Definition.TimeoutDuration)) + } + } + + if registration.Spec.HealthCheck.Definition.DeregisterCriticalServiceAfterDuration != "" { + _, deregCriticalErr := time.ParseDuration(registration.Spec.HealthCheck.Definition.DeregisterCriticalServiceAfterDuration) + if deregCriticalErr != nil { + err = errors.Join(err, fmt.Errorf("invalid registration.Spec.HealthCheck.Definition.DeregisterCriticalServiceAfterDuration value: %q", registration.Spec.HealthCheck.Definition.DeregisterCriticalServiceAfterDuration)) + } + } + + if err != nil { + return err + } + + return nil +} diff --git a/control-plane/api/v1alpha1/registration_webhook_test.go b/control-plane/api/v1alpha1/registration_webhook_test.go new file mode 100644 index 0000000000..8883cbda10 --- /dev/null +++ b/control-plane/api/v1alpha1/registration_webhook_test.go @@ -0,0 +1,330 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package v1alpha1 + +import ( + "context" + "encoding/json" + "testing" + + logrtest "github.com/go-logr/logr/testr" + "github.com/stretchr/testify/require" + admissionv1 "k8s.io/api/admission/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +func TestValidateRegistration(t *testing.T) { + cases := map[string]struct { + newResource *Registration + expectedToAllow bool + expectedErrMessage string + }{ + "valid with health check, status 'passing'": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "node-virtual", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: &HealthCheck{ + Name: "check name", + Status: "passing", + Definition: HealthCheckDefinition{ + IntervalDuration: "10s", + }, + }, + }, + }, + expectedToAllow: true, + }, + "valid with health check, status 'warning'": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "node-virtual", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: &HealthCheck{ + Name: "check name", + Status: "warning", + Definition: HealthCheckDefinition{ + IntervalDuration: "10s", + }, + }, + }, + }, + expectedToAllow: true, + }, + "valid with health check, status 'critical'": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "node-virtual", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: &HealthCheck{ + Name: "check name", + Status: "critical", + Definition: HealthCheckDefinition{ + IntervalDuration: "10s", + }, + }, + }, + }, + expectedToAllow: true, + }, + "valid without health check": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "node-virtual", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: nil, + }, + }, + expectedToAllow: true, + }, + "invalid, missing node field": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: nil, + }, + }, + expectedToAllow: false, + expectedErrMessage: "registration.Spec.Node is required", + }, + "invalid, missing address field": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "test-node", + Address: "", + Service: Service{Name: "test-service"}, + HealthCheck: nil, + }, + }, + expectedToAllow: false, + expectedErrMessage: "registration.Spec.Address is required", + }, + "invalid, missing service.name field": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "test-node", + Address: "10.2.2.1", + Service: Service{Name: ""}, + HealthCheck: nil, + }, + }, + expectedToAllow: false, + expectedErrMessage: "registration.Spec.Service.Name is required", + }, + "invalid, health check is set and name is missing": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "test-node", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: &HealthCheck{ + Name: "", + Status: "passing", + Definition: HealthCheckDefinition{ + IntervalDuration: "10s", + }, + }, + }, + }, + expectedToAllow: false, + expectedErrMessage: "registration.Spec.HealthCheck.Name is required", + }, + "invalid, health check is set and intervalDuration is missing": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "test-node", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: &HealthCheck{ + Name: "check name", + Status: "passing", + Definition: HealthCheckDefinition{ + IntervalDuration: "", + }, + }, + }, + }, + expectedToAllow: false, + expectedErrMessage: "invalid registration.Spec.HealthCheck.Definition.IntervalDuration value: \"\"", + }, + "invalid, health check is set and intervalDuration is invalid duration type": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "test-node", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: &HealthCheck{ + Name: "check name", + Status: "passing", + Definition: HealthCheckDefinition{ + IntervalDuration: "150", + }, + }, + }, + }, + expectedToAllow: false, + expectedErrMessage: "invalid registration.Spec.HealthCheck.Definition.IntervalDuration value: \"150\"", + }, + "invalid, health check is set and timeoutDuration is invalid duration type": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "test-node", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: &HealthCheck{ + Name: "check name", + Status: "passing", + Definition: HealthCheckDefinition{ + IntervalDuration: "10s", + TimeoutDuration: "150", + }, + }, + }, + }, + expectedToAllow: false, + expectedErrMessage: "invalid registration.Spec.HealthCheck.Definition.TimeoutDuration value: \"150\"", + }, + "invalid, health check is set and deregisterCriticalServiceAfterDuration is invalid duration type": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "test-node", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: &HealthCheck{ + Name: "check name", + Status: "passing", + Definition: HealthCheckDefinition{ + IntervalDuration: "10s", + TimeoutDuration: "150s", + DeregisterCriticalServiceAfterDuration: "40", + }, + }, + }, + }, + expectedToAllow: false, + expectedErrMessage: "invalid registration.Spec.HealthCheck.Definition.DeregisterCriticalServiceAfterDuration value: \"40\"", + }, + "invalid, health check is set and status is not 'passing', 'critical', or 'warning'": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "test-node", + Address: "10.2.2.1", + Service: Service{Name: "test-service"}, + HealthCheck: &HealthCheck{ + Name: "check name", + Status: "wrong", + Definition: HealthCheckDefinition{ + IntervalDuration: "10s", + }, + }, + }, + }, + expectedToAllow: false, + expectedErrMessage: "invalid registration.Spec.HealthCheck.Status value, must be 'passing', 'warning', or 'critical', actual: \"wrong\"", + }, + "everything that can go wrong has gone wrong": { + newResource: &Registration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: RegistrationSpec{ + Node: "", + Address: "", + Service: Service{Name: ""}, + HealthCheck: &HealthCheck{ + Name: "", + Status: "wrong", + Definition: HealthCheckDefinition{ + IntervalDuration: "10", + TimeoutDuration: "150", + DeregisterCriticalServiceAfterDuration: "40", + }, + }, + }, + }, + expectedToAllow: false, + expectedErrMessage: "registration.Spec.Node is required\nregistration.Spec.Service.Name is required\nregistration.Spec.Address is required\nregistration.Spec.HealthCheck.Name is required\ninvalid registration.Spec.HealthCheck.Status value, must be 'passing', 'warning', or 'critical', actual: \"wrong\"\ninvalid registration.Spec.HealthCheck.Definition.IntervalDuration value: \"10\"\ninvalid registration.Spec.HealthCheck.Definition.TimeoutDuration value: \"150\"\ninvalid registration.Spec.HealthCheck.Definition.DeregisterCriticalServiceAfterDuration value: \"40\"", + }, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + marshalledRequestObject, err := json.Marshal(c.newResource) + require.NoError(t, err) + s := runtime.NewScheme() + s.AddKnownTypes(GroupVersion, &Registration{}, &RegistrationList{}) + client := fake.NewClientBuilder().WithScheme(s).Build() + decoder := admission.NewDecoder(s) + + validator := &RegistrationWebhook{ + Client: client, + Logger: logrtest.New(t), + decoder: decoder, + } + response := validator.Handle(ctx, admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: c.newResource.KubernetesName(), + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: marshalledRequestObject, + }, + }, + }) + + require.Equal(t, c.expectedToAllow, response.Allowed) + if c.expectedErrMessage != "" { + require.Equal(t, c.expectedErrMessage, response.AdmissionResponse.Result.Message) + } + }) + } +} diff --git a/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml b/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml index 41b11ae569..df8970512b 100644 --- a/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml +++ b/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml @@ -82,9 +82,7 @@ spec: udp: type: string required: - - deregisterCriticalServiceAfterDuration - intervalDuration - - timeoutDuration type: object exposedPort: type: integer @@ -112,7 +110,6 @@ spec: - checkId - definition - name - - node - serviceId - serviceName - status diff --git a/control-plane/config/webhook/manifests.yaml b/control-plane/config/webhook/manifests.yaml index a4b3aaadd0..8b94fac6fd 100644 --- a/control-plane/config/webhook/manifests.yaml +++ b/control-plane/config/webhook/manifests.yaml @@ -454,3 +454,24 @@ webhooks: resources: - gatewaypolicies sideEffects: None +- admissionReviewVersions: + - v1beta1 + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-v1alpha1-registration + failurePolicy: Fail + name: validate-registration.consul.hashicorp.com + rules: + - apiGroups: + - consul.hashicorp.com + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - registration + sideEffects: None diff --git a/control-plane/subcommand/inject-connect/v1controllers.go b/control-plane/subcommand/inject-connect/v1controllers.go index df7fff50c1..82e0391cd7 100644 --- a/control-plane/subcommand/inject-connect/v1controllers.go +++ b/control-plane/subcommand/inject-connect/v1controllers.go @@ -476,6 +476,12 @@ func (c *Command) configureV1Controllers(ctx context.Context, mgr manager.Manage ConsulMeta: consulMeta, }).SetupWithManager(mgr) + (&v1alpha1.RegistrationWebhook{ + Client: mgr.GetClient(), + Logger: ctrl.Log.WithName("webhooks").WithName(apicommon.Registration), + ConsulMeta: consulMeta, + }).SetupWithManager(mgr) + if c.flagEnableWebhookCAUpdate { err = c.updateWebhookCABundle(ctx) if err != nil {