From 9231df198c2e973f4c653d525b7f3a671e8d1d54 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Mon, 22 Apr 2024 14:54:08 -0400 Subject: [PATCH 01/17] service is registering --- .../templates/connect-inject-clusterrole.yaml | 2 + .../consul/templates/crd-registrations.yaml | 61 ++++++++++++ control-plane/api/common/common.go | 1 + .../api/v1alpha1/registration_types.go | 50 ++++++++++ .../api/v1alpha1/zz_generated.deepcopy.go | 73 ++++++++++++++ .../consul.hashicorp.com_registrations.yaml | 56 +++++++++++ .../configentries/registrations_controller.go | 94 +++++++++++++++++++ .../inject-connect/v1controllers.go | 11 +++ 8 files changed, 348 insertions(+) create mode 100644 charts/consul/templates/crd-registrations.yaml create mode 100644 control-plane/api/v1alpha1/registration_types.go create mode 100644 control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml create mode 100644 control-plane/controllers/configentries/registrations_controller.go diff --git a/charts/consul/templates/connect-inject-clusterrole.yaml b/charts/consul/templates/connect-inject-clusterrole.yaml index be816ff391..9c8596b05b 100644 --- a/charts/consul/templates/connect-inject-clusterrole.yaml +++ b/charts/consul/templates/connect-inject-clusterrole.yaml @@ -32,6 +32,7 @@ rules: - routetimeoutfilters - routeauthfilters - gatewaypolicies + - registrations {{- if .Values.global.peering.enabled }} - peeringacceptors - peeringdialers @@ -61,6 +62,7 @@ rules: - terminatinggateways/status - samenessgroups/status - controlplanerequestlimits/status + - registrations/status {{- if .Values.global.peering.enabled }} - peeringacceptors/status - peeringdialers/status diff --git a/charts/consul/templates/crd-registrations.yaml b/charts/consul/templates/crd-registrations.yaml new file mode 100644 index 0000000000..1549988a62 --- /dev/null +++ b/charts/consul/templates/crd-registrations.yaml @@ -0,0 +1,61 @@ +{{- if .Values.connectInject.enabled }} +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.12.1 + labels: + app: {{ template "consul.name" . }} + chart: {{ template "consul.chart" . }} + heritage: {{ .Release.Service }} + release: {{ .Release.Name }} + component: crd + name: registrations.consul.hashicorp.com +spec: + group: consul.hashicorp.com + names: + kind: Registration + listKind: RegistrationList + plural: registrations + singular: registration + scope: Cluster + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + description: Registration defines the. + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: Spec defines the desired state of Registration. + properties: + address: + type: string + name: + type: string + namespace: + type: string + partition: + type: string + port: + type: integer + required: + - address + - name + - port + type: object + type: object + served: true + storage: true +{{- end }} diff --git a/control-plane/api/common/common.go b/control-plane/api/common/common.go index 730fd622ac..077be60a9b 100644 --- a/control-plane/api/common/common.go +++ b/control-plane/api/common/common.go @@ -28,6 +28,7 @@ const ( ControlPlaneRequestLimit string = "controlplanerequestlimit" RouteAuthFilter string = "routeauthfilter" GatewayPolicy string = "gatewaypolicy" + Registration string = "registration" // V2 resources. TrafficPermissions string = "trafficpermissions" diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go new file mode 100644 index 0000000000..5b7667e8cf --- /dev/null +++ b/control-plane/api/v1alpha1/registration_types.go @@ -0,0 +1,50 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package v1alpha1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func init() { + SchemeBuilder.Register(&Registration{}, &RegistrationList{}) +} + +// +genclient +// +kubebuilder:object:root=true +// +kubebuilder:resource:scope=Cluster + +// Registration defines the. +type Registration struct { + // Standard Kubernetes resource metadata. + metav1.TypeMeta `json:",inline"` + + // Standard object's metadata. + metav1.ObjectMeta `json:"metadata,omitempty"` + + // Spec defines the desired state of Registration. + Spec RegistrationSpec `json:"spec,omitempty"` +} + +// +k8s:deepcopy-gen=true + +// RegistrationSpec specifies the desired state of the Config CRD. +type RegistrationSpec struct { + Name string `json:"name"` + Port int `json:"port"` + Address string `json:"address"` + Namespace string `json:"namespace,omitempty"` + Partition string `json:"partition,omitempty"` +} + +// +kubebuilder:object:root=true + +// RegistrationList is a list of Config resources. +type RegistrationList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata"` + + // Items is the list of Configs. + Items []Registration `json:"items"` +} diff --git a/control-plane/api/v1alpha1/zz_generated.deepcopy.go b/control-plane/api/v1alpha1/zz_generated.deepcopy.go index 2a1854d178..e675b98bab 100644 --- a/control-plane/api/v1alpha1/zz_generated.deepcopy.go +++ b/control-plane/api/v1alpha1/zz_generated.deepcopy.go @@ -2483,6 +2483,79 @@ func (in *ReadWriteRatesConfig) DeepCopy() *ReadWriteRatesConfig { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Registration) DeepCopyInto(out *Registration) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) + out.Spec = in.Spec +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Registration. +func (in *Registration) DeepCopy() *Registration { + if in == nil { + return nil + } + out := new(Registration) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *Registration) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RegistrationList) DeepCopyInto(out *RegistrationList) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ListMeta.DeepCopyInto(&out.ListMeta) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]Registration, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RegistrationList. +func (in *RegistrationList) DeepCopy() *RegistrationList { + if in == nil { + return nil + } + out := new(RegistrationList) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *RegistrationList) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RegistrationSpec) DeepCopyInto(out *RegistrationSpec) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RegistrationSpec. +func (in *RegistrationSpec) DeepCopy() *RegistrationSpec { + if in == nil { + return nil + } + out := new(RegistrationSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RemoteJWKS) DeepCopyInto(out *RemoteJWKS) { *out = *in diff --git a/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml b/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml new file mode 100644 index 0000000000..a0e5d53682 --- /dev/null +++ b/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml @@ -0,0 +1,56 @@ +# Copyright (c) HashiCorp, Inc. +# SPDX-License-Identifier: MPL-2.0 + +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.12.1 + name: registrations.consul.hashicorp.com +spec: + group: consul.hashicorp.com + names: + kind: Registration + listKind: RegistrationList + plural: registrations + singular: registration + scope: Cluster + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + description: Registration defines the. + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: Spec defines the desired state of Registration. + properties: + address: + type: string + name: + type: string + namespace: + type: string + partition: + type: string + port: + type: integer + required: + - address + - name + - port + type: object + type: object + served: true + storage: true diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go new file mode 100644 index 0000000000..17ce5fc455 --- /dev/null +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -0,0 +1,94 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package configentries + +import ( + "context" + "fmt" + + "github.com/go-logr/logr" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + capi "github.com/hashicorp/consul/api" + + "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" + consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" + "github.com/hashicorp/consul-k8s/control-plane/consul" +) + +var _ Controller = (*RegistrationsController)(nil) + +// RegistrationsController is the controller for Registrations resources. +type RegistrationsController struct { + client.Client + FinalizerPatcher + Log logr.Logger + Scheme *runtime.Scheme + ConsulClientConfig *consul.Config + ConsulServerConnMgr consul.ServerConnectionManager +} + +// +kubebuilder:rbac:groups=consul.hashicorp.com,resources=servicerouters,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=consul.hashicorp.com,resources=servicerouters/status,verbs=get;update;patch + +func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + log := r.Log.V(1).WithValues("registration", req.NamespacedName) + log.Info("Reconciling Registaration") + + var registration v1alpha1.Registration + // get the gateway + if err := r.Client.Get(ctx, req.NamespacedName, ®istration); err != nil { + if !k8serrors.IsNotFound(err) { + log.Error(err, "unable to get registration") + } + return ctrl.Result{}, client.IgnoreNotFound(err) + } + + client, err := consul.NewClientFromConnMgr(r.ConsulClientConfig, r.ConsulServerConnMgr) + if err != nil { + log.Error(err, "error initializing consul client") + return ctrl.Result{}, err + } + + regReq := &capi.CatalogRegistration{ + Node: "node-virtual", + Address: "127.0.0.1", + TaggedAddresses: map[string]string{}, + NodeMeta: map[string]string{}, + Datacenter: "", + Service: &capi.AgentService{ + ID: fmt.Sprintf("%s-1234", registration.Spec.Name), + Service: registration.Spec.Name, + Address: registration.Spec.Address, + Port: registration.Spec.Port, + }, + SkipNodeUpdate: false, + Partition: "", + Locality: &capi.Locality{}, + } + + _, err = client.Catalog().Register(regReq, nil) + if err != nil { + log.Error(err, "error registering service", "svcName", regReq.Service.Service) + return ctrl.Result{}, err + } + + return ctrl.Result{}, nil +} + +func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger { + return r.Log.WithValues("request", name) +} + +func (r *RegistrationsController) UpdateStatus(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + return r.Status().Update(ctx, obj, opts...) +} + +func (r *RegistrationsController) SetupWithManager(mgr ctrl.Manager) error { + return setupWithManager(mgr, &consulv1alpha1.Registration{}, r) +} diff --git a/control-plane/subcommand/inject-connect/v1controllers.go b/control-plane/subcommand/inject-connect/v1controllers.go index 288ba92189..b7e3a07f0d 100644 --- a/control-plane/subcommand/inject-connect/v1controllers.go +++ b/control-plane/subcommand/inject-connect/v1controllers.go @@ -285,6 +285,17 @@ func (c *Command) configureV1Controllers(ctx context.Context, mgr manager.Manage return err } + if err := (&controllers.RegistrationsController{ + Client: mgr.GetClient(), + ConsulClientConfig: consulConfig, + ConsulServerConnMgr: watcher, + Scheme: mgr.GetScheme(), + Log: ctrl.Log.WithName("controller").WithName(apicommon.Registration), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", apicommon.Registration) + return err + } + if err := mgr.AddReadyzCheck("ready", webhook.ReadinessCheck{CertDir: c.flagCertDir}.Ready); err != nil { setupLog.Error(err, "unable to create readiness check", "controller", endpoints.Controller{}) return err From 202b84b59f71fece78c5f253120285cf0710ed5d Mon Sep 17 00:00:00 2001 From: jm96441n Date: Mon, 22 Apr 2024 17:06:23 -0400 Subject: [PATCH 02/17] add all the fields --- .../consul/templates/crd-registrations.yaml | 168 +++++++++++++++++- .../api/v1alpha1/registration_types.go | 94 +++++++++- .../api/v1alpha1/zz_generated.deepcopy.go | 160 ++++++++++++++++- .../consul.hashicorp.com_registrations.yaml | 168 +++++++++++++++++- .../configentries/registrations_controller.go | 8 +- 5 files changed, 570 insertions(+), 28 deletions(-) diff --git a/charts/consul/templates/crd-registrations.yaml b/charts/consul/templates/crd-registrations.yaml index 1549988a62..eb244d2a28 100644 --- a/charts/consul/templates/crd-registrations.yaml +++ b/charts/consul/templates/crd-registrations.yaml @@ -42,18 +42,168 @@ spec: properties: address: type: string - name: + checks: + items: + description: HealthCheck is used to represent a single check + properties: + definition: + description: HealthCheckDefinition is used to store the details + about a health check's execution. + properties: + body: + type: string + dereigsterCriticalServiceAfterDuration: + type: string + grpc: + type: string + grpcUseTLS: + type: boolean + header: + additionalProperties: + items: + type: string + type: array + type: object + http: + type: string + intervalDuration: + type: string + method: + type: string + osService: + type: string + tcp: + type: string + tcpUseTLS: + type: boolean + timeoutDuration: + type: string + tlsServerName: + type: string + tlsSkipVerify: + type: boolean + udp: + type: string + required: + - dereigsterCriticalServiceAfterDuration + - intervalDuration + - timeoutDuration + type: object + exposedPort: + type: integer + id: + type: string + name: + type: string + namespace: + type: string + node: + type: string + notes: + type: string + output: + type: string + partition: + type: string + serviceId: + type: string + serviceName: + type: string + serviceTags: + items: + type: string + type: array + status: + type: string + type: + type: string + required: + - id + - name + - node + - serviceId + - serviceName + - status + type: object + type: array + datacenter: type: string - namespace: + id: type: string - partition: + node: type: string - port: - type: integer - required: - - address - - name - - port + nodeMeta: + additionalProperties: + type: string + type: object + service: + properties: + address: + type: string + contentHash: + type: string + datacenter: + type: string + enableTagOverride: + type: boolean + id: + type: string + locality: + properties: + region: + type: string + zone: + type: string + type: object + meta: + additionalProperties: + type: string + type: object + name: + type: string + namespace: + type: string + partition: + type: string + port: + type: integer + socketPath: + type: string + taggedAddresses: + additionalProperties: + properties: + address: + type: string + port: + type: integer + required: + - address + - port + type: object + type: object + tags: + items: + type: string + type: array + weights: + properties: + passing: + type: integer + warning: + type: integer + required: + - passing + - warning + type: object + required: + - address + - name + - port + type: object + taggedAddresses: + additionalProperties: + type: string + type: object type: object type: object served: true diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go index 5b7667e8cf..9d7a66ab29 100644 --- a/control-plane/api/v1alpha1/registration_types.go +++ b/control-plane/api/v1alpha1/registration_types.go @@ -31,11 +31,95 @@ type Registration struct { // RegistrationSpec specifies the desired state of the Config CRD. type RegistrationSpec struct { - Name string `json:"name"` - Port int `json:"port"` - Address string `json:"address"` - Namespace string `json:"namespace,omitempty"` - Partition string `json:"partition,omitempty"` + ID string `json:"id,omitempty"` + Node string `json:"node,omitempty"` + Address string `json:"address,omitempty"` + TaggedAddresses map[string]string `json:"taggedAddresses,omitempty"` + NodeMeta map[string]string `json:"nodeMeta,omitempty"` + Datacenter string `json:"datacenter,omitempty"` + Service Service `json:"service,omitempty"` + HealthChecks []HealthCheck `json:"checks,omitempty"` +} + +// +k8s:deepcopy-gen=true + +type Service struct { + ID string `json:"id,omitempty"` + Name string `json:"name"` + Tags []string `json:"tags,omitempty"` + Meta map[string]string `json:"meta,omitempty"` + Port int `json:"port"` + Address string `json:"address"` + SocketPath string `json:"socketPath,omitempty"` + TaggedAddresses map[string]ServiceAddress `json:"taggedAddresses,omitempty"` + Weights Weights `json:"weights,omitempty"` + EnableTagOverride bool `json:"enableTagOverride,omitempty"` + ContentHash string `json:"contentHash,omitempty"` + Datacenter string `json:"datacenter,omitempty"` + Locality Locality `json:"locality,omitempty"` + Namespace string `json:"namespace,omitempty"` + Partition string `json:"partition,omitempty"` +} + +// +k8s:deepcopy-gen=true + +type ServiceAddress struct { + Address string `json:"address"` + Port int `json:"port"` +} + +// +k8s:deepcopy-gen=true + +type Weights struct { + Passing int `json:"passing"` + Warning int `json:"warning"` +} + +// +k8s:deepcopy-gen=true + +type Locality struct { + Region string `json:"region,omitempty"` + Zone string `json:"zone,omitempty"` +} + +// +k8s:deepcopy-gen=true + +// HealthCheck is used to represent a single check +type HealthCheck struct { + Node string `json:"node"` + CheckID string `json:"id"` + Name string `json:"name"` + Status string `json:"status"` + Notes string `json:"notes,omitempty"` + Output string `json:"output,omitempty"` + ServiceID string `json:"serviceId"` + ServiceName string `json:"serviceName"` + ServiceTags []string `json:"serviceTags,omitempty"` + Type string `json:"type,omitempty"` + Namespace string `json:"namespace,omitempty"` + Partition string `json:"partition,omitempty"` + ExposedPort int `json:"exposedPort,omitempty"` + Definition *HealthCheckDefinition `json:"definition,omitempty"` +} + +// HealthCheckDefinition is used to store the details about +// a health check's execution. +type HealthCheckDefinition struct { + HTTP string `json:"http,omitempty"` + Header map[string][]string `json:"header,omitempty"` + Method string `json:"method,omitempty"` + Body string `json:"body,omitempty"` + TLSServerName string `json:"tlsServerName,omitempty"` + TLSSkipVerify bool `json:"tlsSkipVerify,omitempty"` + TCP string `json:"tcp,omitempty"` + TCPUseTLS bool `json:"tcpUseTLS,omitempty"` + UDP string `json:"udp,omitempty"` + GRPC string `json:"grpc,omitempty"` + OSService string `json:"osService,omitempty"` + GRPCUseTLS bool `json:"grpcUseTLS,omitempty"` + IntervalDuration string `json:"intervalDuration"` + TimeoutDuration string `json:"timeoutDuration"` + DeregisterCriticalServiceAfterDuration string `json:"dereigsterCriticalServiceAfterDuration"` } // +kubebuilder:object:root=true diff --git a/control-plane/api/v1alpha1/zz_generated.deepcopy.go b/control-plane/api/v1alpha1/zz_generated.deepcopy.go index e675b98bab..49b4f3b651 100644 --- a/control-plane/api/v1alpha1/zz_generated.deepcopy.go +++ b/control-plane/api/v1alpha1/zz_generated.deepcopy.go @@ -881,6 +881,61 @@ func (in *HashPolicy) DeepCopy() *HashPolicy { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *HealthCheck) DeepCopyInto(out *HealthCheck) { + *out = *in + if in.ServiceTags != nil { + in, out := &in.ServiceTags, &out.ServiceTags + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Definition != nil { + in, out := &in.Definition, &out.Definition + *out = new(HealthCheckDefinition) + (*in).DeepCopyInto(*out) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HealthCheck. +func (in *HealthCheck) DeepCopy() *HealthCheck { + if in == nil { + return nil + } + out := new(HealthCheck) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *HealthCheckDefinition) DeepCopyInto(out *HealthCheckDefinition) { + *out = *in + if in.Header != nil { + in, out := &in.Header, &out.Header + *out = make(map[string][]string, len(*in)) + for key, val := range *in { + var outVal []string + if val == nil { + (*out)[key] = nil + } else { + in, out := &val, &outVal + *out = make([]string, len(*in)) + copy(*out, *in) + } + (*out)[key] = outVal + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HealthCheckDefinition. +func (in *HealthCheckDefinition) DeepCopy() *HealthCheckDefinition { + if in == nil { + return nil + } + out := new(HealthCheckDefinition) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *IngressGateway) DeepCopyInto(out *IngressGateway) { *out = *in @@ -1735,6 +1790,21 @@ func (in *LocalJWKS) DeepCopy() *LocalJWKS { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Locality) DeepCopyInto(out *Locality) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Locality. +func (in *Locality) DeepCopy() *Locality { + if in == nil { + return nil + } + out := new(Locality) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Mesh) DeepCopyInto(out *Mesh) { *out = *in @@ -2488,7 +2558,7 @@ func (in *Registration) DeepCopyInto(out *Registration) { *out = *in out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Spec = in.Spec + in.Spec.DeepCopyInto(&out.Spec) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Registration. @@ -2544,6 +2614,28 @@ func (in *RegistrationList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RegistrationSpec) DeepCopyInto(out *RegistrationSpec) { *out = *in + if in.TaggedAddresses != nil { + in, out := &in.TaggedAddresses, &out.TaggedAddresses + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + if in.NodeMeta != nil { + in, out := &in.NodeMeta, &out.NodeMeta + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + in.Service.DeepCopyInto(&out.Service) + if in.HealthChecks != nil { + in, out := &in.HealthChecks, &out.HealthChecks + *out = make([]HealthCheck, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RegistrationSpec. @@ -3029,6 +3121,57 @@ func (in *SecretRefStatus) DeepCopy() *SecretRefStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Service) DeepCopyInto(out *Service) { + *out = *in + if in.Tags != nil { + in, out := &in.Tags, &out.Tags + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Meta != nil { + in, out := &in.Meta, &out.Meta + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + if in.TaggedAddresses != nil { + in, out := &in.TaggedAddresses, &out.TaggedAddresses + *out = make(map[string]ServiceAddress, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + out.Weights = in.Weights + out.Locality = in.Locality +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Service. +func (in *Service) DeepCopy() *Service { + if in == nil { + return nil + } + out := new(Service) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ServiceAddress) DeepCopyInto(out *ServiceAddress) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAddress. +func (in *ServiceAddress) DeepCopy() *ServiceAddress { + if in == nil { + return nil + } + out := new(ServiceAddress) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ServiceConsumer) DeepCopyInto(out *ServiceConsumer) { *out = *in @@ -4107,3 +4250,18 @@ func (in *Upstreams) DeepCopy() *Upstreams { in.DeepCopyInto(out) return out } + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Weights) DeepCopyInto(out *Weights) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Weights. +func (in *Weights) DeepCopy() *Weights { + if in == nil { + return nil + } + out := new(Weights) + in.DeepCopyInto(out) + return out +} 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 a0e5d53682..0489c4f2ad 100644 --- a/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml +++ b/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml @@ -38,18 +38,168 @@ spec: properties: address: type: string - name: + checks: + items: + description: HealthCheck is used to represent a single check + properties: + definition: + description: HealthCheckDefinition is used to store the details + about a health check's execution. + properties: + body: + type: string + dereigsterCriticalServiceAfterDuration: + type: string + grpc: + type: string + grpcUseTLS: + type: boolean + header: + additionalProperties: + items: + type: string + type: array + type: object + http: + type: string + intervalDuration: + type: string + method: + type: string + osService: + type: string + tcp: + type: string + tcpUseTLS: + type: boolean + timeoutDuration: + type: string + tlsServerName: + type: string + tlsSkipVerify: + type: boolean + udp: + type: string + required: + - dereigsterCriticalServiceAfterDuration + - intervalDuration + - timeoutDuration + type: object + exposedPort: + type: integer + id: + type: string + name: + type: string + namespace: + type: string + node: + type: string + notes: + type: string + output: + type: string + partition: + type: string + serviceId: + type: string + serviceName: + type: string + serviceTags: + items: + type: string + type: array + status: + type: string + type: + type: string + required: + - id + - name + - node + - serviceId + - serviceName + - status + type: object + type: array + datacenter: type: string - namespace: + id: type: string - partition: + node: type: string - port: - type: integer - required: - - address - - name - - port + nodeMeta: + additionalProperties: + type: string + type: object + service: + properties: + address: + type: string + contentHash: + type: string + datacenter: + type: string + enableTagOverride: + type: boolean + id: + type: string + locality: + properties: + region: + type: string + zone: + type: string + type: object + meta: + additionalProperties: + type: string + type: object + name: + type: string + namespace: + type: string + partition: + type: string + port: + type: integer + socketPath: + type: string + taggedAddresses: + additionalProperties: + properties: + address: + type: string + port: + type: integer + required: + - address + - port + type: object + type: object + tags: + items: + type: string + type: array + weights: + properties: + passing: + type: integer + warning: + type: integer + required: + - passing + - warning + type: object + required: + - address + - name + - port + type: object + taggedAddresses: + additionalProperties: + type: string + type: object type: object type: object served: true diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index 17ce5fc455..f21ed78399 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -62,10 +62,10 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques NodeMeta: map[string]string{}, Datacenter: "", Service: &capi.AgentService{ - ID: fmt.Sprintf("%s-1234", registration.Spec.Name), - Service: registration.Spec.Name, - Address: registration.Spec.Address, - Port: registration.Spec.Port, + ID: fmt.Sprintf("%s-1234", registration.Spec.Service.Name), + Service: registration.Spec.Service.Name, + Address: registration.Spec.Service.Address, + Port: registration.Spec.Service.Port, }, SkipNodeUpdate: false, Partition: "", From 652bd61eca5364a2bcca12573b6be2ce7275014b Mon Sep 17 00:00:00 2001 From: jm96441n Date: Thu, 25 Apr 2024 14:05:24 -0400 Subject: [PATCH 03/17] health checks working --- .../consul/templates/crd-registrations.yaml | 169 +++++++++--------- .../api/v1alpha1/registration_types.go | 37 ++-- .../api/v1alpha1/zz_generated.deepcopy.go | 27 ++- .../consul.hashicorp.com_registrations.yaml | 169 +++++++++--------- .../configentries/registrations_controller.go | 99 ++++++++-- 5 files changed, 277 insertions(+), 224 deletions(-) diff --git a/charts/consul/templates/crd-registrations.yaml b/charts/consul/templates/crd-registrations.yaml index eb244d2a28..7d069f1222 100644 --- a/charts/consul/templates/crd-registrations.yaml +++ b/charts/consul/templates/crd-registrations.yaml @@ -42,90 +42,85 @@ spec: properties: address: type: string - checks: - items: - description: HealthCheck is used to represent a single check - properties: - definition: - description: HealthCheckDefinition is used to store the details - about a health check's execution. - properties: - body: - type: string - dereigsterCriticalServiceAfterDuration: - type: string - grpc: - type: string - grpcUseTLS: - type: boolean - header: - additionalProperties: - items: - type: string - type: array - type: object - http: - type: string - intervalDuration: - type: string - method: - type: string - osService: - type: string - tcp: - type: string - tcpUseTLS: - type: boolean - timeoutDuration: - type: string - tlsServerName: - type: string - tlsSkipVerify: - type: boolean - udp: - type: string - required: - - dereigsterCriticalServiceAfterDuration - - intervalDuration - - timeoutDuration - type: object - exposedPort: - type: integer - id: - type: string - name: - type: string - namespace: - type: string - node: - type: string - notes: - type: string - output: - type: string - partition: - type: string - serviceId: - type: string - serviceName: - type: string - serviceTags: - items: + check: + description: HealthCheck is used to represent a single check + properties: + checkId: + type: string + definition: + description: HealthCheckDefinition is used to store the details + about a health check's execution. + properties: + body: type: string - type: array - status: - type: string - type: - type: string - required: - - id - - name - - node - - serviceId - - serviceName - - status - type: object - type: array + deregisterCriticalServiceAfterDuration: + type: string + grpc: + type: string + grpcUseTLS: + type: boolean + header: + additionalProperties: + items: + type: string + type: array + type: object + http: + type: string + intervalDuration: + type: string + method: + type: string + osService: + type: string + tcp: + type: string + tcpUseTLS: + type: boolean + timeoutDuration: + type: string + tlsServerName: + type: string + tlsSkipVerify: + type: boolean + udp: + type: string + required: + - deregisterCriticalServiceAfterDuration + - intervalDuration + - timeoutDuration + type: object + exposedPort: + type: integer + name: + type: string + namespace: + type: string + node: + type: string + notes: + type: string + output: + type: string + partition: + type: string + serviceId: + type: string + serviceName: + type: string + status: + type: string + type: + type: string + required: + - checkId + - definition + - name + - node + - serviceId + - serviceName + - status + type: object datacenter: type: string id: @@ -136,14 +131,12 @@ spec: additionalProperties: type: string type: object + partition: + type: string service: properties: address: type: string - contentHash: - type: string - datacenter: - type: string enableTagOverride: type: boolean id: @@ -200,6 +193,8 @@ spec: - name - port type: object + skipNodeUpdate: + type: boolean taggedAddresses: additionalProperties: type: string diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go index 9d7a66ab29..103bba1def 100644 --- a/control-plane/api/v1alpha1/registration_types.go +++ b/control-plane/api/v1alpha1/registration_types.go @@ -38,7 +38,9 @@ type RegistrationSpec struct { NodeMeta map[string]string `json:"nodeMeta,omitempty"` Datacenter string `json:"datacenter,omitempty"` Service Service `json:"service,omitempty"` - HealthChecks []HealthCheck `json:"checks,omitempty"` + SkipNodeUpdate bool `json:"skipNodeUpdate,omitempty"` + Partition string `json:"partition,omitempty"` + HealthCheck *HealthCheck `json:"check,omitempty"` } // +k8s:deepcopy-gen=true @@ -54,9 +56,7 @@ type Service struct { TaggedAddresses map[string]ServiceAddress `json:"taggedAddresses,omitempty"` Weights Weights `json:"weights,omitempty"` EnableTagOverride bool `json:"enableTagOverride,omitempty"` - ContentHash string `json:"contentHash,omitempty"` - Datacenter string `json:"datacenter,omitempty"` - Locality Locality `json:"locality,omitempty"` + Locality *Locality `json:"locality,omitempty"` Namespace string `json:"namespace,omitempty"` Partition string `json:"partition,omitempty"` } @@ -86,20 +86,19 @@ type Locality struct { // HealthCheck is used to represent a single check type HealthCheck struct { - Node string `json:"node"` - CheckID string `json:"id"` - Name string `json:"name"` - Status string `json:"status"` - Notes string `json:"notes,omitempty"` - Output string `json:"output,omitempty"` - ServiceID string `json:"serviceId"` - ServiceName string `json:"serviceName"` - ServiceTags []string `json:"serviceTags,omitempty"` - Type string `json:"type,omitempty"` - Namespace string `json:"namespace,omitempty"` - Partition string `json:"partition,omitempty"` - ExposedPort int `json:"exposedPort,omitempty"` - Definition *HealthCheckDefinition `json:"definition,omitempty"` + Node string `json:"node"` + CheckID string `json:"checkId"` + Name string `json:"name"` + Status string `json:"status"` + Notes string `json:"notes,omitempty"` + Output string `json:"output,omitempty"` + ServiceID string `json:"serviceId"` + ServiceName string `json:"serviceName"` + Type string `json:"type,omitempty"` + ExposedPort int `json:"exposedPort,omitempty"` + Definition HealthCheckDefinition `json:"definition"` + Namespace string `json:"namespace,omitempty"` + Partition string `json:"partition,omitempty"` } // HealthCheckDefinition is used to store the details about @@ -119,7 +118,7 @@ type HealthCheckDefinition struct { GRPCUseTLS bool `json:"grpcUseTLS,omitempty"` IntervalDuration string `json:"intervalDuration"` TimeoutDuration string `json:"timeoutDuration"` - DeregisterCriticalServiceAfterDuration string `json:"dereigsterCriticalServiceAfterDuration"` + DeregisterCriticalServiceAfterDuration string `json:"deregisterCriticalServiceAfterDuration"` } // +kubebuilder:object:root=true diff --git a/control-plane/api/v1alpha1/zz_generated.deepcopy.go b/control-plane/api/v1alpha1/zz_generated.deepcopy.go index 49b4f3b651..6582832405 100644 --- a/control-plane/api/v1alpha1/zz_generated.deepcopy.go +++ b/control-plane/api/v1alpha1/zz_generated.deepcopy.go @@ -884,16 +884,7 @@ func (in *HashPolicy) DeepCopy() *HashPolicy { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HealthCheck) DeepCopyInto(out *HealthCheck) { *out = *in - if in.ServiceTags != nil { - in, out := &in.ServiceTags, &out.ServiceTags - *out = make([]string, len(*in)) - copy(*out, *in) - } - if in.Definition != nil { - in, out := &in.Definition, &out.Definition - *out = new(HealthCheckDefinition) - (*in).DeepCopyInto(*out) - } + in.Definition.DeepCopyInto(&out.Definition) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HealthCheck. @@ -2629,12 +2620,10 @@ func (in *RegistrationSpec) DeepCopyInto(out *RegistrationSpec) { } } in.Service.DeepCopyInto(&out.Service) - if in.HealthChecks != nil { - in, out := &in.HealthChecks, &out.HealthChecks - *out = make([]HealthCheck, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } + if in.HealthCheck != nil { + in, out := &in.HealthCheck, &out.HealthCheck + *out = new(HealthCheck) + (*in).DeepCopyInto(*out) } } @@ -3144,7 +3133,11 @@ func (in *Service) DeepCopyInto(out *Service) { } } out.Weights = in.Weights - out.Locality = in.Locality + if in.Locality != nil { + in, out := &in.Locality, &out.Locality + *out = new(Locality) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Service. 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 0489c4f2ad..59bc04a2c0 100644 --- a/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml +++ b/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml @@ -38,90 +38,85 @@ spec: properties: address: type: string - checks: - items: - description: HealthCheck is used to represent a single check - properties: - definition: - description: HealthCheckDefinition is used to store the details - about a health check's execution. - properties: - body: - type: string - dereigsterCriticalServiceAfterDuration: - type: string - grpc: - type: string - grpcUseTLS: - type: boolean - header: - additionalProperties: - items: - type: string - type: array - type: object - http: - type: string - intervalDuration: - type: string - method: - type: string - osService: - type: string - tcp: - type: string - tcpUseTLS: - type: boolean - timeoutDuration: - type: string - tlsServerName: - type: string - tlsSkipVerify: - type: boolean - udp: - type: string - required: - - dereigsterCriticalServiceAfterDuration - - intervalDuration - - timeoutDuration - type: object - exposedPort: - type: integer - id: - type: string - name: - type: string - namespace: - type: string - node: - type: string - notes: - type: string - output: - type: string - partition: - type: string - serviceId: - type: string - serviceName: - type: string - serviceTags: - items: + check: + description: HealthCheck is used to represent a single check + properties: + checkId: + type: string + definition: + description: HealthCheckDefinition is used to store the details + about a health check's execution. + properties: + body: type: string - type: array - status: - type: string - type: - type: string - required: - - id - - name - - node - - serviceId - - serviceName - - status - type: object - type: array + deregisterCriticalServiceAfterDuration: + type: string + grpc: + type: string + grpcUseTLS: + type: boolean + header: + additionalProperties: + items: + type: string + type: array + type: object + http: + type: string + intervalDuration: + type: string + method: + type: string + osService: + type: string + tcp: + type: string + tcpUseTLS: + type: boolean + timeoutDuration: + type: string + tlsServerName: + type: string + tlsSkipVerify: + type: boolean + udp: + type: string + required: + - deregisterCriticalServiceAfterDuration + - intervalDuration + - timeoutDuration + type: object + exposedPort: + type: integer + name: + type: string + namespace: + type: string + node: + type: string + notes: + type: string + output: + type: string + partition: + type: string + serviceId: + type: string + serviceName: + type: string + status: + type: string + type: + type: string + required: + - checkId + - definition + - name + - node + - serviceId + - serviceName + - status + type: object datacenter: type: string id: @@ -132,14 +127,12 @@ spec: additionalProperties: type: string type: object + partition: + type: string service: properties: address: type: string - contentHash: - type: string - datacenter: - type: string enableTagOverride: type: boolean id: @@ -196,6 +189,8 @@ spec: - name - port type: object + skipNodeUpdate: + type: boolean taggedAddresses: additionalProperties: type: string diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index f21ed78399..ae5835dd4d 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -6,6 +6,9 @@ package configentries import ( "context" "fmt" + "maps" + "slices" + "time" "github.com/go-logr/logr" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -17,7 +20,6 @@ import ( capi "github.com/hashicorp/consul/api" "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" - consulv1alpha1 "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" "github.com/hashicorp/consul-k8s/control-plane/consul" ) @@ -56,31 +58,100 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques } regReq := &capi.CatalogRegistration{ - Node: "node-virtual", - Address: "127.0.0.1", - TaggedAddresses: map[string]string{}, - NodeMeta: map[string]string{}, - Datacenter: "", + ID: registration.Spec.ID, + Node: registration.Spec.Node, + Address: registration.Spec.Address, + TaggedAddresses: maps.Clone(registration.Spec.TaggedAddresses), + NodeMeta: maps.Clone(registration.Spec.NodeMeta), + Datacenter: registration.Spec.Datacenter, Service: &capi.AgentService{ - ID: fmt.Sprintf("%s-1234", registration.Spec.Service.Name), - Service: registration.Spec.Service.Name, - Address: registration.Spec.Service.Address, - Port: registration.Spec.Service.Port, + ID: registration.Spec.Service.ID, + Service: registration.Spec.Service.Name, + Tags: slices.Clone(registration.Spec.Service.Tags), + Meta: maps.Clone(registration.Spec.Service.Meta), + Port: registration.Spec.Service.Port, + Address: registration.Spec.Service.Address, + SocketPath: registration.Spec.Service.SocketPath, + TaggedAddresses: copyTaggedAddresses(registration.Spec.Service.TaggedAddresses), + Weights: capi.AgentWeights(registration.Spec.Service.Weights), + EnableTagOverride: registration.Spec.Service.EnableTagOverride, + Namespace: registration.Spec.Service.Namespace, + Partition: registration.Spec.Service.Partition, + Locality: copyLocality(registration.Spec.Service.Locality), }, - SkipNodeUpdate: false, - Partition: "", - Locality: &capi.Locality{}, + Check: copyHealthCheck(registration.Spec.HealthCheck), + SkipNodeUpdate: registration.Spec.SkipNodeUpdate, + Partition: registration.Spec.Partition, } + fmt.Println(regReq) _, err = client.Catalog().Register(regReq, nil) if err != nil { log.Error(err, "error registering service", "svcName", regReq.Service.Service) return ctrl.Result{}, err } + log.Info("Successfully registered service", "svcName", regReq.Service.Service) return ctrl.Result{}, nil } +func copyTaggedAddresses(taggedAddresses map[string]v1alpha1.ServiceAddress) map[string]capi.ServiceAddress { + if taggedAddresses == nil { + return nil + } + result := make(map[string]capi.ServiceAddress, len(taggedAddresses)) + for k, v := range taggedAddresses { + result[k] = capi.ServiceAddress(v) + } + return result +} + +func copyLocality(locality *v1alpha1.Locality) *capi.Locality { + if locality == nil { + return nil + } + return &capi.Locality{ + Region: locality.Region, + Zone: locality.Zone, + } +} + +func copyHealthCheck(healthCheck *v1alpha1.HealthCheck) *capi.AgentCheck { + if healthCheck == nil { + return nil + } + + // TODO: handle error + intervalDuration, _ := time.ParseDuration(healthCheck.Definition.IntervalDuration) + timeoutDuration, _ := time.ParseDuration(healthCheck.Definition.TimeoutDuration) + deregisterAfter, _ := time.ParseDuration(healthCheck.Definition.DeregisterCriticalServiceAfterDuration) + + return &capi.AgentCheck{ + CheckID: healthCheck.CheckID, + Name: healthCheck.Name, + Type: healthCheck.Type, + Status: healthCheck.Status, + ServiceID: healthCheck.ServiceID, + Output: healthCheck.Output, + Namespace: healthCheck.Namespace, + Definition: capi.HealthCheckDefinition{ + HTTP: healthCheck.Definition.HTTP, + TCP: healthCheck.Definition.TCP, + GRPC: healthCheck.Definition.GRPC, + GRPCUseTLS: healthCheck.Definition.GRPCUseTLS, + Method: healthCheck.Definition.Method, + Header: healthCheck.Definition.Header, + Body: healthCheck.Definition.Body, + TLSServerName: healthCheck.Definition.TLSServerName, + TLSSkipVerify: healthCheck.Definition.TLSSkipVerify, + OSService: healthCheck.Definition.OSService, + IntervalDuration: intervalDuration, + TimeoutDuration: timeoutDuration, + DeregisterCriticalServiceAfterDuration: deregisterAfter, + }, + } +} + func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger { return r.Log.WithValues("request", name) } @@ -90,5 +161,5 @@ func (r *RegistrationsController) UpdateStatus(ctx context.Context, obj client.O } func (r *RegistrationsController) SetupWithManager(mgr ctrl.Manager) error { - return setupWithManager(mgr, &consulv1alpha1.Registration{}, r) + return setupWithManager(mgr, &v1alpha1.Registration{}, r) } From 559e8b8785567b525fd64652228df67337fe2de5 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Thu, 25 Apr 2024 15:23:20 -0400 Subject: [PATCH 04/17] handle finalizers to clean up --- .../configentries/registrations_controller.go | 61 +++++++++++++++++-- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index ae5835dd4d..1a3b46207c 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -5,7 +5,6 @@ package configentries import ( "context" - "fmt" "maps" "slices" "time" @@ -25,6 +24,8 @@ import ( var _ Controller = (*RegistrationsController)(nil) +const registrationFinalizer = "registration.finalizers.consul.hashicorp.com" + // RegistrationsController is the controller for Registrations resources. type RegistrationsController struct { client.Client @@ -48,6 +49,7 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques if !k8serrors.IsNotFound(err) { log.Error(err, "unable to get registration") } + log.Error(err, "unable to get registration") return ctrl.Result{}, client.IgnoreNotFound(err) } @@ -57,6 +59,33 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, err } + log.Info("Registration", "registration", registration) + // deletion request + if !registration.ObjectMeta.DeletionTimestamp.IsZero() { + log.Info("Deregistering service") + err = r.deregisterService(ctx, log, client, registration) + if err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil + } + + log.Info("Registering service") + err = r.registerService(ctx, log, client, registration) + if err != nil { + return ctrl.Result{}, err + } + + return ctrl.Result{}, nil +} + +func (r *RegistrationsController) registerService(ctx context.Context, log logr.Logger, client *capi.Client, registration v1alpha1.Registration) error { + patch := r.AddFinalizersPatch(®istration, registrationFinalizer) + err := r.Patch(ctx, ®istration, patch) + if err != nil { + return err + } + regReq := &capi.CatalogRegistration{ ID: registration.Spec.ID, Node: registration.Spec.Node, @@ -84,15 +113,39 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques Partition: registration.Spec.Partition, } - fmt.Println(regReq) _, err = client.Catalog().Register(regReq, nil) if err != nil { log.Error(err, "error registering service", "svcName", regReq.Service.Service) - return ctrl.Result{}, err + return err } log.Info("Successfully registered service", "svcName", regReq.Service.Service) - return ctrl.Result{}, nil + return nil +} + +func (r *RegistrationsController) deregisterService(ctx context.Context, log logr.Logger, client *capi.Client, registration v1alpha1.Registration) error { + patch := r.RemoveFinalizersPatch(®istration, registrationFinalizer) + if err := r.Patch(ctx, ®istration, patch); err != nil { + return err + } + + deregReq := &capi.CatalogDeregistration{ + Node: registration.Spec.Node, + Address: registration.Spec.Address, + Datacenter: registration.Spec.Datacenter, + ServiceID: registration.Spec.Service.ID, + CheckID: registration.Spec.HealthCheck.CheckID, + Namespace: registration.Spec.Service.Namespace, + Partition: registration.Spec.Service.Partition, + } + _, err := client.Catalog().Deregister(deregReq, nil) + if err != nil { + log.Error(err, "error deregistering service", "svcID", deregReq.ServiceID) + return err + } + + log.Info("Successfully deregistered service", "svcID", deregReq.ServiceID) + return nil } func copyTaggedAddresses(taggedAddresses map[string]v1alpha1.ServiceAddress) map[string]capi.ServiceAddress { From 1a1e92d1b14fbde2be85b8a10486ba94665d41c0 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Mon, 29 Apr 2024 15:26:22 -0400 Subject: [PATCH 05/17] Add status to registration CRD --- .../consul/templates/crd-registrations.yaml | 41 ++++++++++++++ .../api/v1alpha1/registration_types.go | 28 ++++++++++ .../api/v1alpha1/zz_generated.deepcopy.go | 27 ++++++++++ .../consul.hashicorp.com_registrations.yaml | 41 ++++++++++++++ .../configentries/registrations_controller.go | 54 ++++++++++++++----- 5 files changed, 177 insertions(+), 14 deletions(-) diff --git a/charts/consul/templates/crd-registrations.yaml b/charts/consul/templates/crd-registrations.yaml index 7d069f1222..9faf0fb21f 100644 --- a/charts/consul/templates/crd-registrations.yaml +++ b/charts/consul/templates/crd-registrations.yaml @@ -200,7 +200,48 @@ spec: type: string type: object type: object + status: + description: RegistrationStatus defines the observed state of Registration. + properties: + conditions: + description: Conditions indicate the latest available observations + of a resource's current state. + items: + description: 'Conditions define a readiness condition for a Consul + resource. See: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties' + properties: + lastTransitionTime: + description: LastTransitionTime is the last time the condition + transitioned from one status to another. + format: date-time + type: string + message: + description: A human readable message indicating details about + the transition. + type: string + reason: + description: The reason for the condition's last transition. + type: string + status: + description: Status of the condition, one of True, False, Unknown. + type: string + type: + description: Type of condition. + type: string + required: + - status + - type + type: object + type: array + lastSyncedTime: + description: LastSyncedTime is the last time the resource successfully + synced with Consul. + format: date-time + type: string + type: object type: object served: true storage: true + subresources: + status: {} {{- end }} diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go index 103bba1def..a53145f815 100644 --- a/control-plane/api/v1alpha1/registration_types.go +++ b/control-plane/api/v1alpha1/registration_types.go @@ -4,6 +4,7 @@ package v1alpha1 import ( + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -14,6 +15,7 @@ func init() { // +genclient // +kubebuilder:object:root=true // +kubebuilder:resource:scope=Cluster +// +kubebuilder:subresource:status // Registration defines the. type Registration struct { @@ -25,6 +27,20 @@ type Registration struct { // Spec defines the desired state of Registration. Spec RegistrationSpec `json:"spec,omitempty"` + + Status RegistrationStatus `json:"status,omitempty"` +} + +// RegistrationStatus defines the observed state of Registration. +type RegistrationStatus struct { + // Conditions indicate the latest available observations of a resource's current state. + // +optional + // +patchMergeKey=type + // +patchStrategy=merge + Conditions Conditions `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` + // LastSyncedTime is the last time the resource successfully synced with Consul. + // +optional + LastSyncedTime *metav1.Time `json:"lastSyncedTime,omitempty" description:"last time the condition transitioned from one status to another"` } // +k8s:deepcopy-gen=true @@ -131,3 +147,15 @@ type RegistrationList struct { // Items is the list of Configs. Items []Registration `json:"items"` } + +func (r *Registration) SetSyncedCondition(status corev1.ConditionStatus, reason string, message string) { + r.Status.Conditions = Conditions{ + { + Type: ConditionSynced, + Status: status, + LastTransitionTime: metav1.Now(), + Reason: reason, + Message: message, + }, + } +} diff --git a/control-plane/api/v1alpha1/zz_generated.deepcopy.go b/control-plane/api/v1alpha1/zz_generated.deepcopy.go index 6582832405..8ca7a4f776 100644 --- a/control-plane/api/v1alpha1/zz_generated.deepcopy.go +++ b/control-plane/api/v1alpha1/zz_generated.deepcopy.go @@ -2550,6 +2550,7 @@ func (in *Registration) DeepCopyInto(out *Registration) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) + in.Status.DeepCopyInto(&out.Status) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Registration. @@ -2637,6 +2638,32 @@ func (in *RegistrationSpec) DeepCopy() *RegistrationSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RegistrationStatus) DeepCopyInto(out *RegistrationStatus) { + *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make(Conditions, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } + if in.LastSyncedTime != nil { + in, out := &in.LastSyncedTime, &out.LastSyncedTime + *out = (*in).DeepCopy() + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RegistrationStatus. +func (in *RegistrationStatus) DeepCopy() *RegistrationStatus { + if in == nil { + return nil + } + out := new(RegistrationStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RemoteJWKS) DeepCopyInto(out *RemoteJWKS) { *out = *in 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 59bc04a2c0..58fada9d15 100644 --- a/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml +++ b/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml @@ -196,6 +196,47 @@ spec: type: string type: object type: object + status: + description: RegistrationStatus defines the observed state of Registration. + properties: + conditions: + description: Conditions indicate the latest available observations + of a resource's current state. + items: + description: 'Conditions define a readiness condition for a Consul + resource. See: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties' + properties: + lastTransitionTime: + description: LastTransitionTime is the last time the condition + transitioned from one status to another. + format: date-time + type: string + message: + description: A human readable message indicating details about + the transition. + type: string + reason: + description: The reason for the condition's last transition. + type: string + status: + description: Status of the condition, one of True, False, Unknown. + type: string + type: + description: Type of condition. + type: string + required: + - status + - type + type: object + type: array + lastSyncedTime: + description: LastSyncedTime is the last time the resource successfully + synced with Consul. + format: date-time + type: string + type: object type: object served: true storage: true + subresources: + status: {} diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index 1a3b46207c..cb1adc96c4 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -10,7 +10,9 @@ import ( "time" "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" @@ -22,8 +24,6 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/consul" ) -var _ Controller = (*RegistrationsController)(nil) - const registrationFinalizer = "registration.finalizers.consul.hashicorp.com" // RegistrationsController is the controller for Registrations resources. @@ -43,13 +43,12 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques log := r.Log.V(1).WithValues("registration", req.NamespacedName) log.Info("Reconciling Registaration") - var registration v1alpha1.Registration + registration := &v1alpha1.Registration{} // get the gateway - if err := r.Client.Get(ctx, req.NamespacedName, ®istration); err != nil { + if err := r.Client.Get(ctx, req.NamespacedName, registration); err != nil { if !k8serrors.IsNotFound(err) { log.Error(err, "unable to get registration") } - log.Error(err, "unable to get registration") return ctrl.Result{}, client.IgnoreNotFound(err) } @@ -67,21 +66,27 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques if err != nil { return ctrl.Result{}, err } + r.updateStatusError(ctx, registration, "ConsulErrorDeregistration", err) return ctrl.Result{}, nil } log.Info("Registering service") err = r.registerService(ctx, log, client, registration) if err != nil { + r.updateStatusError(ctx, registration, "ConsulErrorRegistration", err) return ctrl.Result{}, err } - return ctrl.Result{}, nil + err = r.updateStatus(ctx, req.NamespacedName) + if err != nil { + log.Error(err, "failed to update status") + } + return ctrl.Result{}, err } -func (r *RegistrationsController) registerService(ctx context.Context, log logr.Logger, client *capi.Client, registration v1alpha1.Registration) error { - patch := r.AddFinalizersPatch(®istration, registrationFinalizer) - err := r.Patch(ctx, ®istration, patch) +func (r *RegistrationsController) registerService(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + patch := r.AddFinalizersPatch(registration, registrationFinalizer) + err := r.Patch(ctx, registration, patch) if err != nil { return err } @@ -123,9 +128,9 @@ func (r *RegistrationsController) registerService(ctx context.Context, log logr. return nil } -func (r *RegistrationsController) deregisterService(ctx context.Context, log logr.Logger, client *capi.Client, registration v1alpha1.Registration) error { - patch := r.RemoveFinalizersPatch(®istration, registrationFinalizer) - if err := r.Patch(ctx, ®istration, patch); err != nil { +func (r *RegistrationsController) deregisterService(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { + patch := r.RemoveFinalizersPatch(registration, registrationFinalizer) + if err := r.Patch(ctx, registration, patch); err != nil { return err } @@ -209,8 +214,29 @@ func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger return r.Log.WithValues("request", name) } -func (r *RegistrationsController) UpdateStatus(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { - return r.Status().Update(ctx, obj, opts...) +func (r *RegistrationsController) updateStatusError(ctx context.Context, registration *v1alpha1.Registration, reason string, reconcileErr error) { + registration.SetSyncedCondition(corev1.ConditionFalse, reason, reconcileErr.Error()) + err := r.Status().Update(ctx, registration) + if err != nil { + r.Log.Error(err, "failed to update Registration status", "name", registration.Name, "namespace", registration.Namespace) + } +} + +func (r *RegistrationsController) updateStatus(ctx context.Context, req types.NamespacedName) error { + registration := &v1alpha1.Registration{} + err := r.Get(ctx, req, registration) + if err != nil { + return err + } + + registration.Status.LastSyncedTime = &metav1.Time{Time: time.Now()} + registration.SetSyncedCondition(corev1.ConditionTrue, "", "") + err = r.Status().Update(ctx, registration) + if err != nil { + r.Log.Error(err, "failed to update Registration status", "name", registration.Name, "namespace", registration.Namespace) + return err + } + return nil } func (r *RegistrationsController) SetupWithManager(mgr ctrl.Manager) error { From 589f54fa4c73c480c6e63440d5a3dfd79176b255 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 10:33:26 -0400 Subject: [PATCH 06/17] Added initial unit test for reconcile --- .../registrations_controller_test.go | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 control-plane/controllers/configentries/registrations_controller_test.go diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go new file mode 100644 index 0000000000..b2e679ca3f --- /dev/null +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -0,0 +1,102 @@ +package configentries_test + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "testing" + + logrtest "github.com/go-logr/logr/testing" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + capi "github.com/hashicorp/consul/api" + + "github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1" + "github.com/hashicorp/consul-k8s/control-plane/consul" + "github.com/hashicorp/consul-k8s/control-plane/controllers/configentries" + "github.com/hashicorp/consul-k8s/control-plane/helper/test" +) + +func TestReconcile(tt *testing.T) { + cases := map[string]struct { + regName string + regID string + consulShouldError bool + }{ + "success": { + regName: "test-reg", + regID: "test-reg-id", + consulShouldError: false, + }, + } + + for name, tc := range cases { + tt.Run(name, func(t *testing.T) { + s := runtime.NewScheme() + s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}) + ctx := context.Background() + + consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if tc.consulShouldError { + w.WriteHeader(500) + return + } + w.WriteHeader(200) + })) + defer consulServer.Close() + + parsedURL, err := url.Parse(consulServer.URL) + require.NoError(t, err) + + port, err := strconv.Atoi(parsedURL.Port()) + require.NoError(t, err) + + testClient := &test.TestServerClient{ + Cfg: &consul.Config{APIClientConfig: &capi.Config{}, HTTPPort: port}, + Watcher: test.MockConnMgrForIPAndPort(t, parsedURL.Host, port, false), + } + + reg := &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: tc.regName, + Namespace: name, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: tc.regID, + Name: tc.regName, + Port: 8080, + Address: "127.0.0.1", + }, + }, + } + + fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(reg).Build() + + controller := &configentries.RegistrationsController{ + Client: fakeClient, + Log: logrtest.NewTestLogger(t), + Scheme: s, + ConsulClientConfig: testClient.Cfg, + ConsulServerConnMgr: testClient.Watcher, + } + + _, err = controller.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) + }) + } +} From 60a88c1f17c809d359f0dda259eb55c44a5f1ee8 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 10:58:33 -0400 Subject: [PATCH 07/17] success paths for registration and deregistration --- .../api/v1alpha1/registration_types.go | 109 ++++++++++++++++++ .../configentries/registrations_controller.go | 106 ++--------------- .../registrations_controller_test.go | 93 +++++++++------ 3 files changed, 176 insertions(+), 132 deletions(-) diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go index a53145f815..c240a69f4b 100644 --- a/control-plane/api/v1alpha1/registration_types.go +++ b/control-plane/api/v1alpha1/registration_types.go @@ -4,6 +4,12 @@ package v1alpha1 import ( + "maps" + "slices" + "time" + + capi "github.com/hashicorp/consul/api" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -148,6 +154,109 @@ type RegistrationList struct { Items []Registration `json:"items"` } +func (r *Registration) ToCatalogRegistration() *capi.CatalogRegistration { + return &capi.CatalogRegistration{ + ID: r.Spec.ID, + Node: r.Spec.Node, + Address: r.Spec.Address, + TaggedAddresses: maps.Clone(r.Spec.TaggedAddresses), + NodeMeta: maps.Clone(r.Spec.NodeMeta), + Datacenter: r.Spec.Datacenter, + Service: &capi.AgentService{ + ID: r.Spec.Service.ID, + Service: r.Spec.Service.Name, + Tags: slices.Clone(r.Spec.Service.Tags), + Meta: maps.Clone(r.Spec.Service.Meta), + Port: r.Spec.Service.Port, + Address: r.Spec.Service.Address, + SocketPath: r.Spec.Service.SocketPath, + TaggedAddresses: copyTaggedAddresses(r.Spec.Service.TaggedAddresses), + Weights: capi.AgentWeights(r.Spec.Service.Weights), + EnableTagOverride: r.Spec.Service.EnableTagOverride, + Namespace: r.Spec.Service.Namespace, + Partition: r.Spec.Service.Partition, + Locality: copyLocality(r.Spec.Service.Locality), + }, + Check: copyHealthCheck(r.Spec.HealthCheck), + SkipNodeUpdate: r.Spec.SkipNodeUpdate, + Partition: r.Spec.Partition, + } +} + +func copyTaggedAddresses(taggedAddresses map[string]ServiceAddress) map[string]capi.ServiceAddress { + if taggedAddresses == nil { + return nil + } + result := make(map[string]capi.ServiceAddress, len(taggedAddresses)) + for k, v := range taggedAddresses { + result[k] = capi.ServiceAddress(v) + } + return result +} + +func copyLocality(locality *Locality) *capi.Locality { + if locality == nil { + return nil + } + return &capi.Locality{ + Region: locality.Region, + Zone: locality.Zone, + } +} + +func copyHealthCheck(healthCheck *HealthCheck) *capi.AgentCheck { + if healthCheck == nil { + return nil + } + + // TODO: handle error + intervalDuration, _ := time.ParseDuration(healthCheck.Definition.IntervalDuration) + timeoutDuration, _ := time.ParseDuration(healthCheck.Definition.TimeoutDuration) + deregisterAfter, _ := time.ParseDuration(healthCheck.Definition.DeregisterCriticalServiceAfterDuration) + + return &capi.AgentCheck{ + CheckID: healthCheck.CheckID, + Name: healthCheck.Name, + Type: healthCheck.Type, + Status: healthCheck.Status, + ServiceID: healthCheck.ServiceID, + Output: healthCheck.Output, + Namespace: healthCheck.Namespace, + Definition: capi.HealthCheckDefinition{ + HTTP: healthCheck.Definition.HTTP, + TCP: healthCheck.Definition.TCP, + GRPC: healthCheck.Definition.GRPC, + GRPCUseTLS: healthCheck.Definition.GRPCUseTLS, + Method: healthCheck.Definition.Method, + Header: healthCheck.Definition.Header, + Body: healthCheck.Definition.Body, + TLSServerName: healthCheck.Definition.TLSServerName, + TLSSkipVerify: healthCheck.Definition.TLSSkipVerify, + OSService: healthCheck.Definition.OSService, + IntervalDuration: intervalDuration, + TimeoutDuration: timeoutDuration, + DeregisterCriticalServiceAfterDuration: deregisterAfter, + }, + } +} + +func (r *Registration) ToCatalogDeregistration() *capi.CatalogDeregistration { + checkID := "" + if r.Spec.HealthCheck != nil { + checkID = r.Spec.HealthCheck.CheckID + } + + return &capi.CatalogDeregistration{ + Node: r.Spec.Node, + Address: r.Spec.Address, + Datacenter: r.Spec.Datacenter, + ServiceID: r.Spec.Service.ID, + CheckID: checkID, + Namespace: r.Spec.Service.Namespace, + Partition: r.Spec.Service.Partition, + } +} + func (r *Registration) SetSyncedCondition(status corev1.ConditionStatus, reason string, message string) { r.Status.Conditions = Conditions{ { diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index cb1adc96c4..056b8b3298 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -5,8 +5,6 @@ package configentries import ( "context" - "maps" - "slices" "time" "github.com/go-logr/logr" @@ -64,9 +62,10 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques log.Info("Deregistering service") err = r.deregisterService(ctx, log, client, registration) if err != nil { + r.updateStatusError(ctx, registration, "ConsulErrorDeregistration", err) return ctrl.Result{}, err } - r.updateStatusError(ctx, registration, "ConsulErrorDeregistration", err) + r.updateStatus(ctx, req.NamespacedName) return ctrl.Result{}, nil } @@ -91,33 +90,7 @@ func (r *RegistrationsController) registerService(ctx context.Context, log logr. return err } - regReq := &capi.CatalogRegistration{ - ID: registration.Spec.ID, - Node: registration.Spec.Node, - Address: registration.Spec.Address, - TaggedAddresses: maps.Clone(registration.Spec.TaggedAddresses), - NodeMeta: maps.Clone(registration.Spec.NodeMeta), - Datacenter: registration.Spec.Datacenter, - Service: &capi.AgentService{ - ID: registration.Spec.Service.ID, - Service: registration.Spec.Service.Name, - Tags: slices.Clone(registration.Spec.Service.Tags), - Meta: maps.Clone(registration.Spec.Service.Meta), - Port: registration.Spec.Service.Port, - Address: registration.Spec.Service.Address, - SocketPath: registration.Spec.Service.SocketPath, - TaggedAddresses: copyTaggedAddresses(registration.Spec.Service.TaggedAddresses), - Weights: capi.AgentWeights(registration.Spec.Service.Weights), - EnableTagOverride: registration.Spec.Service.EnableTagOverride, - Namespace: registration.Spec.Service.Namespace, - Partition: registration.Spec.Service.Partition, - Locality: copyLocality(registration.Spec.Service.Locality), - }, - Check: copyHealthCheck(registration.Spec.HealthCheck), - SkipNodeUpdate: registration.Spec.SkipNodeUpdate, - Partition: registration.Spec.Partition, - } - + regReq := registration.ToCatalogRegistration() _, err = client.Catalog().Register(regReq, nil) if err != nil { log.Error(err, "error registering service", "svcName", regReq.Service.Service) @@ -134,82 +107,17 @@ func (r *RegistrationsController) deregisterService(ctx context.Context, log log return err } - deregReq := &capi.CatalogDeregistration{ - Node: registration.Spec.Node, - Address: registration.Spec.Address, - Datacenter: registration.Spec.Datacenter, - ServiceID: registration.Spec.Service.ID, - CheckID: registration.Spec.HealthCheck.CheckID, - Namespace: registration.Spec.Service.Namespace, - Partition: registration.Spec.Service.Partition, - } - _, err := client.Catalog().Deregister(deregReq, nil) + deRegReq := registration.ToCatalogDeregistration() + _, err := client.Catalog().Deregister(deRegReq, nil) if err != nil { - log.Error(err, "error deregistering service", "svcID", deregReq.ServiceID) + log.Error(err, "error deregistering service", "svcID", deRegReq.ServiceID) return err } - log.Info("Successfully deregistered service", "svcID", deregReq.ServiceID) + log.Info("Successfully deregistered service", "svcID", deRegReq.ServiceID) return nil } -func copyTaggedAddresses(taggedAddresses map[string]v1alpha1.ServiceAddress) map[string]capi.ServiceAddress { - if taggedAddresses == nil { - return nil - } - result := make(map[string]capi.ServiceAddress, len(taggedAddresses)) - for k, v := range taggedAddresses { - result[k] = capi.ServiceAddress(v) - } - return result -} - -func copyLocality(locality *v1alpha1.Locality) *capi.Locality { - if locality == nil { - return nil - } - return &capi.Locality{ - Region: locality.Region, - Zone: locality.Zone, - } -} - -func copyHealthCheck(healthCheck *v1alpha1.HealthCheck) *capi.AgentCheck { - if healthCheck == nil { - return nil - } - - // TODO: handle error - intervalDuration, _ := time.ParseDuration(healthCheck.Definition.IntervalDuration) - timeoutDuration, _ := time.ParseDuration(healthCheck.Definition.TimeoutDuration) - deregisterAfter, _ := time.ParseDuration(healthCheck.Definition.DeregisterCriticalServiceAfterDuration) - - return &capi.AgentCheck{ - CheckID: healthCheck.CheckID, - Name: healthCheck.Name, - Type: healthCheck.Type, - Status: healthCheck.Status, - ServiceID: healthCheck.ServiceID, - Output: healthCheck.Output, - Namespace: healthCheck.Namespace, - Definition: capi.HealthCheckDefinition{ - HTTP: healthCheck.Definition.HTTP, - TCP: healthCheck.Definition.TCP, - GRPC: healthCheck.Definition.GRPC, - GRPCUseTLS: healthCheck.Definition.GRPCUseTLS, - Method: healthCheck.Definition.Method, - Header: healthCheck.Definition.Header, - Body: healthCheck.Definition.Body, - TLSServerName: healthCheck.Definition.TLSServerName, - TLSSkipVerify: healthCheck.Definition.TLSSkipVerify, - OSService: healthCheck.Definition.OSService, - IntervalDuration: intervalDuration, - TimeoutDuration: timeoutDuration, - DeregisterCriticalServiceAfterDuration: deregisterAfter, - }, - } -} - func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger { return r.Log.WithValues("request", name) } diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index b2e679ca3f..99f5c01382 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -6,12 +6,14 @@ import ( "net/http/httptest" "net/url" "strconv" + "strings" "testing" logrtest "github.com/go-logr/logr/testing" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -24,14 +26,58 @@ import ( ) func TestReconcile(tt *testing.T) { + deletionTime := metav1.Now() cases := map[string]struct { - regName string - regID string + registration *v1alpha1.Registration consulShouldError bool }{ - "success": { - regName: "test-reg", - regID: "test-reg-id", + "success on registration": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + consulShouldError: false, + }, + "success on deregistration": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + DeletionTimestamp: &deletionTime, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, consulShouldError: false, }, } @@ -49,43 +95,20 @@ func TestReconcile(tt *testing.T) { } w.WriteHeader(200) })) - defer consulServer.Close() parsedURL, err := url.Parse(consulServer.URL) require.NoError(t, err) + host := strings.Split(parsedURL.Host, ":")[0] port, err := strconv.Atoi(parsedURL.Port()) require.NoError(t, err) testClient := &test.TestServerClient{ - Cfg: &consul.Config{APIClientConfig: &capi.Config{}, HTTPPort: port}, - Watcher: test.MockConnMgrForIPAndPort(t, parsedURL.Host, port, false), - } - - reg := &v1alpha1.Registration{ - TypeMeta: metav1.TypeMeta{ - Kind: "Registration", - APIVersion: "consul.hashicorp.com/v1alpha1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: tc.regName, - Namespace: name, - }, - Spec: v1alpha1.RegistrationSpec{ - ID: "node-id", - Node: "virtual-node", - Address: "127.0.0.1", - Datacenter: "dc1", - Service: v1alpha1.Service{ - ID: tc.regID, - Name: tc.regName, - Port: 8080, - Address: "127.0.0.1", - }, - }, + Cfg: &consul.Config{APIClientConfig: &capi.Config{Address: host}, HTTPPort: port}, + Watcher: test.MockConnMgrForIPAndPort(t, host, port, false), } - fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(reg).Build() + fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(tc.registration).Build() controller := &configentries.RegistrationsController{ Client: fakeClient, @@ -95,8 +118,12 @@ func TestReconcile(tt *testing.T) { ConsulServerConnMgr: testClient.Watcher, } - _, err = controller.Reconcile(ctx, ctrl.Request{}) + _, err = controller.Reconcile(ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{Name: tc.registration.Name, Namespace: tc.registration.Namespace}, + }) require.NoError(t, err) + + consulServer.Close() }) } } From cf1d304034a1c5d4559015fe656c3fb135c62b71 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 13:28:13 -0400 Subject: [PATCH 08/17] added failure tests, moved finalizer removal logic so it occurs after service is successfully deregistered --- .../configentries/registrations_controller.go | 13 +- .../registrations_controller_test.go | 159 ++++++++++++++++-- 2 files changed, 154 insertions(+), 18 deletions(-) diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index 056b8b3298..39c0f6e147 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -22,7 +22,7 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/consul" ) -const registrationFinalizer = "registration.finalizers.consul.hashicorp.com" +const RegistrationFinalizer = "registration.finalizers.consul.hashicorp.com" // RegistrationsController is the controller for Registrations resources. type RegistrationsController struct { @@ -84,7 +84,7 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques } func (r *RegistrationsController) registerService(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { - patch := r.AddFinalizersPatch(registration, registrationFinalizer) + patch := r.AddFinalizersPatch(registration, RegistrationFinalizer) err := r.Patch(ctx, registration, patch) if err != nil { return err @@ -102,11 +102,6 @@ func (r *RegistrationsController) registerService(ctx context.Context, log logr. } func (r *RegistrationsController) deregisterService(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { - patch := r.RemoveFinalizersPatch(registration, registrationFinalizer) - if err := r.Patch(ctx, registration, patch); err != nil { - return err - } - deRegReq := registration.ToCatalogDeregistration() _, err := client.Catalog().Deregister(deRegReq, nil) if err != nil { @@ -114,6 +109,10 @@ func (r *RegistrationsController) deregisterService(ctx context.Context, log log return err } + patch := r.RemoveFinalizersPatch(registration, RegistrationFinalizer) + if err := r.Patch(ctx, registration, patch); err != nil { + return err + } log.Info("Successfully deregistered service", "svcID", deRegReq.ServiceID) return nil } diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index 99f5c01382..02357ceeed 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -10,7 +10,10 @@ import ( "testing" logrtest "github.com/go-logr/logr/testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -25,11 +28,11 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/helper/test" ) -func TestReconcile(tt *testing.T) { +func TestReconcile_Success(tt *testing.T) { deletionTime := metav1.Now() cases := map[string]struct { - registration *v1alpha1.Registration - consulShouldError bool + registration *v1alpha1.Registration + expectedConditions []v1alpha1.Condition }{ "success on registration": { registration: &v1alpha1.Registration{ @@ -38,7 +41,8 @@ func TestReconcile(tt *testing.T) { APIVersion: "consul.hashicorp.com/v1alpha1", }, ObjectMeta: metav1.ObjectMeta{ - Name: "test-registration", + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, }, Spec: v1alpha1.RegistrationSpec{ ID: "node-id", @@ -53,7 +57,12 @@ func TestReconcile(tt *testing.T) { }, }, }, - consulShouldError: false, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionTrue, + Reason: "", + Message: "", + }}, }, "success on deregistration": { registration: &v1alpha1.Registration{ @@ -63,6 +72,7 @@ func TestReconcile(tt *testing.T) { }, ObjectMeta: metav1.ObjectMeta{ Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, DeletionTimestamp: &deletionTime, }, Spec: v1alpha1.RegistrationSpec{ @@ -78,7 +88,7 @@ func TestReconcile(tt *testing.T) { }, }, }, - consulShouldError: false, + expectedConditions: []v1alpha1.Condition{}, }, } @@ -89,12 +99,9 @@ func TestReconcile(tt *testing.T) { ctx := context.Background() consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if tc.consulShouldError { - w.WriteHeader(500) - return - } w.WriteHeader(200) })) + defer consulServer.Close() parsedURL, err := url.Parse(consulServer.URL) require.NoError(t, err) @@ -123,7 +130,137 @@ func TestReconcile(tt *testing.T) { }) require.NoError(t, err) - consulServer.Close() + fetchedReg := &v1alpha1.Registration{TypeMeta: metav1.TypeMeta{APIVersion: "consul.hashicorp.com/v1alpha1", Kind: "Registration"}} + fakeClient.Get(ctx, types.NamespacedName{Name: tc.registration.Name}, fetchedReg) + + require.Len(t, fetchedReg.Status.Conditions, len(tc.expectedConditions)) + + for i, c := range fetchedReg.Status.Conditions { + if diff := cmp.Diff(c, tc.expectedConditions[i], cmpopts.IgnoreFields(v1alpha1.Condition{}, "LastTransitionTime", "Message")); diff != "" { + t.Errorf("unexpected condition diff: %s", diff) + } + } + }) + } +} + +func TestReconcile_Failure(tt *testing.T) { + deletionTime := metav1.Now() + cases := map[string]struct { + registration *v1alpha1.Registration + expectedConditions []v1alpha1.Condition + }{ + "failure on registration": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionFalse, + Reason: "ConsulErrorRegistration", + Message: "", + }}, + }, + "failure on deregistration": { + registration: &v1alpha1.Registration{ + TypeMeta: metav1.TypeMeta{ + Kind: "Registration", + APIVersion: "consul.hashicorp.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-registration", + Finalizers: []string{configentries.RegistrationFinalizer}, + DeletionTimestamp: &deletionTime, + }, + Spec: v1alpha1.RegistrationSpec{ + ID: "node-id", + Node: "virtual-node", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: v1alpha1.Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + expectedConditions: []v1alpha1.Condition{{ + Type: "Synced", + Status: v1.ConditionFalse, + Reason: "ConsulErrorDeregistration", + Message: "", + }}, + }, + } + + for name, tc := range cases { + tt.Run(name, func(t *testing.T) { + s := runtime.NewScheme() + s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}) + ctx := context.Background() + + consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(500) + })) + defer consulServer.Close() + + parsedURL, err := url.Parse(consulServer.URL) + require.NoError(t, err) + host := strings.Split(parsedURL.Host, ":")[0] + + port, err := strconv.Atoi(parsedURL.Port()) + require.NoError(t, err) + + testClient := &test.TestServerClient{ + Cfg: &consul.Config{APIClientConfig: &capi.Config{Address: host}, HTTPPort: port}, + Watcher: test.MockConnMgrForIPAndPort(t, host, port, false), + } + + fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(tc.registration).Build() + + controller := &configentries.RegistrationsController{ + Client: fakeClient, + Log: logrtest.NewTestLogger(t), + Scheme: s, + ConsulClientConfig: testClient.Cfg, + ConsulServerConnMgr: testClient.Watcher, + } + + _, err = controller.Reconcile(ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{Name: tc.registration.Name, Namespace: tc.registration.Namespace}, + }) + require.Error(t, err) + + fetchedReg := &v1alpha1.Registration{TypeMeta: metav1.TypeMeta{APIVersion: "consul.hashicorp.com/v1alpha1", Kind: "Registration"}} + fakeClient.Get(ctx, types.NamespacedName{Name: tc.registration.Name}, fetchedReg) + + require.Len(t, fetchedReg.Status.Conditions, len(tc.expectedConditions)) + + for i, c := range fetchedReg.Status.Conditions { + if diff := cmp.Diff(c, tc.expectedConditions[i], cmpopts.IgnoreFields(v1alpha1.Condition{}, "LastTransitionTime", "Message")); diff != "" { + t.Errorf("unexpected condition diff: %s", diff) + } + } }) } } From ae7f13ca8144ac1ad591a84a58d4fc94a1895dbe Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 13:41:33 -0400 Subject: [PATCH 09/17] first test for to catalog registration type --- .../api/v1alpha1/registration_types_test.go | 54 +++++++++++++++++++ .../registrations_controller_test.go | 2 + 2 files changed, 56 insertions(+) create mode 100644 control-plane/api/v1alpha1/registration_types_test.go diff --git a/control-plane/api/v1alpha1/registration_types_test.go b/control-plane/api/v1alpha1/registration_types_test.go new file mode 100644 index 0000000000..a27c031504 --- /dev/null +++ b/control-plane/api/v1alpha1/registration_types_test.go @@ -0,0 +1,54 @@ +package v1alpha1 + +import ( + "testing" + + capi "github.com/hashicorp/consul/api" + + "github.com/stretchr/testify/require" +) + +func TestToCatalogRegistration(tt *testing.T) { + cases := map[string]struct { + registration *Registration + expected *capi.CatalogRegistration + }{ + "minimal registration": { + registration: &Registration{ + Spec: RegistrationSpec{ + ID: "node-id", + Node: "node-virtual", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: Service{ + ID: "service-id", + Name: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + expected: &capi.CatalogRegistration{ + ID: "node-id", + Node: "node-virtual", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: &capi.AgentService{ + ID: "service-id", + Service: "service-name", + Port: 8080, + Address: "127.0.0.1", + }, + }, + }, + } + + for name, tc := range cases { + tc := tc + tt.Run(name, func(t *testing.T) { + t.Parallel() + actual := tc.registration.ToCatalogRegistration() + require.Equal(t, tc.expected, actual) + }) + } +} diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index 02357ceeed..2e5334090a 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -94,6 +94,7 @@ func TestReconcile_Success(tt *testing.T) { for name, tc := range cases { tt.Run(name, func(t *testing.T) { + t.Parallel() s := runtime.NewScheme() s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}) ctx := context.Background() @@ -215,6 +216,7 @@ func TestReconcile_Failure(tt *testing.T) { for name, tc := range cases { tt.Run(name, func(t *testing.T) { + t.Parallel() s := runtime.NewScheme() s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.Registration{}) ctx := context.Background() From 6f054d2f9f477bcc7956cc33393571e2a75e5a84 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 14:00:39 -0400 Subject: [PATCH 10/17] maximal registration to catalog test --- .../api/v1alpha1/registration_types.go | 21 ++- .../api/v1alpha1/registration_types_test.go | 149 ++++++++++++++++++ 2 files changed, 163 insertions(+), 7 deletions(-) diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go index c240a69f4b..a38c0df743 100644 --- a/control-plane/api/v1alpha1/registration_types.go +++ b/control-plane/api/v1alpha1/registration_types.go @@ -63,6 +63,7 @@ type RegistrationSpec struct { SkipNodeUpdate bool `json:"skipNodeUpdate,omitempty"` Partition string `json:"partition,omitempty"` HealthCheck *HealthCheck `json:"check,omitempty"` + Locality *Locality `json:"locality,omitempty"` } // +k8s:deepcopy-gen=true @@ -180,6 +181,7 @@ func (r *Registration) ToCatalogRegistration() *capi.CatalogRegistration { Check: copyHealthCheck(r.Spec.HealthCheck), SkipNodeUpdate: r.Spec.SkipNodeUpdate, Partition: r.Spec.Partition, + Locality: copyLocality(r.Spec.Locality), } } @@ -215,13 +217,18 @@ func copyHealthCheck(healthCheck *HealthCheck) *capi.AgentCheck { deregisterAfter, _ := time.ParseDuration(healthCheck.Definition.DeregisterCriticalServiceAfterDuration) return &capi.AgentCheck{ - CheckID: healthCheck.CheckID, - Name: healthCheck.Name, - Type: healthCheck.Type, - Status: healthCheck.Status, - ServiceID: healthCheck.ServiceID, - Output: healthCheck.Output, - Namespace: healthCheck.Namespace, + Node: healthCheck.Node, + Notes: healthCheck.Notes, + ServiceName: healthCheck.ServiceName, + CheckID: healthCheck.CheckID, + Name: healthCheck.Name, + Type: healthCheck.Type, + Status: healthCheck.Status, + ServiceID: healthCheck.ServiceID, + ExposedPort: healthCheck.ExposedPort, + Output: healthCheck.Output, + Namespace: healthCheck.Namespace, + Partition: healthCheck.Partition, Definition: capi.HealthCheckDefinition{ HTTP: healthCheck.Definition.HTTP, TCP: healthCheck.Definition.TCP, diff --git a/control-plane/api/v1alpha1/registration_types_test.go b/control-plane/api/v1alpha1/registration_types_test.go index a27c031504..4708fd0d0e 100644 --- a/control-plane/api/v1alpha1/registration_types_test.go +++ b/control-plane/api/v1alpha1/registration_types_test.go @@ -2,6 +2,7 @@ package v1alpha1 import ( "testing" + "time" capi "github.com/hashicorp/consul/api" @@ -41,6 +42,149 @@ func TestToCatalogRegistration(tt *testing.T) { }, }, }, + "maximal registration - http health check": { + registration: &Registration{ + Spec: RegistrationSpec{ + ID: "node-id", + Node: "node-virtual", + Address: "127.0.0.1", + TaggedAddresses: map[string]string{ + "lan": "8080", + }, + NodeMeta: map[string]string{ + "n1": "m1", + }, + Datacenter: "dc1", + Service: Service{ + ID: "service-id", + Name: "service-name", + Tags: []string{"tag1", "tag2"}, + Meta: map[string]string{ + "m1": "1", + "m2": "2", + }, + Port: 8080, + Address: "127.0.0.1", + TaggedAddresses: map[string]ServiceAddress{ + "lan": { + Address: "10.0.0.10", + Port: 5000, + }, + }, + Weights: Weights{ + Passing: 50, + Warning: 100, + }, + EnableTagOverride: true, + Locality: &Locality{ + Region: "us-east-1", + Zone: "auto", + }, + Namespace: "n1", + Partition: "p1", + }, + Partition: "p1", + HealthCheck: &HealthCheck{ + Node: "node-virtual", + CheckID: "service-check", + Name: "service-health", + Status: "passing", + Notes: "all about that service", + Output: "healthy", + ServiceID: "service-id", + ServiceName: "service-name", + Type: "readiness", + ExposedPort: 19000, + Definition: HealthCheckDefinition{ + HTTP: "/health", + Header: map[string][]string{ + "Content-Type": {"application/json"}, + }, + Method: "GET", + IntervalDuration: "5s", + TimeoutDuration: "10s", + DeregisterCriticalServiceAfterDuration: "30s", + }, + Namespace: "n1", + Partition: "p1", + }, + Locality: &Locality{ + Region: "us-east-1", + Zone: "auto", + }, + }, + }, + expected: &capi.CatalogRegistration{ + ID: "node-id", + Node: "node-virtual", + Address: "127.0.0.1", + TaggedAddresses: map[string]string{ + "lan": "8080", + }, + NodeMeta: map[string]string{ + "n1": "m1", + }, + Datacenter: "dc1", + Service: &capi.AgentService{ + ID: "service-id", + Service: "service-name", + Tags: []string{"tag1", "tag2"}, + Meta: map[string]string{ + "m1": "1", + "m2": "2", + }, + Port: 8080, + Address: "127.0.0.1", + TaggedAddresses: map[string]capi.ServiceAddress{ + "lan": { + Address: "10.0.0.10", + Port: 5000, + }, + }, + Weights: capi.AgentWeights{ + Passing: 50, + Warning: 100, + }, + EnableTagOverride: true, + Locality: &capi.Locality{ + Region: "us-east-1", + Zone: "auto", + }, + Namespace: "n1", + Partition: "p1", + }, + Check: &capi.AgentCheck{ + Node: "node-virtual", + CheckID: "service-check", + Name: "service-health", + Status: "passing", + Notes: "all about that service", + Output: "healthy", + ServiceID: "service-id", + ServiceName: "service-name", + Type: "readiness", + ExposedPort: 19000, + Definition: capi.HealthCheckDefinition{ + HTTP: "/health", + Header: map[string][]string{ + "Content-Type": {"application/json"}, + }, + Method: "GET", + IntervalDuration: toDuration("5s"), + TimeoutDuration: toDuration("10s"), + DeregisterCriticalServiceAfterDuration: toDuration("30s"), + }, + Namespace: "n1", + Partition: "p1", + }, + SkipNodeUpdate: false, + Partition: "p1", + Locality: &capi.Locality{ + Region: "us-east-1", + Zone: "auto", + }, + }, + }, } for name, tc := range cases { @@ -52,3 +196,8 @@ func TestToCatalogRegistration(tt *testing.T) { }) } } + +func toDuration(d string) time.Duration { + duration, _ := time.ParseDuration(d) + return duration +} From d44fc75109840a21215be38c2e4f4b012d3b1534 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 14:10:57 -0400 Subject: [PATCH 11/17] test all the things --- .../api/v1alpha1/registration_types_test.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/control-plane/api/v1alpha1/registration_types_test.go b/control-plane/api/v1alpha1/registration_types_test.go index 4708fd0d0e..7fe8bd06c8 100644 --- a/control-plane/api/v1alpha1/registration_types_test.go +++ b/control-plane/api/v1alpha1/registration_types_test.go @@ -42,7 +42,7 @@ func TestToCatalogRegistration(tt *testing.T) { }, }, }, - "maximal registration - http health check": { + "maximal registration": { registration: &Registration{ Spec: RegistrationSpec{ ID: "node-id", @@ -97,10 +97,17 @@ func TestToCatalogRegistration(tt *testing.T) { ExposedPort: 19000, Definition: HealthCheckDefinition{ HTTP: "/health", + TCP: "tcp-check", Header: map[string][]string{ "Content-Type": {"application/json"}, }, Method: "GET", + TLSServerName: "my-secure-tls-server", + TLSSkipVerify: true, + Body: "some-body", + GRPC: "/grpc-health-check", + GRPCUseTLS: true, + OSService: "osservice-name", IntervalDuration: "5s", TimeoutDuration: "10s", DeregisterCriticalServiceAfterDuration: "30s", @@ -166,10 +173,17 @@ func TestToCatalogRegistration(tt *testing.T) { ExposedPort: 19000, Definition: capi.HealthCheckDefinition{ HTTP: "/health", + TCP: "tcp-check", Header: map[string][]string{ "Content-Type": {"application/json"}, }, Method: "GET", + TLSServerName: "my-secure-tls-server", + TLSSkipVerify: true, + Body: "some-body", + GRPC: "/grpc-health-check", + GRPCUseTLS: true, + OSService: "osservice-name", IntervalDuration: toDuration("5s"), TimeoutDuration: toDuration("10s"), DeregisterCriticalServiceAfterDuration: toDuration("30s"), From 52e3e59524b7900a53a7bfb1194668d4986e1867 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 14:17:16 -0400 Subject: [PATCH 12/17] deregistration tests --- .../api/v1alpha1/registration_types_test.go | 82 +++++++++++++++++-- 1 file changed, 77 insertions(+), 5 deletions(-) diff --git a/control-plane/api/v1alpha1/registration_types_test.go b/control-plane/api/v1alpha1/registration_types_test.go index 7fe8bd06c8..53efb2843b 100644 --- a/control-plane/api/v1alpha1/registration_types_test.go +++ b/control-plane/api/v1alpha1/registration_types_test.go @@ -184,9 +184,9 @@ func TestToCatalogRegistration(tt *testing.T) { GRPC: "/grpc-health-check", GRPCUseTLS: true, OSService: "osservice-name", - IntervalDuration: toDuration("5s"), - TimeoutDuration: toDuration("10s"), - DeregisterCriticalServiceAfterDuration: toDuration("30s"), + IntervalDuration: toDuration(tt, "5s"), + TimeoutDuration: toDuration(tt, "10s"), + DeregisterCriticalServiceAfterDuration: toDuration(tt, "30s"), }, Namespace: "n1", Partition: "p1", @@ -211,7 +211,79 @@ func TestToCatalogRegistration(tt *testing.T) { } } -func toDuration(d string) time.Duration { - duration, _ := time.ParseDuration(d) +func TestToCatalogDeregistration(tt *testing.T) { + cases := map[string]struct { + registration *Registration + expected *capi.CatalogDeregistration + }{ + "with health check": { + registration: &Registration{ + Spec: RegistrationSpec{ + ID: "node-id", + Node: "node-virtual", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: Service{ + ID: "service-id", + Namespace: "n1", + Partition: "p1", + }, + HealthCheck: &HealthCheck{ + CheckID: "checkID", + }, + }, + }, + expected: &capi.CatalogDeregistration{ + Node: "node-virtual", + Address: "127.0.0.1", + Datacenter: "dc1", + ServiceID: "service-id", + CheckID: "checkID", + Namespace: "n1", + Partition: "p1", + }, + }, + "no health check": { + registration: &Registration{ + Spec: RegistrationSpec{ + ID: "node-id", + Node: "node-virtual", + Address: "127.0.0.1", + Datacenter: "dc1", + Service: Service{ + ID: "service-id", + Namespace: "n1", + Partition: "p1", + }, + }, + }, + expected: &capi.CatalogDeregistration{ + Node: "node-virtual", + Address: "127.0.0.1", + Datacenter: "dc1", + ServiceID: "service-id", + CheckID: "", + Namespace: "n1", + Partition: "p1", + }, + }, + } + + for name, tc := range cases { + tc := tc + tt.Run(name, func(t *testing.T) { + t.Parallel() + actual := tc.registration.ToCatalogDeregistration() + require.Equal(t, tc.expected, actual) + }) + } +} + +func toDuration(t *testing.T, d string) time.Duration { + t.Helper() + duration, err := time.ParseDuration(d) + if err != nil { + t.Fatal(err) + } return duration } From 4a5b2ea9899aa0356217bbc8d08ef3f0a19ec7d6 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 14:29:18 -0400 Subject: [PATCH 13/17] update some comments and fields, re-run generators --- charts/consul/templates/crd-registrations.yaml | 11 +++++++++-- control-plane/api/v1alpha1/registration_types.go | 7 +++++-- control-plane/api/v1alpha1/registration_types_test.go | 3 +++ control-plane/api/v1alpha1/zz_generated.deepcopy.go | 5 +++++ .../crd/bases/consul.hashicorp.com_registrations.yaml | 11 +++++++++-- .../configentries/registrations_controller.go | 11 +++++++++-- .../configentries/registrations_controller_test.go | 3 +++ 7 files changed, 43 insertions(+), 8 deletions(-) diff --git a/charts/consul/templates/crd-registrations.yaml b/charts/consul/templates/crd-registrations.yaml index 9faf0fb21f..c693977636 100644 --- a/charts/consul/templates/crd-registrations.yaml +++ b/charts/consul/templates/crd-registrations.yaml @@ -23,7 +23,7 @@ spec: - name: v1alpha1 schema: openAPIV3Schema: - description: Registration defines the. + description: Registration defines the resource for working with service registrations. properties: apiVersion: description: 'APIVersion defines the versioned schema of this representation @@ -43,7 +43,7 @@ spec: address: type: string check: - description: HealthCheck is used to represent a single check + description: HealthCheck is used to represent a single check. properties: checkId: type: string @@ -125,6 +125,13 @@ spec: type: string id: type: string + locality: + properties: + region: + type: string + zone: + type: string + type: object node: type: string nodeMeta: diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go index a38c0df743..a5e4966e4a 100644 --- a/control-plane/api/v1alpha1/registration_types.go +++ b/control-plane/api/v1alpha1/registration_types.go @@ -23,7 +23,7 @@ func init() { // +kubebuilder:resource:scope=Cluster // +kubebuilder:subresource:status -// Registration defines the. +// Registration defines the resource for working with service registrations. type Registration struct { // Standard Kubernetes resource metadata. metav1.TypeMeta `json:",inline"` @@ -107,7 +107,7 @@ type Locality struct { // +k8s:deepcopy-gen=true -// HealthCheck is used to represent a single check +// HealthCheck is used to represent a single check. type HealthCheck struct { Node string `json:"node"` CheckID string `json:"checkId"` @@ -155,6 +155,7 @@ type RegistrationList struct { Items []Registration `json:"items"` } +// ToCatalogRegistration converts a Registration to a Consul CatalogRegistration. func (r *Registration) ToCatalogRegistration() *capi.CatalogRegistration { return &capi.CatalogRegistration{ ID: r.Spec.ID, @@ -247,6 +248,7 @@ func copyHealthCheck(healthCheck *HealthCheck) *capi.AgentCheck { } } +// ToCatalogDeregistration converts a Registration to a Consul CatalogDergistration. func (r *Registration) ToCatalogDeregistration() *capi.CatalogDeregistration { checkID := "" if r.Spec.HealthCheck != nil { @@ -264,6 +266,7 @@ func (r *Registration) ToCatalogDeregistration() *capi.CatalogDeregistration { } } +// SetSyncedCondition sets the synced condition on the Registration. func (r *Registration) SetSyncedCondition(status corev1.ConditionStatus, reason string, message string) { r.Status.Conditions = Conditions{ { diff --git a/control-plane/api/v1alpha1/registration_types_test.go b/control-plane/api/v1alpha1/registration_types_test.go index 53efb2843b..207256ac91 100644 --- a/control-plane/api/v1alpha1/registration_types_test.go +++ b/control-plane/api/v1alpha1/registration_types_test.go @@ -1,3 +1,6 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + package v1alpha1 import ( diff --git a/control-plane/api/v1alpha1/zz_generated.deepcopy.go b/control-plane/api/v1alpha1/zz_generated.deepcopy.go index 8ca7a4f776..a8aed9b1ff 100644 --- a/control-plane/api/v1alpha1/zz_generated.deepcopy.go +++ b/control-plane/api/v1alpha1/zz_generated.deepcopy.go @@ -2626,6 +2626,11 @@ func (in *RegistrationSpec) DeepCopyInto(out *RegistrationSpec) { *out = new(HealthCheck) (*in).DeepCopyInto(*out) } + if in.Locality != nil { + in, out := &in.Locality, &out.Locality + *out = new(Locality) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RegistrationSpec. 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 58fada9d15..41b11ae569 100644 --- a/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml +++ b/control-plane/config/crd/bases/consul.hashicorp.com_registrations.yaml @@ -19,7 +19,7 @@ spec: - name: v1alpha1 schema: openAPIV3Schema: - description: Registration defines the. + description: Registration defines the resource for working with service registrations. properties: apiVersion: description: 'APIVersion defines the versioned schema of this representation @@ -39,7 +39,7 @@ spec: address: type: string check: - description: HealthCheck is used to represent a single check + description: HealthCheck is used to represent a single check. properties: checkId: type: string @@ -121,6 +121,13 @@ spec: type: string id: type: string + locality: + properties: + region: + type: string + zone: + type: string + type: object node: type: string nodeMeta: diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index 39c0f6e147..a4390f7be6 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -42,7 +42,7 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques log.Info("Reconciling Registaration") registration := &v1alpha1.Registration{} - // get the gateway + // get the registration if err := r.Client.Get(ctx, req.NamespacedName, registration); err != nil { if !k8serrors.IsNotFound(err) { log.Error(err, "unable to get registration") @@ -56,7 +56,6 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, err } - log.Info("Registration", "registration", registration) // deletion request if !registration.ObjectMeta.DeletionTimestamp.IsZero() { log.Info("Deregistering service") @@ -85,12 +84,14 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques func (r *RegistrationsController) registerService(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { patch := r.AddFinalizersPatch(registration, RegistrationFinalizer) + err := r.Patch(ctx, registration, patch) if err != nil { return err } regReq := registration.ToCatalogRegistration() + _, err = client.Catalog().Register(regReq, nil) if err != nil { log.Error(err, "error registering service", "svcName", regReq.Service.Service) @@ -103,6 +104,7 @@ func (r *RegistrationsController) registerService(ctx context.Context, log logr. func (r *RegistrationsController) deregisterService(ctx context.Context, log logr.Logger, client *capi.Client, registration *v1alpha1.Registration) error { deRegReq := registration.ToCatalogDeregistration() + _, err := client.Catalog().Deregister(deRegReq, nil) if err != nil { log.Error(err, "error deregistering service", "svcID", deRegReq.ServiceID) @@ -110,6 +112,7 @@ func (r *RegistrationsController) deregisterService(ctx context.Context, log log } patch := r.RemoveFinalizersPatch(registration, RegistrationFinalizer) + if err := r.Patch(ctx, registration, patch); err != nil { return err } @@ -123,6 +126,8 @@ func (r *RegistrationsController) Logger(name types.NamespacedName) logr.Logger func (r *RegistrationsController) updateStatusError(ctx context.Context, registration *v1alpha1.Registration, reason string, reconcileErr error) { registration.SetSyncedCondition(corev1.ConditionFalse, reason, reconcileErr.Error()) + registration.Status.LastSyncedTime = &metav1.Time{Time: time.Now()} + err := r.Status().Update(ctx, registration) if err != nil { r.Log.Error(err, "failed to update Registration status", "name", registration.Name, "namespace", registration.Namespace) @@ -131,6 +136,7 @@ func (r *RegistrationsController) updateStatusError(ctx context.Context, registr func (r *RegistrationsController) updateStatus(ctx context.Context, req types.NamespacedName) error { registration := &v1alpha1.Registration{} + err := r.Get(ctx, req, registration) if err != nil { return err @@ -138,6 +144,7 @@ func (r *RegistrationsController) updateStatus(ctx context.Context, req types.Na registration.Status.LastSyncedTime = &metav1.Time{Time: time.Now()} registration.SetSyncedCondition(corev1.ConditionTrue, "", "") + err = r.Status().Update(ctx, registration) if err != nil { r.Log.Error(err, "failed to update Registration status", "name", registration.Name, "namespace", registration.Namespace) diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index 2e5334090a..366eb3eaa0 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -1,3 +1,6 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + package configentries_test import ( From 1570d486cca1b555e5e18d2116d4c87d327e5dd6 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 14:30:20 -0400 Subject: [PATCH 14/17] Added changelog --- .changelog/3943.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/3943.txt diff --git a/.changelog/3943.txt b/.changelog/3943.txt new file mode 100644 index 0000000000..3be45fc453 --- /dev/null +++ b/.changelog/3943.txt @@ -0,0 +1,3 @@ +```release-note:feature +control-plane: Add the ability to register services via CRD. +``` From 321aa265f86c9a7b95374c808ef3b4e72fb09b28 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Wed, 1 May 2024 14:44:15 -0400 Subject: [PATCH 15/17] linting all the things --- control-plane/cni/main.go | 2 +- .../controllers/configentries/registrations_controller.go | 6 +++++- .../configentries/registrations_controller_test.go | 2 ++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/control-plane/cni/main.go b/control-plane/cni/main.go index eb710ff9cb..0f9a32b265 100644 --- a/control-plane/cni/main.go +++ b/control-plane/cni/main.go @@ -269,7 +269,7 @@ func main() { } // createK8sClient configures the command's Kubernetes API client if it doesn't -// already exist +// already exist. func (c *Command) createK8sClient(cfg *PluginConf) error { restConfig, err := clientcmd.BuildConfigFromFlags("", filepath.Join(cfg.CNINetDir, cfg.Kubeconfig)) if err != nil { diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index a4390f7be6..d0a51bbed6 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -64,7 +64,11 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques r.updateStatusError(ctx, registration, "ConsulErrorDeregistration", err) return ctrl.Result{}, err } - r.updateStatus(ctx, req.NamespacedName) + err := r.updateStatus(ctx, req.NamespacedName) + if err != nil { + log.Error(err, "failed to update status") + } + return ctrl.Result{}, nil } diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index 366eb3eaa0..a011168ad1 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -96,6 +96,7 @@ func TestReconcile_Success(tt *testing.T) { } for name, tc := range cases { + tc := tc tt.Run(name, func(t *testing.T) { t.Parallel() s := runtime.NewScheme() @@ -218,6 +219,7 @@ func TestReconcile_Failure(tt *testing.T) { } for name, tc := range cases { + tc := tc tt.Run(name, func(t *testing.T) { t.Parallel() s := runtime.NewScheme() From e1791b27e5bd2ed85f262a2781fd94307f96f148 Mon Sep 17 00:00:00 2001 From: jm96441n Date: Mon, 6 May 2024 13:01:25 -0400 Subject: [PATCH 16/17] fixing test setup for new controller runtime --- .../configentries/registrations_controller_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/control-plane/controllers/configentries/registrations_controller_test.go b/control-plane/controllers/configentries/registrations_controller_test.go index a011168ad1..5e2b2721fa 100644 --- a/control-plane/controllers/configentries/registrations_controller_test.go +++ b/control-plane/controllers/configentries/registrations_controller_test.go @@ -120,7 +120,11 @@ func TestReconcile_Success(tt *testing.T) { Watcher: test.MockConnMgrForIPAndPort(t, host, port, false), } - fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(tc.registration).Build() + fakeClient := fake.NewClientBuilder(). + WithScheme(s). + WithRuntimeObjects(tc.registration). + WithStatusSubresource(&v1alpha1.Registration{}). + Build() controller := &configentries.RegistrationsController{ Client: fakeClient, @@ -243,7 +247,11 @@ func TestReconcile_Failure(tt *testing.T) { Watcher: test.MockConnMgrForIPAndPort(t, host, port, false), } - fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(tc.registration).Build() + fakeClient := fake.NewClientBuilder(). + WithScheme(s). + WithRuntimeObjects(tc.registration). + WithStatusSubresource(&v1alpha1.Registration{}). + Build() controller := &configentries.RegistrationsController{ Client: fakeClient, From faf95793e938b21eb99759918d91dc06cf6ecb6a Mon Sep 17 00:00:00 2001 From: jm96441n Date: Tue, 7 May 2024 14:14:34 -0400 Subject: [PATCH 17/17] Handle errors for parsing duration --- .../api/v1alpha1/registration_types.go | 47 ++++++++++++++----- .../api/v1alpha1/registration_types_test.go | 3 +- .../configentries/registrations_controller.go | 5 +- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/control-plane/api/v1alpha1/registration_types.go b/control-plane/api/v1alpha1/registration_types.go index a5e4966e4a..9ca58e7c23 100644 --- a/control-plane/api/v1alpha1/registration_types.go +++ b/control-plane/api/v1alpha1/registration_types.go @@ -4,6 +4,7 @@ package v1alpha1 import ( + "errors" "maps" "slices" "time" @@ -146,17 +147,22 @@ type HealthCheckDefinition struct { // +kubebuilder:object:root=true -// RegistrationList is a list of Config resources. +// RegistrationList is a list of Registration resources. type RegistrationList struct { metav1.TypeMeta `json:",inline"` metav1.ListMeta `json:"metadata"` - // Items is the list of Configs. + // Items is the list of Registrations. Items []Registration `json:"items"` } // ToCatalogRegistration converts a Registration to a Consul CatalogRegistration. -func (r *Registration) ToCatalogRegistration() *capi.CatalogRegistration { +func (r *Registration) ToCatalogRegistration() (*capi.CatalogRegistration, error) { + check, err := copyHealthCheck(r.Spec.HealthCheck) + if err != nil { + return nil, err + } + return &capi.CatalogRegistration{ ID: r.Spec.ID, Node: r.Spec.Node, @@ -179,11 +185,11 @@ func (r *Registration) ToCatalogRegistration() *capi.CatalogRegistration { Partition: r.Spec.Service.Partition, Locality: copyLocality(r.Spec.Service.Locality), }, - Check: copyHealthCheck(r.Spec.HealthCheck), + Check: check, SkipNodeUpdate: r.Spec.SkipNodeUpdate, Partition: r.Spec.Partition, Locality: copyLocality(r.Spec.Locality), - } + }, nil } func copyTaggedAddresses(taggedAddresses map[string]ServiceAddress) map[string]capi.ServiceAddress { @@ -207,15 +213,32 @@ func copyLocality(locality *Locality) *capi.Locality { } } -func copyHealthCheck(healthCheck *HealthCheck) *capi.AgentCheck { +var ( + ErrInvalidInterval = errors.New("invalid value for IntervalDuration") + ErrInvalidTimeout = errors.New("invalid value for TimeoutDuration") + ErrInvalidDergisterAfter = errors.New("invalid value for DeregisterCriticalServiceAfterDuration") +) + +func copyHealthCheck(healthCheck *HealthCheck) (*capi.AgentCheck, error) { if healthCheck == nil { - return nil + return nil, nil } // TODO: handle error - intervalDuration, _ := time.ParseDuration(healthCheck.Definition.IntervalDuration) - timeoutDuration, _ := time.ParseDuration(healthCheck.Definition.TimeoutDuration) - deregisterAfter, _ := time.ParseDuration(healthCheck.Definition.DeregisterCriticalServiceAfterDuration) + intervalDuration, err := time.ParseDuration(healthCheck.Definition.IntervalDuration) + if err != nil { + return nil, ErrInvalidInterval + } + + timeoutDuration, err := time.ParseDuration(healthCheck.Definition.TimeoutDuration) + if err != nil { + return nil, ErrInvalidTimeout + } + + deregisterAfter, err := time.ParseDuration(healthCheck.Definition.DeregisterCriticalServiceAfterDuration) + if err != nil { + return nil, ErrInvalidDergisterAfter + } return &capi.AgentCheck{ Node: healthCheck.Node, @@ -245,10 +268,10 @@ func copyHealthCheck(healthCheck *HealthCheck) *capi.AgentCheck { TimeoutDuration: timeoutDuration, DeregisterCriticalServiceAfterDuration: deregisterAfter, }, - } + }, nil } -// ToCatalogDeregistration converts a Registration to a Consul CatalogDergistration. +// ToCatalogDeregistration converts a Registration to a Consul CatalogDeregistration. func (r *Registration) ToCatalogDeregistration() *capi.CatalogDeregistration { checkID := "" if r.Spec.HealthCheck != nil { diff --git a/control-plane/api/v1alpha1/registration_types_test.go b/control-plane/api/v1alpha1/registration_types_test.go index 207256ac91..8c3744efb9 100644 --- a/control-plane/api/v1alpha1/registration_types_test.go +++ b/control-plane/api/v1alpha1/registration_types_test.go @@ -208,7 +208,8 @@ func TestToCatalogRegistration(tt *testing.T) { tc := tc tt.Run(name, func(t *testing.T) { t.Parallel() - actual := tc.registration.ToCatalogRegistration() + actual, err := tc.registration.ToCatalogRegistration() + require.NoError(t, err) require.Equal(t, tc.expected, actual) }) } diff --git a/control-plane/controllers/configentries/registrations_controller.go b/control-plane/controllers/configentries/registrations_controller.go index d0a51bbed6..c9137ff63a 100644 --- a/control-plane/controllers/configentries/registrations_controller.go +++ b/control-plane/controllers/configentries/registrations_controller.go @@ -94,7 +94,10 @@ func (r *RegistrationsController) registerService(ctx context.Context, log logr. return err } - regReq := registration.ToCatalogRegistration() + regReq, err := registration.ToCatalogRegistration() + if err != nil { + return err + } _, err = client.Catalog().Register(regReq, nil) if err != nil {