From 25c1f0d523a6a15652a32502c3f197c44d169948 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 12 Apr 2021 13:53:21 -0700 Subject: [PATCH] Add Conditions to LDAPIdentityProvider's Status and start to fill them - The ldap_upstream_watcher.go controller validates the bind secret and uses the Conditions to report errors. Shares some condition reporting logic with its sibling controller oidc_upstream_watcher.go, to the extent which is convenient without generics in golang. --- .../types_ldapidentityprovider.go.tmpl | 7 + ...or.pinniped.dev_ldapidentityproviders.yaml | 67 ++++++ generated/1.17/README.adoc | 18 +- .../v1alpha1/types_ldapidentityprovider.go | 7 + .../idp/v1alpha1/zz_generated.deepcopy.go | 9 +- ...or.pinniped.dev_ldapidentityproviders.yaml | 67 ++++++ generated/1.18/README.adoc | 18 +- .../v1alpha1/types_ldapidentityprovider.go | 7 + .../idp/v1alpha1/zz_generated.deepcopy.go | 9 +- ...or.pinniped.dev_ldapidentityproviders.yaml | 67 ++++++ generated/1.19/README.adoc | 18 +- .../v1alpha1/types_ldapidentityprovider.go | 7 + .../idp/v1alpha1/zz_generated.deepcopy.go | 9 +- ...or.pinniped.dev_ldapidentityproviders.yaml | 67 ++++++ generated/1.20/README.adoc | 18 +- .../v1alpha1/types_ldapidentityprovider.go | 7 + .../idp/v1alpha1/zz_generated.deepcopy.go | 9 +- ...or.pinniped.dev_ldapidentityproviders.yaml | 67 ++++++ .../v1alpha1/types_ldapidentityprovider.go | 7 + .../idp/v1alpha1/zz_generated.deepcopy.go | 9 +- .../upstreamwatcher/ldap_upstream_watcher.go | 77 ++++++- .../ldap_upstream_watcher_test.go | 196 ++++++++++++++---- .../upstreamwatcher/oidc_upstream_watcher.go | 41 ++-- .../oidc_upstream_watcher_test.go | 6 +- 24 files changed, 732 insertions(+), 82 deletions(-) diff --git a/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go.tmpl b/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go.tmpl index 5e602f31..1550f0c7 100644 --- a/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go.tmpl +++ b/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go.tmpl @@ -26,6 +26,13 @@ type LDAPIdentityProviderStatus struct { // +kubebuilder:default=Pending // +kubebuilder:validation:Enum=Pending;Ready;Error Phase LDAPIdentityProviderPhase `json:"phase,omitempty"` + + // Represents the observations of an identity provider's current state. + // +patchMergeKey=type + // +patchStrategy=merge + // +listType=map + // +listMapKey=type + Conditions []Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` } type LDAPIdentityProviderTLSSpec struct { diff --git a/deploy/supervisor/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/deploy/supervisor/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 1e54d043..db84c861 100644 --- a/deploy/supervisor/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/deploy/supervisor/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -131,6 +131,73 @@ spec: status: description: Status of the identity provider. properties: + conditions: + description: Represents the observations of an identity provider's + current state. + items: + description: Condition status of a resource (mirrored from the metav1.Condition + type added in Kubernetes 1.19). In a future API version we can + switch to using the upstream type. See https://github.com/kubernetes/apimachinery/blob/v0.19.0/pkg/apis/meta/v1/types.go#L1353-L1413. + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition + transitioned from one status to another. This should be when + the underlying condition changed. If that is not known, then + using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: message is a human readable message indicating + details about the transition. This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: observedGeneration represents the .metadata.generation + that the condition was set based upon. For instance, if .metadata.generation + is currently 12, but the .status.conditions[x].observedGeneration + is 9, the condition is out of date with respect to the current + state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: reason contains a programmatic identifier indicating + the reason for the condition's last transition. Producers + of specific condition types may define expected values and + meanings for this field, and whether the values are considered + a guaranteed API. The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + --- Many .condition.type values are consistent across resources + like Available, but because arbitrary conditions can be useful + (see .node.status.conditions), the ability to deconflict is + important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map phase: default: Pending description: Phase summarizes the overall status of the LDAPIdentityProvider. diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index dec89a34..485b3f96 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -669,10 +669,11 @@ Package v1alpha1 is the v1alpha1 version of the Pinniped supervisor identity pro [id="{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-condition"] ==== Condition -Condition status of a resource (mirrored from the metav1.Condition type added in Kubernetes 1.19). In a future API version we can switch to using the upstream type. See https://github.com/kubernetes/apimachinery/blob/v0.19.0/pkg/apis/meta/v1/types.go#L1353-L1413. + .Appears In: **** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-ldapidentityproviderstatus[$$LDAPIdentityProviderStatus$$] - xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-oidcidentityproviderstatus[$$OIDCIdentityProviderStatus$$] **** @@ -688,6 +689,18 @@ Condition status of a resource (mirrored from the metav1.Condition type added in |=== +[id="{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-conditionstatus"] +==== ConditionStatus (string) + + + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] +**** + + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-ldapidentityprovider"] ==== LDAPIdentityProvider @@ -761,6 +774,7 @@ Status of an LDAP identity provider. |=== | Field | Description | *`phase`* __LDAPIdentityProviderPhase__ | Phase summarizes the overall status of the LDAPIdentityProvider. +| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] array__ | Represents the observations of an identity provider's current state. |=== @@ -927,7 +941,7 @@ Status of an OIDC identity provider. |=== | Field | Description | *`phase`* __OIDCIdentityProviderPhase__ | Phase summarizes the overall status of the OIDCIdentityProvider. -| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-condition[$$Condition$$]__ | Represents the observations of an identity provider's current state. +| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] array__ | Represents the observations of an identity provider's current state. |=== diff --git a/generated/1.17/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/1.17/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 5e602f31..1550f0c7 100644 --- a/generated/1.17/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/1.17/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -26,6 +26,13 @@ type LDAPIdentityProviderStatus struct { // +kubebuilder:default=Pending // +kubebuilder:validation:Enum=Pending;Ready;Error Phase LDAPIdentityProviderPhase `json:"phase,omitempty"` + + // Represents the observations of an identity provider's current state. + // +patchMergeKey=type + // +patchStrategy=merge + // +listType=map + // +listMapKey=type + Conditions []Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` } type LDAPIdentityProviderTLSSpec struct { diff --git a/generated/1.17/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go b/generated/1.17/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go index 6ddcebad..1b19762c 100644 --- a/generated/1.17/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.17/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go @@ -34,7 +34,7 @@ func (in *LDAPIdentityProvider) DeepCopyInto(out *LDAPIdentityProvider) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) return } @@ -131,6 +131,13 @@ func (in *LDAPIdentityProviderSpec) DeepCopy() *LDAPIdentityProviderSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LDAPIdentityProviderStatus) DeepCopyInto(out *LDAPIdentityProviderStatus) { *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } return } diff --git a/generated/1.17/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/generated/1.17/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 1e54d043..db84c861 100644 --- a/generated/1.17/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/generated/1.17/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -131,6 +131,73 @@ spec: status: description: Status of the identity provider. properties: + conditions: + description: Represents the observations of an identity provider's + current state. + items: + description: Condition status of a resource (mirrored from the metav1.Condition + type added in Kubernetes 1.19). In a future API version we can + switch to using the upstream type. See https://github.com/kubernetes/apimachinery/blob/v0.19.0/pkg/apis/meta/v1/types.go#L1353-L1413. + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition + transitioned from one status to another. This should be when + the underlying condition changed. If that is not known, then + using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: message is a human readable message indicating + details about the transition. This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: observedGeneration represents the .metadata.generation + that the condition was set based upon. For instance, if .metadata.generation + is currently 12, but the .status.conditions[x].observedGeneration + is 9, the condition is out of date with respect to the current + state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: reason contains a programmatic identifier indicating + the reason for the condition's last transition. Producers + of specific condition types may define expected values and + meanings for this field, and whether the values are considered + a guaranteed API. The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + --- Many .condition.type values are consistent across resources + like Available, but because arbitrary conditions can be useful + (see .node.status.conditions), the ability to deconflict is + important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map phase: default: Pending description: Phase summarizes the overall status of the LDAPIdentityProvider. diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index 6609724e..57bfe7d2 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -669,10 +669,11 @@ Package v1alpha1 is the v1alpha1 version of the Pinniped supervisor identity pro [id="{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-condition"] ==== Condition -Condition status of a resource (mirrored from the metav1.Condition type added in Kubernetes 1.19). In a future API version we can switch to using the upstream type. See https://github.com/kubernetes/apimachinery/blob/v0.19.0/pkg/apis/meta/v1/types.go#L1353-L1413. + .Appears In: **** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-ldapidentityproviderstatus[$$LDAPIdentityProviderStatus$$] - xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-oidcidentityproviderstatus[$$OIDCIdentityProviderStatus$$] **** @@ -688,6 +689,18 @@ Condition status of a resource (mirrored from the metav1.Condition type added in |=== +[id="{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-conditionstatus"] +==== ConditionStatus (string) + + + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] +**** + + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-ldapidentityprovider"] ==== LDAPIdentityProvider @@ -761,6 +774,7 @@ Status of an LDAP identity provider. |=== | Field | Description | *`phase`* __LDAPIdentityProviderPhase__ | Phase summarizes the overall status of the LDAPIdentityProvider. +| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] array__ | Represents the observations of an identity provider's current state. |=== @@ -927,7 +941,7 @@ Status of an OIDC identity provider. |=== | Field | Description | *`phase`* __OIDCIdentityProviderPhase__ | Phase summarizes the overall status of the OIDCIdentityProvider. -| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-condition[$$Condition$$]__ | Represents the observations of an identity provider's current state. +| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] array__ | Represents the observations of an identity provider's current state. |=== diff --git a/generated/1.18/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/1.18/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 5e602f31..1550f0c7 100644 --- a/generated/1.18/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/1.18/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -26,6 +26,13 @@ type LDAPIdentityProviderStatus struct { // +kubebuilder:default=Pending // +kubebuilder:validation:Enum=Pending;Ready;Error Phase LDAPIdentityProviderPhase `json:"phase,omitempty"` + + // Represents the observations of an identity provider's current state. + // +patchMergeKey=type + // +patchStrategy=merge + // +listType=map + // +listMapKey=type + Conditions []Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` } type LDAPIdentityProviderTLSSpec struct { diff --git a/generated/1.18/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go b/generated/1.18/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go index 6ddcebad..1b19762c 100644 --- a/generated/1.18/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.18/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go @@ -34,7 +34,7 @@ func (in *LDAPIdentityProvider) DeepCopyInto(out *LDAPIdentityProvider) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) return } @@ -131,6 +131,13 @@ func (in *LDAPIdentityProviderSpec) DeepCopy() *LDAPIdentityProviderSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LDAPIdentityProviderStatus) DeepCopyInto(out *LDAPIdentityProviderStatus) { *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } return } diff --git a/generated/1.18/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/generated/1.18/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 1e54d043..db84c861 100644 --- a/generated/1.18/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/generated/1.18/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -131,6 +131,73 @@ spec: status: description: Status of the identity provider. properties: + conditions: + description: Represents the observations of an identity provider's + current state. + items: + description: Condition status of a resource (mirrored from the metav1.Condition + type added in Kubernetes 1.19). In a future API version we can + switch to using the upstream type. See https://github.com/kubernetes/apimachinery/blob/v0.19.0/pkg/apis/meta/v1/types.go#L1353-L1413. + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition + transitioned from one status to another. This should be when + the underlying condition changed. If that is not known, then + using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: message is a human readable message indicating + details about the transition. This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: observedGeneration represents the .metadata.generation + that the condition was set based upon. For instance, if .metadata.generation + is currently 12, but the .status.conditions[x].observedGeneration + is 9, the condition is out of date with respect to the current + state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: reason contains a programmatic identifier indicating + the reason for the condition's last transition. Producers + of specific condition types may define expected values and + meanings for this field, and whether the values are considered + a guaranteed API. The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + --- Many .condition.type values are consistent across resources + like Available, but because arbitrary conditions can be useful + (see .node.status.conditions), the ability to deconflict is + important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map phase: default: Pending description: Phase summarizes the overall status of the LDAPIdentityProvider. diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index e9c1538e..db1c1590 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -669,10 +669,11 @@ Package v1alpha1 is the v1alpha1 version of the Pinniped supervisor identity pro [id="{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-condition"] ==== Condition -Condition status of a resource (mirrored from the metav1.Condition type added in Kubernetes 1.19). In a future API version we can switch to using the upstream type. See https://github.com/kubernetes/apimachinery/blob/v0.19.0/pkg/apis/meta/v1/types.go#L1353-L1413. + .Appears In: **** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-ldapidentityproviderstatus[$$LDAPIdentityProviderStatus$$] - xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-oidcidentityproviderstatus[$$OIDCIdentityProviderStatus$$] **** @@ -688,6 +689,18 @@ Condition status of a resource (mirrored from the metav1.Condition type added in |=== +[id="{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-conditionstatus"] +==== ConditionStatus (string) + + + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] +**** + + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-ldapidentityprovider"] ==== LDAPIdentityProvider @@ -761,6 +774,7 @@ Status of an LDAP identity provider. |=== | Field | Description | *`phase`* __LDAPIdentityProviderPhase__ | Phase summarizes the overall status of the LDAPIdentityProvider. +| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] array__ | Represents the observations of an identity provider's current state. |=== @@ -927,7 +941,7 @@ Status of an OIDC identity provider. |=== | Field | Description | *`phase`* __OIDCIdentityProviderPhase__ | Phase summarizes the overall status of the OIDCIdentityProvider. -| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-condition[$$Condition$$]__ | Represents the observations of an identity provider's current state. +| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] array__ | Represents the observations of an identity provider's current state. |=== diff --git a/generated/1.19/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/1.19/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 5e602f31..1550f0c7 100644 --- a/generated/1.19/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/1.19/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -26,6 +26,13 @@ type LDAPIdentityProviderStatus struct { // +kubebuilder:default=Pending // +kubebuilder:validation:Enum=Pending;Ready;Error Phase LDAPIdentityProviderPhase `json:"phase,omitempty"` + + // Represents the observations of an identity provider's current state. + // +patchMergeKey=type + // +patchStrategy=merge + // +listType=map + // +listMapKey=type + Conditions []Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` } type LDAPIdentityProviderTLSSpec struct { diff --git a/generated/1.19/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go b/generated/1.19/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go index 6ddcebad..1b19762c 100644 --- a/generated/1.19/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.19/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go @@ -34,7 +34,7 @@ func (in *LDAPIdentityProvider) DeepCopyInto(out *LDAPIdentityProvider) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) return } @@ -131,6 +131,13 @@ func (in *LDAPIdentityProviderSpec) DeepCopy() *LDAPIdentityProviderSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LDAPIdentityProviderStatus) DeepCopyInto(out *LDAPIdentityProviderStatus) { *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } return } diff --git a/generated/1.19/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/generated/1.19/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 1e54d043..db84c861 100644 --- a/generated/1.19/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/generated/1.19/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -131,6 +131,73 @@ spec: status: description: Status of the identity provider. properties: + conditions: + description: Represents the observations of an identity provider's + current state. + items: + description: Condition status of a resource (mirrored from the metav1.Condition + type added in Kubernetes 1.19). In a future API version we can + switch to using the upstream type. See https://github.com/kubernetes/apimachinery/blob/v0.19.0/pkg/apis/meta/v1/types.go#L1353-L1413. + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition + transitioned from one status to another. This should be when + the underlying condition changed. If that is not known, then + using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: message is a human readable message indicating + details about the transition. This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: observedGeneration represents the .metadata.generation + that the condition was set based upon. For instance, if .metadata.generation + is currently 12, but the .status.conditions[x].observedGeneration + is 9, the condition is out of date with respect to the current + state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: reason contains a programmatic identifier indicating + the reason for the condition's last transition. Producers + of specific condition types may define expected values and + meanings for this field, and whether the values are considered + a guaranteed API. The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + --- Many .condition.type values are consistent across resources + like Available, but because arbitrary conditions can be useful + (see .node.status.conditions), the ability to deconflict is + important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map phase: default: Pending description: Phase summarizes the overall status of the LDAPIdentityProvider. diff --git a/generated/1.20/README.adoc b/generated/1.20/README.adoc index f49bf630..25cfd6ff 100644 --- a/generated/1.20/README.adoc +++ b/generated/1.20/README.adoc @@ -669,10 +669,11 @@ Package v1alpha1 is the v1alpha1 version of the Pinniped supervisor identity pro [id="{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-condition"] ==== Condition -Condition status of a resource (mirrored from the metav1.Condition type added in Kubernetes 1.19). In a future API version we can switch to using the upstream type. See https://github.com/kubernetes/apimachinery/blob/v0.19.0/pkg/apis/meta/v1/types.go#L1353-L1413. + .Appears In: **** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-ldapidentityproviderstatus[$$LDAPIdentityProviderStatus$$] - xref:{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-oidcidentityproviderstatus[$$OIDCIdentityProviderStatus$$] **** @@ -688,6 +689,18 @@ Condition status of a resource (mirrored from the metav1.Condition type added in |=== +[id="{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-conditionstatus"] +==== ConditionStatus (string) + + + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] +**** + + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-ldapidentityprovider"] ==== LDAPIdentityProvider @@ -761,6 +774,7 @@ Status of an LDAP identity provider. |=== | Field | Description | *`phase`* __LDAPIdentityProviderPhase__ | Phase summarizes the overall status of the LDAPIdentityProvider. +| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] array__ | Represents the observations of an identity provider's current state. |=== @@ -927,7 +941,7 @@ Status of an OIDC identity provider. |=== | Field | Description | *`phase`* __OIDCIdentityProviderPhase__ | Phase summarizes the overall status of the OIDCIdentityProvider. -| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-condition[$$Condition$$]__ | Represents the observations of an identity provider's current state. +| *`conditions`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-20-apis-supervisor-idp-v1alpha1-condition[$$Condition$$] array__ | Represents the observations of an identity provider's current state. |=== diff --git a/generated/1.20/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/1.20/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 5e602f31..1550f0c7 100644 --- a/generated/1.20/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/1.20/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -26,6 +26,13 @@ type LDAPIdentityProviderStatus struct { // +kubebuilder:default=Pending // +kubebuilder:validation:Enum=Pending;Ready;Error Phase LDAPIdentityProviderPhase `json:"phase,omitempty"` + + // Represents the observations of an identity provider's current state. + // +patchMergeKey=type + // +patchStrategy=merge + // +listType=map + // +listMapKey=type + Conditions []Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` } type LDAPIdentityProviderTLSSpec struct { diff --git a/generated/1.20/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go b/generated/1.20/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go index 6ddcebad..1b19762c 100644 --- a/generated/1.20/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.20/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go @@ -34,7 +34,7 @@ func (in *LDAPIdentityProvider) DeepCopyInto(out *LDAPIdentityProvider) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) return } @@ -131,6 +131,13 @@ func (in *LDAPIdentityProviderSpec) DeepCopy() *LDAPIdentityProviderSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LDAPIdentityProviderStatus) DeepCopyInto(out *LDAPIdentityProviderStatus) { *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } return } diff --git a/generated/1.20/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml b/generated/1.20/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml index 1e54d043..db84c861 100644 --- a/generated/1.20/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml +++ b/generated/1.20/crds/idp.supervisor.pinniped.dev_ldapidentityproviders.yaml @@ -131,6 +131,73 @@ spec: status: description: Status of the identity provider. properties: + conditions: + description: Represents the observations of an identity provider's + current state. + items: + description: Condition status of a resource (mirrored from the metav1.Condition + type added in Kubernetes 1.19). In a future API version we can + switch to using the upstream type. See https://github.com/kubernetes/apimachinery/blob/v0.19.0/pkg/apis/meta/v1/types.go#L1353-L1413. + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition + transitioned from one status to another. This should be when + the underlying condition changed. If that is not known, then + using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: message is a human readable message indicating + details about the transition. This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: observedGeneration represents the .metadata.generation + that the condition was set based upon. For instance, if .metadata.generation + is currently 12, but the .status.conditions[x].observedGeneration + is 9, the condition is out of date with respect to the current + state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: reason contains a programmatic identifier indicating + the reason for the condition's last transition. Producers + of specific condition types may define expected values and + meanings for this field, and whether the values are considered + a guaranteed API. The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + --- Many .condition.type values are consistent across resources + like Available, but because arbitrary conditions can be useful + (see .node.status.conditions), the ability to deconflict is + important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map phase: default: Pending description: Phase summarizes the overall status of the LDAPIdentityProvider. diff --git a/generated/latest/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go b/generated/latest/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go index 5e602f31..1550f0c7 100644 --- a/generated/latest/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go +++ b/generated/latest/apis/supervisor/idp/v1alpha1/types_ldapidentityprovider.go @@ -26,6 +26,13 @@ type LDAPIdentityProviderStatus struct { // +kubebuilder:default=Pending // +kubebuilder:validation:Enum=Pending;Ready;Error Phase LDAPIdentityProviderPhase `json:"phase,omitempty"` + + // Represents the observations of an identity provider's current state. + // +patchMergeKey=type + // +patchStrategy=merge + // +listType=map + // +listMapKey=type + Conditions []Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` } type LDAPIdentityProviderTLSSpec struct { diff --git a/generated/latest/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go b/generated/latest/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go index 6ddcebad..1b19762c 100644 --- a/generated/latest/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go +++ b/generated/latest/apis/supervisor/idp/v1alpha1/zz_generated.deepcopy.go @@ -34,7 +34,7 @@ func (in *LDAPIdentityProvider) DeepCopyInto(out *LDAPIdentityProvider) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) return } @@ -131,6 +131,13 @@ func (in *LDAPIdentityProviderSpec) DeepCopy() *LDAPIdentityProviderSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LDAPIdentityProviderStatus) DeepCopyInto(out *LDAPIdentityProviderStatus) { *out = *in + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } return } diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go index 207d7a79..ae214c23 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go @@ -4,11 +4,15 @@ package upstreamwatcher import ( + "context" "fmt" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" corev1informers "k8s.io/client-go/informers/core/v1" + "k8s.io/klog/v2/klogr" "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" pinnipedclientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned" @@ -22,6 +26,9 @@ import ( const ( ldapControllerName = "ldap-upstream-observer" ldapBindAccountSecretType = corev1.SecretTypeBasicAuth + + // Constants related to conditions. + typeBindSecretValid = "BindSecretValid" ) // UpstreamLDAPIdentityProviderICache is a thread safe cache that holds a list of validated upstream LDAP IDP configurations. @@ -78,7 +85,7 @@ func (c *ldapWatcherController) Sync(ctx controllerlib.Context) error { requeue := false validatedUpstreams := make([]provider.UpstreamLDAPIdentityProviderI, 0, len(actualUpstreams)) for _, upstream := range actualUpstreams { - valid := c.validateUpstream(upstream) + valid := c.validateUpstream(ctx.Context, upstream) if valid == nil { requeue = true } else { @@ -92,7 +99,7 @@ func (c *ldapWatcherController) Sync(ctx controllerlib.Context) error { return nil } -func (c *ldapWatcherController) validateUpstream(upstream *v1alpha1.LDAPIdentityProvider) provider.UpstreamLDAPIdentityProviderI { +func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider) provider.UpstreamLDAPIdentityProviderI { spec := upstream.Spec result := &upstreamldap.Provider{ Name: upstream.Name, @@ -106,7 +113,13 @@ func (c *ldapWatcherController) validateUpstream(upstream *v1alpha1.LDAPIdentity }, Dialer: c.ldapDialer, } - _ = c.validateSecret(upstream, result) + conditions := []*v1alpha1.Condition{ + c.validateSecret(upstream, result), + } + hadErrorCondition := c.updateStatus(ctx, upstream, conditions) + if hadErrorCondition { + return nil + } return result } @@ -115,22 +128,64 @@ func (c ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityPro secret, err := c.secretInformer.Lister().Secrets(upstream.Namespace).Get(secretName) if err != nil { - // TODO - return nil + return &v1alpha1.Condition{ + Type: typeBindSecretValid, + Status: v1alpha1.ConditionFalse, + Reason: reasonNotFound, + Message: err.Error(), + } } if secret.Type != corev1.SecretTypeBasicAuth { - // TODO - return nil + return &v1alpha1.Condition{ + Type: typeBindSecretValid, + Status: v1alpha1.ConditionFalse, + Reason: reasonWrongType, + Message: fmt.Sprintf("referenced Secret %q has wrong type %q (should be %q)", secretName, secret.Type, corev1.SecretTypeBasicAuth), + } } result.BindUsername = string(secret.Data[corev1.BasicAuthUsernameKey]) result.BindPassword = string(secret.Data[corev1.BasicAuthPasswordKey]) if len(result.BindUsername) == 0 || len(result.BindPassword) == 0 { - // TODO - return nil + return &v1alpha1.Condition{ + Type: typeBindSecretValid, + Status: v1alpha1.ConditionFalse, + Reason: reasonMissingKeys, + Message: fmt.Sprintf("referenced Secret %q is missing required keys %q", secretName, []string{corev1.BasicAuthUsernameKey, corev1.BasicAuthPasswordKey}), + } } - var cond *v1alpha1.Condition // satisfy linter - return cond + return &v1alpha1.Condition{ + Type: typeBindSecretValid, + Status: v1alpha1.ConditionTrue, + Reason: reasonSuccess, + Message: "loaded bind secret", + } +} + +func (c *ldapWatcherController) updateStatus(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, conditions []*v1alpha1.Condition) bool { + log := klogr.New().WithValues("namespace", upstream.Namespace, "name", upstream.Name) + updated := upstream.DeepCopy() + + hadErrorCondition := mergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log) + + updated.Status.Phase = v1alpha1.LDAPPhaseReady + if hadErrorCondition { + updated.Status.Phase = v1alpha1.LDAPPhaseError + } + + if equality.Semantic.DeepEqual(upstream, updated) { + return hadErrorCondition + } + + _, err := c.client. + IDPV1alpha1(). + LDAPIdentityProviders(upstream.Namespace). + UpdateStatus(ctx, updated, metav1.UpdateOptions{}) + if err != nil { + log.Error(err, "failed to update status") + } + + return hadErrorCondition } diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go index e7f9f503..79951433 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go @@ -5,7 +5,9 @@ package upstreamwatcher import ( "context" + "fmt" "testing" + "time" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -138,11 +140,12 @@ func (d *comparableDialer) Dial(ctx context.Context, hostAndPort string) (upstre func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { t.Parallel() + now := metav1.NewTime(time.Now().UTC()) const ( testNamespace = "test-namespace" testName = "test-name" - testSecretName = "test-client-secret" + testSecretName = "test-bind-secret" testBindUsername = "test-bind-username" testBindPassword = "test-bind-password" testHost = "ldap.example.com:123" @@ -163,6 +166,38 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, } + validUpstream := &v1alpha1.LDAPIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Name: testName, Namespace: testNamespace, Generation: 1234}, + Spec: v1alpha1.LDAPIdentityProviderSpec{ + Host: testHost, + TLS: &v1alpha1.LDAPIdentityProviderTLSSpec{CertificateAuthorityData: testCABundle}, + Bind: v1alpha1.LDAPIdentityProviderBindSpec{SecretName: testSecretName}, + UserSearch: v1alpha1.LDAPIdentityProviderUserSearchSpec{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + Attributes: v1alpha1.LDAPIdentityProviderUserSearchAttributesSpec{ + Username: testUsernameAttrName, + UniqueID: testUIDAttrName, + }, + }, + }, + } + + providerForValidUpstream := &upstreamldap.Provider{ + Name: testName, + Host: testHost, + CABundle: []byte(testCABundle), + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: &upstreamldap.UserSearch{ + Base: testUserSearchBase, + Filter: testUserSearchFilter, + UsernameAttribute: testUsernameAttrName, + UIDAttribute: testUIDAttrName, + }, + Dialer: successfulDialer, // the dialer passed to the controller's constructor should have been passed through + } + tests := []struct { name string inputUpstreams []runtime.Object @@ -176,50 +211,108 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { name: "no LDAPIdentityProvider upstreams clears the cache", }, { - name: "one valid upstream updates the cache to include only that upstream", - ldapDialer: successfulDialer, - inputUpstreams: []runtime.Object{&v1alpha1.LDAPIdentityProvider{ - ObjectMeta: metav1.ObjectMeta{Name: testName, Namespace: testNamespace, Generation: 1234}, - Spec: v1alpha1.LDAPIdentityProviderSpec{ - Host: testHost, - TLS: &v1alpha1.LDAPIdentityProviderTLSSpec{CertificateAuthorityData: testCABundle}, - Bind: v1alpha1.LDAPIdentityProviderBindSpec{SecretName: testSecretName}, - UserSearch: v1alpha1.LDAPIdentityProviderUserSearchSpec{ - Base: testUserSearchBase, - Filter: testUserSearchFilter, - Attributes: v1alpha1.LDAPIdentityProviderUserSearchAttributesSpec{ - Username: testUsernameAttrName, - UniqueID: testUIDAttrName, - }, - }, - }, - }}, + name: "one valid upstream updates the cache to include only that upstream", + ldapDialer: successfulDialer, + inputUpstreams: []runtime.Object{validUpstream}, inputSecrets: []runtime.Object{&corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, Type: corev1.SecretTypeBasicAuth, Data: testValidSecretData, }}, - wantResultingCache: []*upstreamldap.Provider{ - { - Name: testName, - Host: testHost, - CABundle: []byte(testCABundle), - BindUsername: testBindUsername, - BindPassword: testBindPassword, - UserSearch: &upstreamldap.UserSearch{ - Base: testUserSearchBase, - Filter: testUserSearchFilter, - UsernameAttribute: testUsernameAttrName, - UIDAttribute: testUIDAttrName, - }, - Dialer: successfulDialer, // the dialer passed to the controller's constructor should have been passed through - }, - }, + wantResultingCache: []*upstreamldap.Provider{providerForValidUpstream}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ Phase: "Ready", - // TODO Conditions + Conditions: []v1alpha1.Condition{ + { + Type: "BindSecretValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded bind secret", + ObservedGeneration: 1234, + }, + }, + }, + }}, + }, + { + name: "missing secret", + ldapDialer: successfulDialer, + inputUpstreams: []runtime.Object{validUpstream}, + inputSecrets: []runtime.Object{}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingCache: []*upstreamldap.Provider{}, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + { + Type: "BindSecretValid", + Status: "False", + LastTransitionTime: now, + Reason: "SecretNotFound", + Message: fmt.Sprintf(`secret "%s" not found`, testSecretName), + ObservedGeneration: 1234, + }, + }, + }, + }}, + }, + { + name: "secret has wrong type", + ldapDialer: successfulDialer, + inputUpstreams: []runtime.Object{validUpstream}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, + Type: "some-other-type", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingCache: []*upstreamldap.Provider{}, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + { + Type: "BindSecretValid", + Status: "False", + LastTransitionTime: now, + Reason: "SecretWrongType", + Message: fmt.Sprintf(`referenced Secret "%s" has wrong type "some-other-type" (should be "kubernetes.io/basic-auth")`, testSecretName), + ObservedGeneration: 1234, + }, + }, + }, + }}, + }, + { + name: "secret is missing key", + ldapDialer: successfulDialer, + inputUpstreams: []runtime.Object{validUpstream}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, + Type: corev1.SecretTypeBasicAuth, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantResultingCache: []*upstreamldap.Provider{}, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + { + Type: "BindSecretValid", + Status: "False", + LastTransitionTime: now, + Reason: "SecretMissingKeys", + Message: fmt.Sprintf(`referenced Secret "%s" is missing required keys ["username" "password"]`, testSecretName), + ObservedGeneration: 1234, + }, + }, }, }}, }, @@ -271,9 +364,13 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().LDAPIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{}) require.NoError(t, err) - // TODO maybe use something like the normalizeUpstreams() helper to make assertions about what was updated - _ = actualUpstreams - // require.ElementsMatch(t, tt.wantResultingUpstreams, actualUpstreams.Items) + // Assert on the expected Status of the upstreams. Preprocess the upstreams a bit so that they're easier to assert against. + normalizedActualUpstreams := normalizeLDAPUpstreams(actualUpstreams.Items, now) + require.Equal(t, len(tt.wantResultingUpstreams), len(normalizedActualUpstreams)) + for i := range tt.wantResultingUpstreams { + // Require each separately to get a nice diff when the test fails. + require.Equal(t, tt.wantResultingUpstreams[i], normalizedActualUpstreams[i]) + } // Running the sync() a second time should be idempotent, and should return the same error. if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantErr != "" { @@ -284,3 +381,24 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }) } } + +func normalizeLDAPUpstreams(upstreams []v1alpha1.LDAPIdentityProvider, now metav1.Time) []v1alpha1.LDAPIdentityProvider { + result := make([]v1alpha1.LDAPIdentityProvider, 0, len(upstreams)) + for _, u := range upstreams { + normalized := u.DeepCopy() + + // We're only interested in comparing the status, so zero out the spec. + normalized.Spec = v1alpha1.LDAPIdentityProviderSpec{} + + // Round down the LastTransitionTime values to `now` if they were just updated. This makes + // it much easier to encode assertions about the expected timestamps. + for i := range normalized.Status.Conditions { + if time.Since(normalized.Status.Conditions[i].LastTransitionTime.Time) < 5*time.Second { + normalized.Status.Conditions[i].LastTransitionTime = now + } + } + result = append(result, *normalized) + } + + return result +} diff --git a/internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher.go index cbf2690f..7fa0d541 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher.go @@ -338,24 +338,13 @@ func (c *oidcWatcherController) updateStatus(ctx context.Context, upstream *v1al log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name) updated := upstream.DeepCopy() + hadErrorCondition := mergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log) + updated.Status.Phase = v1alpha1.PhaseReady - - for i := range conditions { - cond := conditions[i].DeepCopy() - cond.LastTransitionTime = metav1.Now() - cond.ObservedGeneration = upstream.Generation - if c.mergeCondition(&updated.Status.Conditions, cond) { - log.Info("updated condition", "type", cond.Type, "status", cond.Status, "reason", cond.Reason, "message", cond.Message) - } - if cond.Status == v1alpha1.ConditionFalse { - updated.Status.Phase = v1alpha1.PhaseError - } + if hadErrorCondition { + updated.Status.Phase = v1alpha1.PhaseError } - sort.SliceStable(updated.Status.Conditions, func(i, j int) bool { - return updated.Status.Conditions[i].Type < updated.Status.Conditions[j].Type - }) - if equality.Semantic.DeepEqual(upstream, updated) { return } @@ -369,9 +358,29 @@ func (c *oidcWatcherController) updateStatus(ctx context.Context, upstream *v1al } } +// mergeConditions merges conditions into conditionsToUpdate. If returns true if it merged any error conditions. +func mergeConditions(conditions []*v1alpha1.Condition, observedGeneration int64, conditionsToUpdate *[]v1alpha1.Condition, log logr.Logger) bool { + hadErrorCondition := false + for i := range conditions { + cond := conditions[i].DeepCopy() + cond.LastTransitionTime = metav1.Now() + cond.ObservedGeneration = observedGeneration + if mergeCondition(conditionsToUpdate, cond) { + log.Info("updated condition", "type", cond.Type, "status", cond.Status, "reason", cond.Reason, "message", cond.Message) + } + if cond.Status == v1alpha1.ConditionFalse { + hadErrorCondition = true + } + } + sort.SliceStable(*conditionsToUpdate, func(i, j int) bool { + return (*conditionsToUpdate)[i].Type < (*conditionsToUpdate)[j].Type + }) + return hadErrorCondition +} + // mergeCondition merges a new v1alpha1.Condition into a slice of existing conditions. It returns true // if the condition has meaningfully changed. -func (*oidcWatcherController) mergeCondition(existing *[]v1alpha1.Condition, new *v1alpha1.Condition) bool { +func mergeCondition(existing *[]v1alpha1.Condition, new *v1alpha1.Condition) bool { // Find any existing condition with a matching type. var old *v1alpha1.Condition for i := range *existing { diff --git a/internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher_test.go index 35ffcd99..44b2a57e 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher_test.go @@ -655,8 +655,8 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().OIDCIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{}) require.NoError(t, err) - // Preprocess the set of upstreams a bit so that they're easier to assert against. - require.ElementsMatch(t, tt.wantResultingUpstreams, normalizeUpstreams(actualUpstreams.Items, now)) + // Assert on the expected Status of the upstreams. Preprocess the upstreams a bit so that they're easier to assert against. + require.ElementsMatch(t, tt.wantResultingUpstreams, normalizeOIDCUpstreams(actualUpstreams.Items, now)) // Running the sync() a second time should be idempotent except for logs, and should return the same error. // This also helps exercise code paths where the OIDC provider discovery hits cache. @@ -669,7 +669,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { } } -func normalizeUpstreams(upstreams []v1alpha1.OIDCIdentityProvider, now metav1.Time) []v1alpha1.OIDCIdentityProvider { +func normalizeOIDCUpstreams(upstreams []v1alpha1.OIDCIdentityProvider, now metav1.Time) []v1alpha1.OIDCIdentityProvider { result := make([]v1alpha1.OIDCIdentityProvider, 0, len(upstreams)) for _, u := range upstreams { normalized := u.DeepCopy()