From 0985b0ddcb0d267d70901d5283101e95014f2f31 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Mon, 11 Jul 2022 11:35:58 -0400 Subject: [PATCH 1/2] Support Conditions on Peering CRDs - Add printer columns so describes have more metadata. --- .gitignore | 1 + .../templates/crd-peeringacceptors.yaml | 65 +++++++++++++----- .../consul/templates/crd-peeringdialers.yaml | 65 +++++++++++++----- .../api/v1alpha1/peeringacceptor_types.go | 38 +++++++---- .../api/v1alpha1/peeringdialer_types.go | 31 +++++++-- .../api/v1alpha1/zz_generated.deepcopy.go | 65 ++++++------------ ...consul.hashicorp.com_peeringacceptors.yaml | 65 +++++++++++++----- .../consul.hashicorp.com_peeringdialers.yaml | 65 +++++++++++++----- .../peering_acceptor_controller.go | 39 +++++------ .../peering_acceptor_controller_test.go | 50 +++++++++----- .../peering_dialer_controller.go | 42 ++++-------- .../peering_dialer_controller_test.go | 68 +++++++++---------- 12 files changed, 361 insertions(+), 233 deletions(-) diff --git a/.gitignore b/.gitignore index 33b40aed54..8014afb2bf 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ .DS_Store .terraform/ .terraform.tfstate* +.terraform.lock.hcl terraform.tfstate* terraform.tfvars values.dev.yaml diff --git a/charts/consul/templates/crd-peeringacceptors.yaml b/charts/consul/templates/crd-peeringacceptors.yaml index 6cd7090ef7..e06e830f04 100644 --- a/charts/consul/templates/crd-peeringacceptors.yaml +++ b/charts/consul/templates/crd-peeringacceptors.yaml @@ -19,10 +19,25 @@ spec: kind: PeeringAcceptor listKind: PeeringAcceptorList plural: peeringacceptors + shortNames: + - peering-acceptor singular: peeringacceptor scope: Namespaced versions: - - name: v1alpha1 + - additionalPrinterColumns: + - description: The sync status of the resource with Consul + jsonPath: .status.conditions[?(@.type=="Synced")].status + name: Synced + type: string + - description: The last successful synced time of the resource with Consul + jsonPath: .status.lastSyncedTime + name: Last Synced + type: date + - description: The age of the resource + jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 schema: openAPIV3Schema: description: PeeringAcceptor is the Schema for the peeringacceptors API. @@ -67,8 +82,39 @@ spec: status: description: PeeringAcceptorStatus defines the observed state of PeeringAcceptor. properties: - lastReconcileTime: - description: LastReconcileTime is the last time the resource was reconciled. + 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 latestPeeringVersion: @@ -76,19 +122,6 @@ spec: that was reconciled. format: int64 type: integer - reconcileError: - description: ReconcileError shows any errors during the last reconciliation - of this resource. - properties: - error: - description: Error is a boolean indicating if there was an error - during the last reconcile of this resource. - type: boolean - message: - description: Message displays the error message from the last - reconcile. - type: string - type: object secret: description: SecretRef shows the status of the secret. properties: diff --git a/charts/consul/templates/crd-peeringdialers.yaml b/charts/consul/templates/crd-peeringdialers.yaml index d5dee45f61..e24401e761 100644 --- a/charts/consul/templates/crd-peeringdialers.yaml +++ b/charts/consul/templates/crd-peeringdialers.yaml @@ -19,10 +19,25 @@ spec: kind: PeeringDialer listKind: PeeringDialerList plural: peeringdialers + shortNames: + - peering-dialer singular: peeringdialer scope: Namespaced versions: - - name: v1alpha1 + - additionalPrinterColumns: + - description: The sync status of the resource with Consul + jsonPath: .status.conditions[?(@.type=="Synced")].status + name: Synced + type: string + - description: The last successful synced time of the resource with Consul + jsonPath: .status.lastSyncedTime + name: Last Synced + type: date + - description: The age of the resource + jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 schema: openAPIV3Schema: description: PeeringDialer is the Schema for the peeringdialers API. @@ -67,8 +82,39 @@ spec: status: description: PeeringDialerStatus defines the observed state of PeeringDialer. properties: - lastReconcileTime: - description: LastReconcileTime is the last time the resource was reconciled. + 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 latestPeeringVersion: @@ -76,19 +122,6 @@ spec: that was reconciled. format: int64 type: integer - reconcileError: - description: ReconcileError shows any errors during the last reconciliation - of this resource. - properties: - error: - description: Error is a boolean indicating if there was an error - during the last reconcile of this resource. - type: boolean - message: - description: Message displays the error message from the last - reconcile. - type: string - type: object secret: description: SecretRef shows the status of the secret. properties: diff --git a/control-plane/api/v1alpha1/peeringacceptor_types.go b/control-plane/api/v1alpha1/peeringacceptor_types.go index ad671a9e96..032870cb80 100644 --- a/control-plane/api/v1alpha1/peeringacceptor_types.go +++ b/control-plane/api/v1alpha1/peeringacceptor_types.go @@ -1,6 +1,7 @@ package v1alpha1 import ( + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -20,6 +21,10 @@ func init() { //+kubebuilder:subresource:status // PeeringAcceptor is the Schema for the peeringacceptors API. +// +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type==\"Synced\")].status",description="The sync status of the resource with Consul" +// +kubebuilder:printcolumn:name="Last Synced",type="date",JSONPath=".status.lastSyncedTime",description="The last successful synced time of the resource with Consul" +// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="The age of the resource" +// +kubebuilder:resource:shortName="peering-acceptor" type PeeringAcceptor struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` @@ -61,22 +66,17 @@ type Secret struct { type PeeringAcceptorStatus struct { // LatestPeeringVersion is the latest version of the resource that was reconciled. LatestPeeringVersion *uint64 `json:"latestPeeringVersion,omitempty"` - // LastReconcileTime is the last time the resource was reconciled. - // +optional - LastReconcileTime *metav1.Time `json:"lastReconcileTime,omitempty" description:"last time the resource was reconciled"` - // ReconcileError shows any errors during the last reconciliation of this resource. - // +optional - ReconcileError *ReconcileErrorStatus `json:"reconcileError,omitempty"` // SecretRef shows the status of the secret. // +optional SecretRef *SecretRefStatus `json:"secret,omitempty"` -} - -type ReconcileErrorStatus struct { - // Error is a boolean indicating if there was an error during the last reconcile of this resource. - Error *bool `json:"error,omitempty"` - // Message displays the error message from the last reconcile. - Message *string `json:"message,omitempty"` + // 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"` } type SecretRefStatus struct { @@ -124,3 +124,15 @@ func (pa *PeeringAcceptor) Validate() error { } return nil } + +func (pa *PeeringAcceptor) SetSyncedCondition(status corev1.ConditionStatus, reason string, message string) { + pa.Status.Conditions = Conditions{ + { + Type: ConditionSynced, + Status: status, + LastTransitionTime: metav1.Now(), + Reason: reason, + Message: message, + }, + } +} diff --git a/control-plane/api/v1alpha1/peeringdialer_types.go b/control-plane/api/v1alpha1/peeringdialer_types.go index 4998b27543..4ddde3fe2f 100644 --- a/control-plane/api/v1alpha1/peeringdialer_types.go +++ b/control-plane/api/v1alpha1/peeringdialer_types.go @@ -1,6 +1,7 @@ package v1alpha1 import ( + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -19,6 +20,10 @@ func init() { //+kubebuilder:subresource:status // PeeringDialer is the Schema for the peeringdialers API. +// +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type==\"Synced\")].status",description="The sync status of the resource with Consul" +// +kubebuilder:printcolumn:name="Last Synced",type="date",JSONPath=".status.lastSyncedTime",description="The last successful synced time of the resource with Consul" +// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="The age of the resource" +// +kubebuilder:resource:shortName="peering-dialer" type PeeringDialer struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` @@ -46,15 +51,17 @@ type PeeringDialerSpec struct { type PeeringDialerStatus struct { // LatestPeeringVersion is the latest version of the resource that was reconciled. LatestPeeringVersion *uint64 `json:"latestPeeringVersion,omitempty"` - // LastReconcileTime is the last time the resource was reconciled. - // +optional - LastReconcileTime *metav1.Time `json:"lastReconcileTime,omitempty" description:"last time the resource was reconciled"` - // ReconcileError shows any errors during the last reconciliation of this resource. - // +optional - ReconcileError *ReconcileErrorStatus `json:"reconcileError,omitempty"` // SecretRef shows the status of the secret. // +optional SecretRef *SecretRefStatus `json:"secret,omitempty"` + // 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"` } func (pd *PeeringDialer) Secret() *Secret { @@ -99,3 +106,15 @@ func (pd *PeeringDialer) Validate() error { } return nil } + +func (pd *PeeringDialer) SetSyncedCondition(status corev1.ConditionStatus, reason string, message string) { + pd.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 4806d72e6b..437ca89d1c 100644 --- a/control-plane/api/v1alpha1/zz_generated.deepcopy.go +++ b/control-plane/api/v1alpha1/zz_generated.deepcopy.go @@ -921,20 +921,22 @@ func (in *PeeringAcceptorStatus) DeepCopyInto(out *PeeringAcceptorStatus) { *out = new(uint64) **out = **in } - if in.LastReconcileTime != nil { - in, out := &in.LastReconcileTime, &out.LastReconcileTime - *out = (*in).DeepCopy() - } - if in.ReconcileError != nil { - in, out := &in.ReconcileError, &out.ReconcileError - *out = new(ReconcileErrorStatus) - (*in).DeepCopyInto(*out) - } if in.SecretRef != nil { in, out := &in.SecretRef, &out.SecretRef *out = new(SecretRefStatus) **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 PeeringAcceptorStatus. @@ -1034,20 +1036,22 @@ func (in *PeeringDialerStatus) DeepCopyInto(out *PeeringDialerStatus) { *out = new(uint64) **out = **in } - if in.LastReconcileTime != nil { - in, out := &in.LastReconcileTime, &out.LastReconcileTime - *out = (*in).DeepCopy() - } - if in.ReconcileError != nil { - in, out := &in.ReconcileError, &out.ReconcileError - *out = new(ReconcileErrorStatus) - (*in).DeepCopyInto(*out) - } if in.SecretRef != nil { in, out := &in.SecretRef, &out.SecretRef *out = new(SecretRefStatus) **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 PeeringDialerStatus. @@ -1151,31 +1155,6 @@ func (in *ProxyDefaultsSpec) DeepCopy() *ProxyDefaultsSpec { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ReconcileErrorStatus) DeepCopyInto(out *ReconcileErrorStatus) { - *out = *in - if in.Error != nil { - in, out := &in.Error, &out.Error - *out = new(bool) - **out = **in - } - if in.Message != nil { - in, out := &in.Message, &out.Message - *out = new(string) - **out = **in - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ReconcileErrorStatus. -func (in *ReconcileErrorStatus) DeepCopy() *ReconcileErrorStatus { - if in == nil { - return nil - } - out := new(ReconcileErrorStatus) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RingHashConfig) DeepCopyInto(out *RingHashConfig) { *out = *in diff --git a/control-plane/config/crd/bases/consul.hashicorp.com_peeringacceptors.yaml b/control-plane/config/crd/bases/consul.hashicorp.com_peeringacceptors.yaml index f2df1eb96b..e782ef472f 100644 --- a/control-plane/config/crd/bases/consul.hashicorp.com_peeringacceptors.yaml +++ b/control-plane/config/crd/bases/consul.hashicorp.com_peeringacceptors.yaml @@ -12,10 +12,25 @@ spec: kind: PeeringAcceptor listKind: PeeringAcceptorList plural: peeringacceptors + shortNames: + - peering-acceptor singular: peeringacceptor scope: Namespaced versions: - - name: v1alpha1 + - additionalPrinterColumns: + - description: The sync status of the resource with Consul + jsonPath: .status.conditions[?(@.type=="Synced")].status + name: Synced + type: string + - description: The last successful synced time of the resource with Consul + jsonPath: .status.lastSyncedTime + name: Last Synced + type: date + - description: The age of the resource + jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 schema: openAPIV3Schema: description: PeeringAcceptor is the Schema for the peeringacceptors API. @@ -60,8 +75,39 @@ spec: status: description: PeeringAcceptorStatus defines the observed state of PeeringAcceptor. properties: - lastReconcileTime: - description: LastReconcileTime is the last time the resource was reconciled. + 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 latestPeeringVersion: @@ -69,19 +115,6 @@ spec: that was reconciled. format: int64 type: integer - reconcileError: - description: ReconcileError shows any errors during the last reconciliation - of this resource. - properties: - error: - description: Error is a boolean indicating if there was an error - during the last reconcile of this resource. - type: boolean - message: - description: Message displays the error message from the last - reconcile. - type: string - type: object secret: description: SecretRef shows the status of the secret. properties: diff --git a/control-plane/config/crd/bases/consul.hashicorp.com_peeringdialers.yaml b/control-plane/config/crd/bases/consul.hashicorp.com_peeringdialers.yaml index 8492acec3a..d5103252a5 100644 --- a/control-plane/config/crd/bases/consul.hashicorp.com_peeringdialers.yaml +++ b/control-plane/config/crd/bases/consul.hashicorp.com_peeringdialers.yaml @@ -12,10 +12,25 @@ spec: kind: PeeringDialer listKind: PeeringDialerList plural: peeringdialers + shortNames: + - peering-dialer singular: peeringdialer scope: Namespaced versions: - - name: v1alpha1 + - additionalPrinterColumns: + - description: The sync status of the resource with Consul + jsonPath: .status.conditions[?(@.type=="Synced")].status + name: Synced + type: string + - description: The last successful synced time of the resource with Consul + jsonPath: .status.lastSyncedTime + name: Last Synced + type: date + - description: The age of the resource + jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 schema: openAPIV3Schema: description: PeeringDialer is the Schema for the peeringdialers API. @@ -60,8 +75,39 @@ spec: status: description: PeeringDialerStatus defines the observed state of PeeringDialer. properties: - lastReconcileTime: - description: LastReconcileTime is the last time the resource was reconciled. + 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 latestPeeringVersion: @@ -69,19 +115,6 @@ spec: that was reconciled. format: int64 type: integer - reconcileError: - description: ReconcileError shows any errors during the last reconciliation - of this resource. - properties: - error: - description: Error is a boolean indicating if there was an error - during the last reconcile of this resource. - type: boolean - message: - description: Message displays the error message from the last - reconcile. - type: string - type: object secret: description: SecretRef shows the status of the secret. properties: diff --git a/control-plane/connect-inject/peering_acceptor_controller.go b/control-plane/connect-inject/peering_acceptor_controller.go index dff8651fd5..c6777fdce4 100644 --- a/control-plane/connect-inject/peering_acceptor_controller.go +++ b/control-plane/connect-inject/peering_acceptor_controller.go @@ -34,7 +34,12 @@ type PeeringAcceptorController struct { context.Context } -const FinalizerName = "finalizers.consul.hashicorp.com" +const ( + FinalizerName = "finalizers.consul.hashicorp.com" + ConsulAgentError = "ConsulAgentError" + InternalError = "InternalError" + KubernetesError = "KubernetesError" +) //+kubebuilder:rbac:groups=consul.hashicorp.com,resources=peeringacceptors,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=consul.hashicorp.com,resources=peeringacceptors/status,verbs=get;update;patch @@ -100,7 +105,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ if statusSecretSet { existingStatusSecret, err = r.getExistingSecret(ctx, acceptor.SecretRef().Name, acceptor.Namespace) if err != nil { - r.updateStatusError(ctx, acceptor, err) + r.updateStatusError(ctx, acceptor, KubernetesError, err) return ctrl.Result{}, err } } @@ -124,7 +129,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ r.Log.Info("stale secret in status; deleting stale secret", "name", acceptor.Name) err := r.Client.Delete(ctx, existingStatusSecret) if err != nil { - r.updateStatusError(ctx, acceptor, err) + r.updateStatusError(ctx, acceptor, KubernetesError, err) return ctrl.Result{}, err } } @@ -132,13 +137,13 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ // Generate and store the peering token. var resp *api.PeeringGenerateTokenResponse if resp, err = r.generateToken(ctx, acceptor.Name); err != nil { - r.updateStatusError(ctx, acceptor, err) + r.updateStatusError(ctx, acceptor, ConsulAgentError, err) return ctrl.Result{}, err } if acceptor.Secret().Backend == "kubernetes" { secretResourceVersion, err = r.createOrUpdateK8sSecret(ctx, acceptor, resp) if err != nil { - r.updateStatusError(ctx, acceptor, err) + r.updateStatusError(ctx, acceptor, KubernetesError, err) return ctrl.Result{}, err } } @@ -160,7 +165,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ if statusSecretSet { shouldGenerate, nameChanged, err = shouldGenerateToken(acceptor, existingStatusSecret) if err != nil { - r.updateStatusError(ctx, acceptor, err) + r.updateStatusError(ctx, acceptor, InternalError, err) return ctrl.Result{}, err } } else { @@ -184,7 +189,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ if existingStatusSecret != nil { err := r.Client.Delete(ctx, existingStatusSecret) if err != nil { - r.updateStatusError(ctx, acceptor, err) + r.updateStatusError(ctx, acceptor, ConsulAgentError, err) return ctrl.Result{}, err } } @@ -243,11 +248,8 @@ func (r *PeeringAcceptorController) updateStatus(ctx context.Context, acceptor * Secret: *acceptor.Secret(), ResourceVersion: secretResourceVersion, } - acceptor.Status.LastReconcileTime = &metav1.Time{Time: time.Now()} - acceptor.Status.ReconcileError = &consulv1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(false), - Message: pointerToString(""), - } + acceptor.Status.LastSyncedTime = &metav1.Time{Time: time.Now()} + acceptor.SetSyncedCondition(corev1.ConditionTrue, "", "") if peeringVersionString, ok := acceptor.Annotations[annotationPeeringVersion]; ok { peeringVersion, err := strconv.ParseUint(peeringVersionString, 10, 64) if err != nil { @@ -266,13 +268,8 @@ func (r *PeeringAcceptorController) updateStatus(ctx context.Context, acceptor * } // updateStatusError updates the peeringAcceptor's ReconcileError in the status. -func (r *PeeringAcceptorController) updateStatusError(ctx context.Context, acceptor *consulv1alpha1.PeeringAcceptor, reconcileErr error) { - acceptor.Status.ReconcileError = &consulv1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(true), - Message: pointerToString(reconcileErr.Error()), - } - - acceptor.Status.LastReconcileTime = &metav1.Time{Time: time.Now()} +func (r *PeeringAcceptorController) updateStatusError(ctx context.Context, acceptor *consulv1alpha1.PeeringAcceptor, reason string, reconcileErr error) { + acceptor.SetSyncedCondition(corev1.ConditionFalse, reason, reconcileErr.Error()) err := r.Status().Update(ctx, acceptor) if err != nil { r.Log.Error(err, "failed to update PeeringAcceptor status", "name", acceptor.Name, "namespace", acceptor.Namespace) @@ -427,10 +424,6 @@ func createSecret(name, namespace, key, value string) *corev1.Secret { return secret } -func pointerToString(s string) *string { - return &s -} - // containsString returns true if s is in slice. func containsString(slice []string, s string) bool { for _, item := range slice { diff --git a/control-plane/connect-inject/peering_acceptor_controller_test.go b/control-plane/connect-inject/peering_acceptor_controller_test.go index 69599f84a5..8a38762c4e 100644 --- a/control-plane/connect-inject/peering_acceptor_controller_test.go +++ b/control-plane/connect-inject/peering_acceptor_controller_test.go @@ -979,9 +979,11 @@ func TestAcceptorUpdateStatus(t *testing.T) { }, ResourceVersion: "1234", }, - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(false), - Message: pointerToString(""), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionTrue, + }, }, }, }, @@ -1022,9 +1024,11 @@ func TestAcceptorUpdateStatus(t *testing.T) { }, ResourceVersion: "1234", }, - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(false), - Message: pointerToString(""), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionTrue, + }, }, }, }, @@ -1062,7 +1066,7 @@ func TestAcceptorUpdateStatus(t *testing.T) { require.Equal(t, tt.expStatus.SecretRef.Key, acceptor.SecretRef().Key) require.Equal(t, tt.expStatus.SecretRef.Backend, acceptor.SecretRef().Backend) require.Equal(t, tt.expStatus.SecretRef.ResourceVersion, acceptor.SecretRef().ResourceVersion) - require.Equal(t, *tt.expStatus.ReconcileError.Error, *acceptor.Status.ReconcileError.Error) + require.Equal(t, tt.expStatus.Conditions[0].Message, acceptor.Status.Conditions[0].Message) }) } @@ -1094,9 +1098,13 @@ func TestAcceptorUpdateStatusError(t *testing.T) { }, reconcileErr: errors.New("this is an error"), expStatus: v1alpha1.PeeringAcceptorStatus{ - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(true), - Message: pointerToString("this is an error"), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionFalse, + Reason: InternalError, + Message: "this is an error", + }, }, }, }, @@ -1117,17 +1125,23 @@ func TestAcceptorUpdateStatusError(t *testing.T) { }, }, Status: v1alpha1.PeeringAcceptorStatus{ - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(false), - Message: pointerToString(""), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionTrue, + }, }, }, }, reconcileErr: errors.New("this is an error"), expStatus: v1alpha1.PeeringAcceptorStatus{ - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(true), - Message: pointerToString("this is an error"), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionFalse, + Reason: InternalError, + Message: "this is an error", + }, }, }, }, @@ -1151,7 +1165,7 @@ func TestAcceptorUpdateStatusError(t *testing.T) { Scheme: s, } - controller.updateStatusError(context.Background(), tt.acceptor, tt.reconcileErr) + controller.updateStatusError(context.Background(), tt.acceptor, InternalError, tt.reconcileErr) acceptor := &v1alpha1.PeeringAcceptor{} acceptorName := types.NamespacedName{ @@ -1160,7 +1174,7 @@ func TestAcceptorUpdateStatusError(t *testing.T) { } err := fakeClient.Get(context.Background(), acceptorName, acceptor) require.NoError(t, err) - require.Equal(t, *tt.expStatus.ReconcileError.Error, *acceptor.Status.ReconcileError.Error) + require.Equal(t, tt.expStatus.Conditions[0].Message, acceptor.Status.Conditions[0].Message) }) } diff --git a/control-plane/connect-inject/peering_dialer_controller.go b/control-plane/connect-inject/peering_dialer_controller.go index a5f3e19094..ffa88e3d61 100644 --- a/control-plane/connect-inject/peering_dialer_controller.go +++ b/control-plane/connect-inject/peering_dialer_controller.go @@ -78,29 +78,18 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques } } - // TODO(peering): remove this once CRD validation exists. - secretSet := false - if dialer.Secret() != nil { - secretSet = true - } - if !secretSet { - err = errors.New("PeeringDialer spec.peer.secret was not set") - r.updateStatusError(ctx, dialer, err) - return ctrl.Result{}, err - } - // specSecret will be nil if the secret specified by the spec doesn't exist. var specSecret *corev1.Secret specSecret, err = r.getSecret(ctx, dialer.Secret().Name, dialer.Namespace) if err != nil { - r.updateStatusError(ctx, dialer, err) + r.updateStatusError(ctx, dialer, KubernetesError, err) return ctrl.Result{}, err } // If specSecret doesn't exist, error because we can only initiate peering if we have a token to initiate with. if specSecret == nil { err = errors.New("PeeringDialer spec.peer.secret does not exist") - r.updateStatusError(ctx, dialer, err) + r.updateStatusError(ctx, dialer, InternalError, err) return ctrl.Result{}, err } @@ -115,7 +104,7 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques if secretRefSet { statusSecret, err = r.getSecret(ctx, dialer.SecretRef().Name, dialer.Namespace) if err != nil { - r.updateStatusError(ctx, dialer, err) + r.updateStatusError(ctx, dialer, KubernetesError, err) return ctrl.Result{}, err } } @@ -128,7 +117,7 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques r.Log.Info("the secret in status.secretRef doesn't exist or wasn't set, establishing peering with the existing spec.peer.secret", "secret-name", dialer.Secret().Name, "secret-namespace", dialer.Namespace) peeringToken := specSecret.Data[dialer.Secret().Key] if err := r.establishPeering(ctx, dialer.Name, string(peeringToken)); err != nil { - r.updateStatusError(ctx, dialer, err) + r.updateStatusError(ctx, dialer, ConsulAgentError, err) return ctrl.Result{}, err } else { err := r.updateStatus(ctx, dialer, specSecret.ResourceVersion) @@ -151,7 +140,7 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques r.Log.Info("status.secret exists, but the peering doesn't exist in Consul; establishing peering with the existing spec.peer.secret", "secret-name", dialer.Secret().Name, "secret-namespace", dialer.Namespace) peeringToken := specSecret.Data[dialer.Secret().Key] if err := r.establishPeering(ctx, dialer.Name, string(peeringToken)); err != nil { - r.updateStatusError(ctx, dialer, err) + r.updateStatusError(ctx, dialer, ConsulAgentError, err) return ctrl.Result{}, err } else { err := r.updateStatus(ctx, dialer, specSecret.ResourceVersion) @@ -165,7 +154,7 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques r.Log.Info("the version annotation was incremented; re-establishing peering with spec.peer.secret", "secret-name", dialer.Secret().Name, "secret-namespace", dialer.Namespace) peeringToken := specSecret.Data[dialer.Secret().Key] if err := r.establishPeering(ctx, dialer.Name, string(peeringToken)); err != nil { - r.updateStatusError(ctx, dialer, err) + r.updateStatusError(ctx, dialer, ConsulAgentError, err) return ctrl.Result{}, err } else { err := r.updateStatus(ctx, dialer, specSecret.ResourceVersion) @@ -177,14 +166,14 @@ func (r *PeeringDialerController) Reconcile(ctx context.Context, req ctrl.Reques r.Log.Info("status.secret exists, but the peering doesn't exist in Consul; establishing peering with the existing spec.peer.secret", "secret-name", dialer.Secret().Name, "secret-namespace", dialer.Namespace) peeringToken := specSecret.Data[dialer.Secret().Key] if err := r.establishPeering(ctx, dialer.Name, string(peeringToken)); err != nil { - r.updateStatusError(ctx, dialer, err) + r.updateStatusError(ctx, dialer, ConsulAgentError, err) return ctrl.Result{}, err } else { err := r.updateStatus(ctx, dialer, specSecret.ResourceVersion) return ctrl.Result{}, err } } else if err != nil { - r.updateStatusError(ctx, dialer, err) + r.updateStatusError(ctx, dialer, InternalError, err) return ctrl.Result{}, err } } @@ -210,11 +199,8 @@ func (r *PeeringDialerController) updateStatus(ctx context.Context, dialer *cons Secret: *dialer.Spec.Peer.Secret, ResourceVersion: resourceVersion, } - dialer.Status.LastReconcileTime = &metav1.Time{Time: time.Now()} - dialer.Status.ReconcileError = &consulv1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(false), - Message: pointerToString(""), - } + dialer.Status.LastSyncedTime = &metav1.Time{Time: time.Now()} + dialer.SetSyncedCondition(corev1.ConditionTrue, "", "") if peeringVersionString, ok := dialer.Annotations[annotationPeeringVersion]; ok { peeringVersion, err := strconv.ParseUint(peeringVersionString, 10, 64) if err != nil { @@ -232,12 +218,8 @@ func (r *PeeringDialerController) updateStatus(ctx context.Context, dialer *cons return err } -func (r *PeeringDialerController) updateStatusError(ctx context.Context, dialer *consulv1alpha1.PeeringDialer, reconcileErr error) { - dialer.Status.ReconcileError = &consulv1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(true), - Message: pointerToString(reconcileErr.Error()), - } - dialer.Status.LastReconcileTime = &metav1.Time{Time: time.Now()} +func (r *PeeringDialerController) updateStatusError(ctx context.Context, dialer *consulv1alpha1.PeeringDialer, reason string, reconcileErr error) { + dialer.SetSyncedCondition(corev1.ConditionFalse, reason, reconcileErr.Error()) err := r.Status().Update(ctx, dialer) if err != nil { r.Log.Error(err, "failed to update PeeringDialer status", "name", dialer.Name, "namespace", dialer.Namespace) diff --git a/control-plane/connect-inject/peering_dialer_controller_test.go b/control-plane/connect-inject/peering_dialer_controller_test.go index bc530cbdce..efeaf784db 100644 --- a/control-plane/connect-inject/peering_dialer_controller_test.go +++ b/control-plane/connect-inject/peering_dialer_controller_test.go @@ -41,24 +41,6 @@ func TestReconcile_CreateUpdatePeeringDialer(t *testing.T) { expectDeletedK8sSecret *types.NamespacedName peeringExists bool }{ - "Errors when Secret is not set on the spec": { - k8sObjects: func() []runtime.Object { - dialer := &v1alpha1.PeeringDialer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "peering", - Namespace: "default", - }, - Spec: v1alpha1.PeeringDialerSpec{ - Peer: &v1alpha1.Peer{ - Secret: nil, - }, - }, - } - return []runtime.Object{dialer} - }, - expErr: "PeeringDialer spec.peer.secret was not set", - peeringSecret: func(_ string) *corev1.Secret { return nil }, - }, "Errors when Secret set on the spec does not exist in the cluster": { k8sObjects: func() []runtime.Object { dialer := &v1alpha1.PeeringDialer{ @@ -845,9 +827,11 @@ func TestDialerUpdateStatus(t *testing.T) { }, ResourceVersion: "1234", }, - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(false), - Message: pointerToString(""), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionTrue, + }, }, }, }, @@ -888,9 +872,11 @@ func TestDialerUpdateStatus(t *testing.T) { }, ResourceVersion: "1234", }, - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(false), - Message: pointerToString(""), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionTrue, + }, }, }, }, @@ -928,7 +914,7 @@ func TestDialerUpdateStatus(t *testing.T) { require.Equal(t, tt.expStatus.SecretRef.Key, dialer.SecretRef().Key) require.Equal(t, tt.expStatus.SecretRef.Backend, dialer.SecretRef().Backend) require.Equal(t, tt.expStatus.SecretRef.ResourceVersion, dialer.SecretRef().ResourceVersion) - require.Equal(t, *tt.expStatus.ReconcileError.Error, *dialer.Status.ReconcileError.Error) + require.Equal(t, tt.expStatus.Conditions[0].Message, dialer.Status.Conditions[0].Message) }) } } @@ -959,9 +945,13 @@ func TestDialerUpdateStatusError(t *testing.T) { }, reconcileErr: errors.New("this is an error"), expStatus: v1alpha1.PeeringDialerStatus{ - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(true), - Message: pointerToString("this is an error"), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionFalse, + Reason: InternalError, + Message: "this is an error", + }, }, }, }, @@ -982,17 +972,23 @@ func TestDialerUpdateStatusError(t *testing.T) { }, }, Status: v1alpha1.PeeringDialerStatus{ - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(false), - Message: pointerToString(""), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionTrue, + }, }, }, }, reconcileErr: errors.New("this is an error"), expStatus: v1alpha1.PeeringDialerStatus{ - ReconcileError: &v1alpha1.ReconcileErrorStatus{ - Error: pointerToBool(true), - Message: pointerToString("this is an error"), + Conditions: v1alpha1.Conditions{ + { + Type: v1alpha1.ConditionSynced, + Status: corev1.ConditionFalse, + Reason: InternalError, + Message: "this is an error", + }, }, }, }, @@ -1016,7 +1012,7 @@ func TestDialerUpdateStatusError(t *testing.T) { Scheme: s, } - controller.updateStatusError(context.Background(), tt.dialer, tt.reconcileErr) + controller.updateStatusError(context.Background(), tt.dialer, InternalError, tt.reconcileErr) dialer := &v1alpha1.PeeringDialer{} dialerName := types.NamespacedName{ @@ -1025,7 +1021,7 @@ func TestDialerUpdateStatusError(t *testing.T) { } err := fakeClient.Get(context.Background(), dialerName, dialer) require.NoError(t, err) - require.Equal(t, *tt.expStatus.ReconcileError.Error, *dialer.Status.ReconcileError.Error) + require.Equal(t, tt.expStatus.Conditions[0].Message, dialer.Status.Conditions[0].Message) }) } From f85ee68ce62f04ca8743a07e1040fea93ec9d413 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Mon, 11 Jul 2022 11:40:28 -0400 Subject: [PATCH 2/2] CHANGELOG --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bd8211e63..85b50e3fc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ FEATURES: * [Experimental] Cluster Peering: * Add support for secret watchers on the Peering Acceptor and Peering Dialer controllers. [[GH-1284](https://github.com/hashicorp/consul-k8s/pull/1284)] * Add support for version annotation on the Peering Acceptor and Peering Dialer controllers. [[GH-1302](https://github.com/hashicorp/consul-k8s/pull/1302)] - * Add validation webhooks for the Peering Acceptor and Peering Dialer CRDs [[GH-1310](https://github.com/hashicorp/consul-k8s/pull/1310)] + * Add validation webhooks for the Peering Acceptor and Peering Dialer CRDs. [[GH-1310](https://github.com/hashicorp/consul-k8s/pull/1310)] + * Add Conditions to the status of the Peering Acceptor and Peering Dialer CRDs. [[GH-1335](https://github.com/hashicorp/consul-k8s/pull/1335)] IMPROVEMENTS: * Control Plane