diff --git a/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go.tmpl b/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go.tmpl index 1a36a441b..c84f46dbd 100644 --- a/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go.tmpl +++ b/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go.tmpl @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/deploy/supervisor/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/deploy/supervisor/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 40a300718..b1d762384 100644 --- a/deploy/supervisor/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/deploy/supervisor/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/1.21/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/1.21/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/1.21/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/1.21/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/generated/1.21/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/generated/1.21/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 21ab3e8ff..6ae17a03d 100644 --- a/generated/1.21/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/generated/1.21/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/1.22/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/1.22/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/1.22/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/1.22/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/generated/1.22/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/generated/1.22/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 21ab3e8ff..6ae17a03d 100644 --- a/generated/1.22/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/generated/1.22/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/1.23/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/1.23/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/1.23/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/1.23/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/generated/1.23/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/generated/1.23/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 40a300718..b1d762384 100644 --- a/generated/1.23/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/generated/1.23/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/1.24/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/1.24/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/1.24/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/1.24/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/generated/1.24/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/generated/1.24/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 40a300718..b1d762384 100644 --- a/generated/1.24/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/generated/1.24/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/1.25/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/1.25/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/1.25/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/1.25/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/generated/1.25/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/generated/1.25/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 40a300718..b1d762384 100644 --- a/generated/1.25/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/generated/1.25/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/1.26/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/1.26/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/1.26/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/1.26/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/generated/1.26/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/generated/1.26/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 40a300718..b1d762384 100644 --- a/generated/1.26/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/generated/1.26/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/1.27/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/1.27/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/1.27/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/1.27/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/generated/1.27/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/generated/1.27/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 40a300718..b1d762384 100644 --- a/generated/1.27/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/generated/1.27/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/1.28/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/1.28/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/1.28/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/1.28/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/generated/1.28/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/generated/1.28/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 40a300718..b1d762384 100644 --- a/generated/1.28/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/generated/1.28/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/1.29/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/1.29/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/1.29/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/1.29/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/generated/1.29/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml b/generated/1.29/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml index 40a300718..b1d762384 100644 --- a/generated/1.29/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml +++ b/generated/1.29/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml @@ -114,6 +114,7 @@ spec: - organizations type: object claims: + default: {} description: Claims allows customization of the username and groups claims. properties: diff --git a/generated/latest/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go b/generated/latest/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go index 1a36a441b..c84f46dbd 100644 --- a/generated/latest/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go +++ b/generated/latest/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go @@ -211,7 +211,7 @@ type GitHubIdentityProviderSpec struct { // Claims allows customization of the username and groups claims. // - // +optional + // +kubebuilder:default={} Claims GitHubClaims `json:"claims,omitempty"` // AllowAuthentication allows customization of who can authenticate using this IDP and how. diff --git a/internal/controller/conditionsutil/conditions_util_test.go b/internal/controller/conditionsutil/conditions_util_test.go index 2049e9f2b..373d6adee 100644 --- a/internal/controller/conditionsutil/conditions_util_test.go +++ b/internal/controller/conditionsutil/conditions_util_test.go @@ -30,7 +30,34 @@ func TestMergeIDPConditions(t *testing.T) { wantConditions []metav1.Condition }{ { - name: "True -> False returns true", + name: "Adding a new condition with status=True returns false", + newConditions: []*metav1.Condition{ + { + Type: "NewType", + Status: metav1.ConditionTrue, + Reason: "new reason", + Message: "new message", + }, + }, + observedGeneration: int64(999), + conditionsToUpdate: &[]metav1.Condition{}, + wantLogSnippets: []string{ + `"message":"updated condition","type":"NewType","status":"True"`, + }, + wantConditions: []metav1.Condition{ + { + Type: "NewType", + Status: metav1.ConditionTrue, + ObservedGeneration: int64(999), + LastTransitionTime: testTime, + Reason: "new reason", + Message: "new message", + }, + }, + wantResult: false, + }, + { + name: "Updating a condition status from False to True returns true", newConditions: []*metav1.Condition{ { Type: "UnchangedType", @@ -144,7 +171,13 @@ func TestMergeIDPConditions(t *testing.T) { var log bytes.Buffer logger := plog.TestLogger(t, &log) - result := MergeConditions(tt.newConditions, tt.observedGeneration, tt.conditionsToUpdate, logger, testTime) + result := MergeConditions( + tt.newConditions, + tt.observedGeneration, + tt.conditionsToUpdate, + logger, + testTime, + ) logString := log.String() require.Equal(t, len(tt.wantLogSnippets), strings.Count(logString, "\n")) diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index 3baa898f3..e6897e323 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -36,6 +36,7 @@ import ( "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/endpointaddr" "go.pinniped.dev/internal/federationdomain/upstreamprovider" + "go.pinniped.dev/internal/net/phttp" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/upstreamgithub" ) @@ -47,6 +48,8 @@ const ( gitHubClientSecretType corev1.SecretType = "secrets.pinniped.dev/github-client" clientIDDataKey, clientSecretDataKey string = "clientID", "clientSecret" + countExpectedConditions = 6 + HostValid string = "HostValid" TLSConfigurationValid string = "TLSConfigurationValid" OrganizationsPolicyValid string = "OrganizationsPolicyValid" @@ -54,6 +57,7 @@ const ( // have been obtained. The controller has no way to verify whether they are valid. ClientCredentialsObtained string = "ClientCredentialsObtained" //nolint:gosec // this is not a credential GitHubConnectionValid string = "GitHubConnectionValid" + ClaimsValid string = "ClaimsValid" ) // UpstreamGitHubIdentityProviderICache is a thread safe cache that holds a list of validated upstream GitHub IDP configurations. @@ -68,7 +72,6 @@ type gitHubWatcherController struct { client supervisorclientset.Interface gitHubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer secretInformer corev1informers.SecretInformer - httpClientBuilder func(rootCAs *x509.CertPool) *http.Client clock clock.Clock } @@ -82,7 +85,6 @@ func New( log plog.Logger, withInformer pinnipedcontroller.WithInformerOptionFunc, clock clock.Clock, - httpClientBuilder func(rootCAs *x509.CertPool) *http.Client, ) controllerlib.Controller { c := gitHubWatcherController{ namespace: namespace, @@ -91,7 +93,6 @@ func New( log: log.WithName(controllerName), gitHubIdentityProviderInformer: gitHubIdentityProviderInformer, secretInformer: secretInformer, - httpClientBuilder: httpClientBuilder, clock: clock, } @@ -252,10 +253,8 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contro // Should there be some sort of catch-all condition to capture this? // This does not actually prevent a GitHub IDP from being added to the cache. // CRD defaulting and validation should eliminate the possibility of an error here. - groupNameAttribute, usernameAttribute, userAndGroupErr := validateUserAndGroupAttributes(upstream) - if userAndGroupErr != nil { - applicationErrors = append(applicationErrors, userAndGroupErr) - } + userAndGroupCondition, groupNameAttribute, usernameAttribute := validateUserAndGroupAttributes(upstream) + conditions = append(conditions, userAndGroupCondition) organizationPolicyCondition, policy := validateOrganizationsPolicy(&upstream.Spec.AllowAuthentication.Organizations) conditions = append(conditions, organizationPolicyCondition) @@ -279,12 +278,12 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contro // The critical pattern to maintain is that every run of the sync loop will populate the exact number of the exact // same set of conditions. Conditions depending on other conditions should get Status: metav1.ConditionUnknown, or // Status: metav1.ConditionFalse, never be omitted. - if len(conditions) != 5 { // untested since all code paths result in 5 conditions - applicationErrors = append(applicationErrors, fmt.Errorf("expected %d conditions but found %d conditions", 5, len(conditions))) + if len(conditions) != countExpectedConditions { // untested since all code paths return the same number of conditions + applicationErrors = append(applicationErrors, fmt.Errorf("expected %d conditions but found %d conditions", countExpectedConditions, len(conditions))) return nil, k8sutilerrors.NewAggregate(applicationErrors) } hadErrorCondition, updateStatusErr := c.updateStatus(ctx.Context, upstream, conditions) - if updateStatusErr != nil { // untested + if updateStatusErr != nil { applicationErrors = append(applicationErrors, updateStatusErr) } // Any error condition means we will not add the IDP to the cache, so just return nil here @@ -390,7 +389,7 @@ func (c *gitHubWatcherController) validateGitHubConnection( Status: metav1.ConditionTrue, Reason: upstreamwatchers.ReasonSuccess, Message: fmt.Sprintf("spec.githubAPI.host (%q) is reachable and TLS verification succeeds", hostPort.Endpoint()), - }, fmt.Sprintf("https://%s", hostPort.Endpoint()), c.httpClientBuilder(certPool), conn.Close() + }, fmt.Sprintf("https://%s", hostPort.Endpoint()), phttp.Default(certPool), conn.Close() } // buildDialErrorMessage standardizes DNS error messages that appear differently on different platforms, so that tests and log grepping is uniform. @@ -408,15 +407,28 @@ func buildDialErrorMessage(tlsDialErr error) string { return reason } -func validateUserAndGroupAttributes(upstream *v1alpha1.GitHubIdentityProvider) (v1alpha1.GitHubGroupNameAttribute, v1alpha1.GitHubUsernameAttribute, error) { - var groupNameAttribute v1alpha1.GitHubGroupNameAttribute - var usernameAttribute v1alpha1.GitHubUsernameAttribute +func validateUserAndGroupAttributes(upstream *v1alpha1.GitHubIdentityProvider) (*metav1.Condition, v1alpha1.GitHubGroupNameAttribute, v1alpha1.GitHubUsernameAttribute) { + buildInvalidCondition := func(message string) *metav1.Condition { + return &metav1.Condition{ + Type: ClaimsValid, + Status: metav1.ConditionFalse, + Reason: "Invalid", + Message: message, + } + } - if upstream.Spec.Claims.Groups == nil || upstream.Spec.Claims.Username == nil { - return "", "", fmt.Errorf("spec.claims.groups and spec.claims.username are required") + var usernameAttribute v1alpha1.GitHubUsernameAttribute + if upstream.Spec.Claims.Username == nil { + return buildInvalidCondition("spec.claims.username is required"), "", "" + } else { + usernameAttribute = *upstream.Spec.Claims.Username + } + + var groupNameAttribute v1alpha1.GitHubGroupNameAttribute + if upstream.Spec.Claims.Groups == nil { + return buildInvalidCondition("spec.claims.groups is required"), "", "" } else { groupNameAttribute = *upstream.Spec.Claims.Groups - usernameAttribute = *upstream.Spec.Claims.Username } switch usernameAttribute { @@ -425,7 +437,7 @@ func validateUserAndGroupAttributes(upstream *v1alpha1.GitHubIdentityProvider) ( case v1alpha1.GitHubUsernameID: default: // Should not happen due to CRD enum validation - return "", "", fmt.Errorf("invalid spec.claims.username (%q)", usernameAttribute) + return buildInvalidCondition(fmt.Sprintf("spec.claims.username (%q) is not valid", usernameAttribute)), "", "" } switch groupNameAttribute { @@ -433,10 +445,15 @@ func validateUserAndGroupAttributes(upstream *v1alpha1.GitHubIdentityProvider) ( case v1alpha1.GitHubUseTeamSlugForGroupName: default: // Should not happen due to CRD enum validation - return "", "", fmt.Errorf("invalid spec.claims.groups (%q)", groupNameAttribute) + return buildInvalidCondition(fmt.Sprintf("spec.claims.groups (%q) is not valid", groupNameAttribute)), "", "" } - return groupNameAttribute, usernameAttribute, nil + return &metav1.Condition{ + Type: ClaimsValid, + Status: metav1.ConditionTrue, + Reason: upstreamwatchers.ReasonSuccess, + Message: "spec.claims are valid", + }, groupNameAttribute, usernameAttribute } func (c *gitHubWatcherController) updateStatus( @@ -459,7 +476,7 @@ func (c *gitHubWatcherController) updateStatus( updated.Status.Phase = v1alpha1.GitHubPhaseError } - if equality.Semantic.DeepEqual(upstream, updated) { // untested + if equality.Semantic.DeepEqual(upstream, updated) { return hadErrorCondition, nil } diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index c2793d052..d8b72110a 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -6,7 +6,6 @@ package githubupstreamwatcher import ( "bytes" "context" - "crypto/x509" "encoding/base64" "fmt" "net" @@ -22,21 +21,21 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + utilnet "k8s.io/apimachinery/pkg/util/net" k8sinformers "k8s.io/client-go/informers" kubernetesfake "k8s.io/client-go/kubernetes/fake" coretesting "k8s.io/client-go/testing" - "k8s.io/client-go/util/cert" "k8s.io/utils/clock" clocktesting "k8s.io/utils/clock/testing" "k8s.io/utils/ptr" "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" - pinnipedfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" + supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" pinnipedinformers "go.pinniped.dev/generated/latest/client/supervisor/informers/externalversions" "go.pinniped.dev/internal/certauthority" + pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatchers" "go.pinniped.dev/internal/controllerlib" - "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/federationdomain/dynamicupstreamprovider" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/net/phttp" @@ -46,8 +45,6 @@ import ( "go.pinniped.dev/internal/upstreamgithub" ) -const countExpectedConditions = 5 - var ( githubIDPGVR = schema.GroupVersionResource{ Group: v1alpha1.SchemeGroupVersion.Group, @@ -59,7 +56,7 @@ var ( ) func TestController(t *testing.T) { - require.Equal(t, 5, countExpectedConditions) + require.Equal(t, 6, countExpectedConditions) goodServer, goodServerCA := tlsserver.TestServerIPv4(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { }), tlsserver.RecordTLSHello) @@ -116,6 +113,8 @@ func TestController(t *testing.T) { Client: v1alpha1.GitHubClientSpec{ SecretName: goodSecret.Name, }, + // These claims are optional when using the actual Kubernetes CRD. + // However, they are required here because CRD defaulting/validation does not occur during testing. Claims: v1alpha1.GitHubClaims{ Username: ptr.To(v1alpha1.GitHubUsernameLogin), Groups: ptr.To(v1alpha1.GitHubUseTeamSlugForGroupName), @@ -266,6 +265,30 @@ func TestController(t *testing.T) { } } + buildClaimsValidatedTrue := func(t *testing.T) metav1.Condition { + t.Helper() + return metav1.Condition{ + Type: ClaimsValid, + Status: metav1.ConditionTrue, + ObservedGeneration: generation, + LastTransitionTime: lastTransitionTime, + Reason: upstreamwatchers.ReasonSuccess, + Message: "spec.claims are valid", + } + } + + buildClaimsValidatedFalse := func(t *testing.T, message string) metav1.Condition { + t.Helper() + return metav1.Condition{ + Type: ClaimsValid, + Status: metav1.ConditionFalse, + ObservedGeneration: generation, + LastTransitionTime: lastTransitionTime, + Reason: "Invalid", + Message: message, + } + } + buildGitHubConnectionValidTrue := func(t *testing.T, host string) metav1.Condition { t.Helper() @@ -305,6 +328,14 @@ func TestController(t *testing.T) { } } + buildLogForUpdatingClaimsValidTrue := func(name string) string { + return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"ClaimsValid","status":"True","reason":"Success","message":"spec.claims are valid"}`, name) + } + + buildLogForUpdatingClaimsValidFalse := func(name, message string) string { + return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"ClaimsValid","status":"False","reason":"Invalid","message":"%s"}`, name, message) + } + buildLogForUpdatingClientCredentialsObtained := func(name, status, reason, message string) string { return fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"github-upstream-observer","caller":"conditionsutil/conditions_util.go:$conditionsutil.MergeConditions","message":"updated condition","namespace":"some-namespace","name":"%s","type":"ClientCredentialsObtained","status":"%s","reason":"%s","message":"%s"}`, name, status, reason, message) } @@ -365,7 +396,7 @@ func TestController(t *testing.T) { AllowedOrganizations: []string{"organization1", "org2"}, OrganizationLoginPolicy: "OnlyUsersFromAllowedOrganizations", AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host), - HttpClient: buildPretendHttpClient(t, goodServerCA), + HttpClient: nil, // let the test runner populate this for us }, }, wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{ @@ -375,6 +406,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -386,6 +418,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -412,7 +445,7 @@ func TestController(t *testing.T) { }, OrganizationLoginPolicy: "AllGitHubUsers", AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", goodServerDomain), - HttpClient: buildPretendHttpClient(t, goodServerCA), + HttpClient: nil, // let the test runner populate this for us }, }, wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{ @@ -422,6 +455,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validMinimalIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validMinimalIDP.Spec.GitHubAPI.Host), @@ -433,6 +467,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validMinimalIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -466,7 +501,7 @@ func TestController(t *testing.T) { }, OrganizationLoginPolicy: "AllGitHubUsers", AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", goodServerIPv6Domain), - HttpClient: buildPretendHttpClient(t, goodServerCA), + HttpClient: nil, // let the test runner populate this for us }, }, wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{ @@ -484,6 +519,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, goodServerIPv6Domain), buildHostValidTrue(t, goodServerIPv6Domain), @@ -495,6 +531,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, goodServerIPv6Domain), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -546,7 +583,7 @@ func TestController(t *testing.T) { AllowedOrganizations: []string{"organization1", "org2"}, OrganizationLoginPolicy: "OnlyUsersFromAllowedOrganizations", AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host), - HttpClient: buildPretendHttpClient(t, goodServerCA), + HttpClient: nil, // let the test runner populate this for us }, { Name: "other-idp-name", @@ -561,7 +598,7 @@ func TestController(t *testing.T) { AllowedOrganizations: []string{"organization1", "org2"}, OrganizationLoginPolicy: "OnlyUsersFromAllowedOrganizations", AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host), - HttpClient: buildPretendHttpClient(t, goodServerCA), + HttpClient: nil, // let the test runner populate this for us }, }, wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{ @@ -579,6 +616,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedFalse( t, `secret "no-secret-with-this-name" not found`, @@ -608,6 +646,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, "other-secret-name"), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -622,6 +661,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseReady, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -633,6 +673,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("invalid-idp-name", "False", "SecretNotFound", `secret \"no-secret-with-this-name\" not found: secret from spec.client.SecretName (\"no-secret-with-this-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClaimsValidTrue("invalid-idp-name"), buildLogForUpdatingOrganizationPolicyValid("invalid-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("invalid-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("invalid-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -640,6 +681,7 @@ func TestController(t *testing.T) { buildLogForUpdatingPhase("invalid-idp-name", "Error"), buildLogForUpdatingClientCredentialsObtained("other-idp-name", "True", "Success", `clientID and clientSecret have been read from spec.client.SecretName (\"other-secret-name\")`), + buildLogForUpdatingClaimsValidTrue("other-idp-name"), buildLogForUpdatingOrganizationPolicyValid("other-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("other-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("other-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -647,6 +689,7 @@ func TestController(t *testing.T) { buildLogForUpdatingPhase("other-idp-name", "Ready"), buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -675,6 +718,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "", "must not be empty"), @@ -686,6 +730,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: must not be empty`, ""), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -714,6 +759,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "https://example.com", `invalid port "//example.com"`), @@ -725,6 +771,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: invalid port \"//example.com\"`, "https://example.com"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -753,6 +800,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "example.com/foo", `host "example.com/foo" is not a valid hostname or IP address`), @@ -764,6 +812,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: host \"example.com/foo\" is not a valid hostname or IP address`, "example.com/foo"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -792,6 +841,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "u:p@example.com", `invalid port "p@example.com"`), @@ -803,6 +853,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: invalid port \"p@example.com\"`, "u:p@example.com"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -831,6 +882,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "example.com?a=b", `host "example.com?a=b" is not a valid hostname or IP address`), @@ -842,6 +894,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: host \"example.com?a=b\" is not a valid hostname or IP address`, "example.com?a=b"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -870,6 +923,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidFalse(t, "example.com#a", `host "example.com#a" is not a valid hostname or IP address`), @@ -881,6 +935,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"%s\") is not valid: host \"example.com#a\" is not a valid hostname or IP address`, "example.com#a"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -913,6 +968,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidUnknown(t), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -924,6 +980,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "False", "InvalidTLSConfig", "spec.githubAPI.tls.certificateAuthorityData is not valid: certificateAuthorityData is not valid PEM: data does not contain any valid RSA or ECDSA certificates"), @@ -953,6 +1010,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot dial server spec.githubAPI.host (%q): dial tcp: lookup nowhere.bad-tld: no such host`, "nowhere.bad-tld:443")), buildHostValidTrue(t, "nowhere.bad-tld"), @@ -964,6 +1022,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "nowhere.bad-tld"), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -971,6 +1030,47 @@ func TestController(t *testing.T) { buildLogForUpdatingPhase("minimal-idp-name", "Error"), }, }, + { + name: "Connection error - ipv6 without brackets", + secrets: []runtime.Object{goodSecret}, + githubIdentityProviders: []runtime.Object{ + func() runtime.Object { + badIDP := validMinimalIDP.DeepCopy() + badIDP.Spec.GitHubAPI.Host = ptr.To("0:0:0:0:0:0:0:1:9876") + return badIDP + }(), + }, + wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{ + { + ObjectMeta: validMinimalIDP.ObjectMeta, + Spec: func() v1alpha1.GitHubIdentityProviderSpec { + badSpec := validMinimalIDP.Spec.DeepCopy() + badSpec.GitHubAPI.Host = ptr.To("0:0:0:0:0:0:0:1:9876") + return *badSpec + }(), + Status: v1alpha1.GitHubIdentityProviderStatus{ + Phase: v1alpha1.GitHubPhaseError, + Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), + buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName), + buildGitHubConnectionValidUnknown(t), + buildHostValidFalse(t, "0:0:0:0:0:0:0:1:9876", `host "0:0:0:0:0:0:0:1:9876" is not a valid hostname or IP address`), + buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy), + buildTLSConfigurationValidTrue(t), + }, + }, + }, + }, + wantLogs: []string{ + buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), + buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), + buildLogForUpdatingHostValid("minimal-idp-name", "False", "InvalidHost", `spec.githubAPI.host (\"0:0:0:0:0:0:0:1:9876\") is not valid: host \"%s\" is not a valid hostname or IP address`, "0:0:0:0:0:0:0:1:9876"), + buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), + buildLogForUpdatingGitHubConnectionValidUnknown("minimal-idp-name"), + buildLogForUpdatingPhase("minimal-idp-name", "Error"), + }, + }, { name: "Connection error - host not trusted by system certs", secrets: []runtime.Object{goodSecret}, @@ -993,6 +1093,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot dial server spec.githubAPI.host (%q): tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host)), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1004,6 +1105,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1037,6 +1139,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidFalse(t, fmt.Sprintf(`cannot dial server spec.githubAPI.host (%q): tls: failed to verify certificate: x509: certificate signed by unknown authority`, *validFilledOutIDP.Spec.GitHubAPI.Host)), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1048,6 +1151,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1076,6 +1180,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1087,6 +1192,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1115,6 +1221,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1126,6 +1233,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1154,6 +1262,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1165,6 +1274,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1193,6 +1303,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1204,6 +1315,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "False", "Invalid", "spec.allowAuthentication.organizations.policy must be 'AllGitHubUsers' when spec.allowAuthentication.organizations.allowed is empty"), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1221,7 +1333,6 @@ func TestController(t *testing.T) { return badIDP }(), }, - wantErr: "spec.claims.groups and spec.claims.username are required", wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{ { ObjectMeta: validFilledOutIDP.ObjectMeta, @@ -1231,8 +1342,9 @@ func TestController(t *testing.T) { return *badSpec }(), Status: v1alpha1.GitHubIdentityProviderStatus{ - Phase: v1alpha1.GitHubPhaseReady, + Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedFalse(t, "spec.claims.username is required"), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1244,11 +1356,12 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidFalse("some-idp-name", "spec.claims.username is required"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host), - buildLogForUpdatingPhase("some-idp-name", "Ready"), + buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, { @@ -1261,7 +1374,6 @@ func TestController(t *testing.T) { return badIDP }(), }, - wantErr: "invalid spec.claims.username", wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{ { ObjectMeta: validFilledOutIDP.ObjectMeta, @@ -1271,8 +1383,9 @@ func TestController(t *testing.T) { return *badSpec }(), Status: v1alpha1.GitHubIdentityProviderStatus{ - Phase: v1alpha1.GitHubPhaseReady, + Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedFalse(t, `spec.claims.username ("a") is not valid`), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1284,11 +1397,12 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidFalse("some-idp-name", `spec.claims.username (\"a\") is not valid`), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host), - buildLogForUpdatingPhase("some-idp-name", "Ready"), + buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, { @@ -1301,7 +1415,6 @@ func TestController(t *testing.T) { return badIDP }(), }, - wantErr: "spec.claims.groups and spec.claims.username are required", wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{ { ObjectMeta: validFilledOutIDP.ObjectMeta, @@ -1311,8 +1424,9 @@ func TestController(t *testing.T) { return *badSpec }(), Status: v1alpha1.GitHubIdentityProviderStatus{ - Phase: v1alpha1.GitHubPhaseReady, + Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedFalse(t, "spec.claims.groups is required"), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1324,11 +1438,12 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidFalse("some-idp-name", "spec.claims.groups is required"), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host), - buildLogForUpdatingPhase("some-idp-name", "Ready"), + buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, { @@ -1337,22 +1452,22 @@ func TestController(t *testing.T) { githubIdentityProviders: []runtime.Object{ func() runtime.Object { badIDP := validFilledOutIDP.DeepCopy() - badIDP.Spec.Claims.Groups = ptr.To[v1alpha1.GitHubGroupNameAttribute]("a") + badIDP.Spec.Claims.Groups = ptr.To[v1alpha1.GitHubGroupNameAttribute]("b") return badIDP }(), }, - wantErr: "invalid spec.claims.groups", wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{ { ObjectMeta: validFilledOutIDP.ObjectMeta, Spec: func() v1alpha1.GitHubIdentityProviderSpec { badSpec := validFilledOutIDP.Spec.DeepCopy() - badSpec.Claims.Groups = ptr.To[v1alpha1.GitHubGroupNameAttribute]("a") + badSpec.Claims.Groups = ptr.To[v1alpha1.GitHubGroupNameAttribute]("b") return *badSpec }(), Status: v1alpha1.GitHubIdentityProviderStatus{ - Phase: v1alpha1.GitHubPhaseReady, + Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedFalse(t, `spec.claims.groups ("b") is not valid`), buildClientCredentialsObtainedTrue(t, validFilledOutIDP.Spec.Client.SecretName), buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), @@ -1364,11 +1479,12 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidFalse("some-idp-name", `spec.claims.groups (\"b\") is not valid`), buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host), - buildLogForUpdatingPhase("some-idp-name", "Ready"), + buildLogForUpdatingPhase("some-idp-name", "Error"), }, }, { @@ -1388,6 +1504,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedFalse( t, fmt.Sprintf("secret %q not found", validMinimalIDP.Spec.Client.SecretName), @@ -1405,6 +1522,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `secret \"some-secret-name\" not found: secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1429,6 +1547,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedFalse( t, `wrong secret type "other-type"`, @@ -1446,6 +1565,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `wrong secret type \"other-type\": secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validMinimalIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1470,6 +1590,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedFalse( t, `missing key "clientID"`, @@ -1487,6 +1608,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `missing key \"clientID\": secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1511,6 +1633,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedFalse( t, `missing key "clientSecret"`, @@ -1528,6 +1651,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `missing key \"clientSecret\": secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validMinimalIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1552,6 +1676,7 @@ func TestController(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), buildClientCredentialsObtainedFalse( t, "extra keys found", @@ -1569,6 +1694,7 @@ func TestController(t *testing.T) { }, wantLogs: []string{ buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "False", "SecretNotFound", `extra keys found: secret from spec.client.SecretName (\"some-secret-name\") must be found in namespace \"some-namespace\" with type \"secrets.pinniped.dev/github-client\" and keys \"clientID\" and \"clientSecret\"`), + buildLogForUpdatingClaimsValidTrue("minimal-idp-name"), buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))), buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"), @@ -1583,9 +1709,8 @@ func TestController(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - fakePinnipedClient := pinnipedfake.NewSimpleClientset(tt.githubIdentityProviders...) - fakePinnipedClientForInformers := pinnipedfake.NewSimpleClientset(tt.githubIdentityProviders...) - pinnipedInformers := pinnipedinformers.NewSharedInformerFactory(fakePinnipedClientForInformers, 0) + fakeSupervisorClient := supervisorfake.NewSimpleClientset(tt.githubIdentityProviders...) + supervisorInformers := pinnipedinformers.NewSharedInformerFactory(fakeSupervisorClient, 0) fakeKubeClient := kubernetesfake.NewSimpleClientset(tt.secrets...) kubeInformers := k8sinformers.NewSharedInformerFactoryWithOptions(fakeKubeClient, 0) @@ -1598,24 +1723,23 @@ func TestController(t *testing.T) { var log bytes.Buffer logger := plog.TestLogger(t, &log) - gitHubIdentityProviderInformer := pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders() + gitHubIdentityProviderInformer := supervisorInformers.IDP().V1alpha1().GitHubIdentityProviders() controller := New( namespace, cache, - fakePinnipedClient, + fakeSupervisorClient, gitHubIdentityProviderInformer, kubeInformers.Core().V1().Secrets(), logger, controllerlib.WithInformer, frozenClock, - buildHttpClient, ) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - pinnipedInformers.Start(ctx.Done()) + supervisorInformers.Start(ctx.Done()) kubeInformers.Start(ctx.Done()) controllerlib.TestRunSynchronously(t, controller) @@ -1653,16 +1777,25 @@ func TestController(t *testing.T) { require.Equal(t, tt.wantResultingCache[i].AllowedOrganizations, actualIDP.GetAllowedOrganizations()) require.Equal(t, tt.wantResultingCache[i].OrganizationLoginPolicy, actualIDP.GetOrganizationLoginPolicy()) require.Equal(t, tt.wantResultingCache[i].AuthorizationURL, actualIDP.GetAuthorizationURL()) - compareTLSClientConfigWithinHttpClients(t, tt.wantResultingCache[i].HttpClient, actualIDP.GetHttpClient()) + + require.GreaterOrEqual(t, len(tt.githubIdentityProviders), i+1, "there must be at least as many input identity providers as items in the cache") + githubIDP, ok := tt.githubIdentityProviders[i].(*v1alpha1.GitHubIdentityProvider) + require.True(t, ok) + certPool, _, err := pinnipedcontroller.BuildCertPoolIDP(githubIDP.Spec.GitHubAPI.TLS) + require.NoError(t, err) + + compareTLSClientConfigWithinHttpClients(t, phttp.Default(certPool), actualIDP.GetHttpClient()) require.Equal(t, tt.wantResultingCache[i].OAuth2Config, actualIDP.OAuth2Config) } // Verify the status conditions as reported in Kubernetes - allGitHubIDPs, err := fakePinnipedClient.IDPV1alpha1().GitHubIdentityProviders(namespace).List(ctx, metav1.ListOptions{}) + allGitHubIDPs, err := fakeSupervisorClient.IDPV1alpha1().GitHubIdentityProviders(namespace).List(ctx, metav1.ListOptions{}) require.NoError(t, err) require.Equal(t, len(tt.wantResultingUpstreams), len(allGitHubIDPs.Items)) for i := 0; i < len(tt.wantResultingUpstreams); i++ { + require.Len(t, tt.wantResultingUpstreams[i].Status.Conditions, countExpectedConditions) + // Do not expect any particular order in the K8s objects var actualIDP *v1alpha1.GitHubIdentityProvider for _, possibleMatch := range allGitHubIDPs.Items { @@ -1673,7 +1806,7 @@ func TestController(t *testing.T) { } require.NotNil(t, actualIDP, "must find IDP with name %s", tt.wantResultingUpstreams[i].Name) - require.Equal(t, countExpectedConditions, len(actualIDP.Status.Conditions)) + require.Len(t, actualIDP.Status.Conditions, countExpectedConditions) // Update all expected conditions to the frozenTime. // TODO: Push this out to the test table @@ -1693,18 +1826,21 @@ func TestController(t *testing.T) { require.Equal(t, expectedLogs, log.String()) // This needs to happen after the expected condition LastTransitionTime has been updated. - wantActions := make([]coretesting.Action, 1+len(tt.wantResultingUpstreams)) + wantActions := make([]coretesting.Action, 3+len(tt.wantResultingUpstreams)) + wantActions[0] = coretesting.NewListAction(githubIDPGVR, githubIDPKind, "", metav1.ListOptions{}) + wantActions[1] = coretesting.NewWatchAction(githubIDPGVR, "", metav1.ListOptions{}) for i, want := range tt.wantResultingUpstreams { - wantActions[i] = coretesting.NewUpdateSubresourceAction(githubIDPGVR, "status", want.Namespace, ptr.To(want)) + wantActions[2+i] = coretesting.NewUpdateSubresourceAction(githubIDPGVR, "status", want.Namespace, ptr.To(want)) } - wantActions[len(tt.wantResultingUpstreams)] = coretesting.NewListAction(githubIDPGVR, githubIDPKind, namespace, metav1.ListOptions{}) - require.Equal(t, wantActions, fakePinnipedClient.Actions()) + // This List action is coming from the test code when it pulls the GitHubIdentityProviders from K8s + wantActions[len(wantActions)-1] = coretesting.NewListAction(githubIDPGVR, githubIDPKind, namespace, metav1.ListOptions{}) + require.Equal(t, wantActions, fakeSupervisorClient.Actions()) }) } } -func TestController_WithExistingConditions(t *testing.T) { - require.Equal(t, 5, countExpectedConditions) +func TestController_OnlyWantActions(t *testing.T) { + require.Equal(t, 6, countExpectedConditions) goodServer, goodServerCA := tlsserver.TestServerIPv4(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { }), tlsserver.RecordTLSHello) @@ -1717,6 +1853,49 @@ func TestController_WithExistingConditions(t *testing.T) { nowDoesntMatter := time.Date(1122, time.September, 33, 4, 55, 56, 778899, time.Local) frozenClock := clocktesting.NewFakeClock(nowDoesntMatter) + goodSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-secret-name", + Namespace: namespace, + }, + Type: "secrets.pinniped.dev/github-client", + Data: map[string][]byte{ + "clientID": []byte("some-client-id"), + "clientSecret": []byte("some-client-secret"), + }, + } + + validMinimalIDP := &v1alpha1.GitHubIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "minimal-idp-name", + Namespace: namespace, + UID: types.UID("minimal-uid"), + Generation: 1234, + }, + Spec: v1alpha1.GitHubIdentityProviderSpec{ + GitHubAPI: v1alpha1.GitHubAPIConfig{ + Host: ptr.To(goodServerDomain), + TLS: &v1alpha1.TLSSpec{ + CertificateAuthorityData: goodServerCAB64, + }, + }, + // These claims are optional when using the actual Kubernetes CRD. + // However, they are required here because CRD defaulting/validation does not occur during testing. + Claims: v1alpha1.GitHubClaims{ + Username: ptr.To(v1alpha1.GitHubUsernameLogin), + Groups: ptr.To(v1alpha1.GitHubUseTeamSlugForGroupName), + }, + Client: v1alpha1.GitHubClientSpec{ + SecretName: goodSecret.Name, + }, + AllowAuthentication: v1alpha1.GitHubAllowAuthenticationSpec{ + Organizations: v1alpha1.GitHubOrganizationsSpec{ + Policy: ptr.To(v1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers), + }, + }, + }, + } + alreadyInvalidExistingIDP := &v1alpha1.GitHubIdentityProvider{ ObjectMeta: metav1.ObjectMeta{ Name: "already-existing-invalid-idp-name", @@ -1737,8 +1916,7 @@ func TestController_WithExistingConditions(t *testing.T) { }, }, Claims: v1alpha1.GitHubClaims{ - Username: ptr.To(v1alpha1.GitHubUsernameLogin), - Groups: ptr.To(v1alpha1.GitHubUseTeamSlugForGroupName), + Groups: ptr.To(v1alpha1.GitHubUseTeamSlugForGroupName), }, Client: v1alpha1.GitHubClientSpec{ SecretName: "unknown-secret", @@ -1747,6 +1925,14 @@ func TestController_WithExistingConditions(t *testing.T) { Status: v1alpha1.GitHubIdentityProviderStatus{ Phase: v1alpha1.GitHubPhaseError, Conditions: []metav1.Condition{ + { + Type: ClaimsValid, + Status: metav1.ConditionFalse, + ObservedGeneration: 333, + LastTransitionTime: oneHourAgo, + Reason: "Invalid", + Message: "spec.claims.username is required", + }, { Type: ClientCredentialsObtained, Status: metav1.ConditionFalse, @@ -1795,6 +1981,8 @@ func TestController_WithExistingConditions(t *testing.T) { name string secrets []runtime.Object githubIdentityProviders []runtime.Object + addSupervisorReactors func(*supervisorfake.Clientset) + wantErr string wantActions []coretesting.Action }{ { @@ -1833,6 +2021,76 @@ func TestController_WithExistingConditions(t *testing.T) { }(), }, }, + { + name: "K8s client error - cannot update githubidentityproviders", + secrets: []runtime.Object{goodSecret}, + githubIdentityProviders: []runtime.Object{validMinimalIDP}, + wantErr: "error from reactor - unable to update", + addSupervisorReactors: func(fake *supervisorfake.Clientset) { + fake.PrependReactor("update", "githubidentityproviders", func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("error from reactor - unable to update") + }) + }, + wantActions: []coretesting.Action{ + coretesting.NewUpdateSubresourceAction(githubIDPGVR, "status", namespace, func() runtime.Object { + idpWithConditions := validMinimalIDP.DeepCopy() + idpWithConditions.Status = v1alpha1.GitHubIdentityProviderStatus{ + Phase: v1alpha1.GitHubPhaseReady, + Conditions: []metav1.Condition{ + { + Type: ClaimsValid, + Status: metav1.ConditionTrue, + ObservedGeneration: 1234, + LastTransitionTime: metav1.NewTime(frozenClock.Now()), + Reason: upstreamwatchers.ReasonSuccess, + Message: "spec.claims are valid", + }, + { + Type: ClientCredentialsObtained, + Status: metav1.ConditionTrue, + ObservedGeneration: 1234, + LastTransitionTime: metav1.NewTime(frozenClock.Now()), + Reason: upstreamwatchers.ReasonSuccess, + Message: `clientID and clientSecret have been read from spec.client.SecretName ("some-secret-name")`, + }, + { + Type: GitHubConnectionValid, + Status: metav1.ConditionTrue, + ObservedGeneration: 1234, + LastTransitionTime: metav1.NewTime(frozenClock.Now()), + Reason: upstreamwatchers.ReasonSuccess, + Message: fmt.Sprintf("spec.githubAPI.host (%q) is reachable and TLS verification succeeds", goodServerDomain), + }, + { + Type: HostValid, + Status: metav1.ConditionTrue, + ObservedGeneration: 1234, + LastTransitionTime: metav1.NewTime(frozenClock.Now()), + Reason: upstreamwatchers.ReasonSuccess, + Message: fmt.Sprintf("spec.githubAPI.host (%q) is valid", goodServerDomain), + }, + { + Type: OrganizationsPolicyValid, + Status: metav1.ConditionTrue, + ObservedGeneration: 1234, + LastTransitionTime: metav1.NewTime(frozenClock.Now()), + Reason: upstreamwatchers.ReasonSuccess, + Message: `spec.allowAuthentication.organizations.policy ("AllGitHubUsers") is valid`, + }, + { + Type: TLSConfigurationValid, + Status: metav1.ConditionTrue, + ObservedGeneration: 1234, + LastTransitionTime: metav1.NewTime(frozenClock.Now()), + Reason: upstreamwatchers.ReasonSuccess, + Message: "spec.githubAPI.tls.certificateAuthorityData is valid", + }, + }, + } + return idpWithConditions + }()), + }, + }, } for _, tt := range tests { @@ -1840,8 +2098,12 @@ func TestController_WithExistingConditions(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - fakePinnipedClient := pinnipedfake.NewSimpleClientset(tt.githubIdentityProviders...) - pinnipedInformers := pinnipedinformers.NewSharedInformerFactory(pinnipedfake.NewSimpleClientset(tt.githubIdentityProviders...), 0) + fakeSupervisorClient := supervisorfake.NewSimpleClientset(tt.githubIdentityProviders...) + supervisorInformers := pinnipedinformers.NewSharedInformerFactory(supervisorfake.NewSimpleClientset(tt.githubIdentityProviders...), 0) + + if tt.addSupervisorReactors != nil { + tt.addSupervisorReactors(fakeSupervisorClient) + } kubeInformers := k8sinformers.NewSharedInformerFactoryWithOptions(kubernetesfake.NewSimpleClientset(tt.secrets...), 0) @@ -1851,67 +2113,51 @@ func TestController_WithExistingConditions(t *testing.T) { controller := New( namespace, dynamicupstreamprovider.NewDynamicUpstreamIDPProvider(), - fakePinnipedClient, - pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(), + fakeSupervisorClient, + supervisorInformers.IDP().V1alpha1().GitHubIdentityProviders(), kubeInformers.Core().V1().Secrets(), logger, controllerlib.WithInformer, frozenClock, - buildHttpClient, ) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - pinnipedInformers.Start(ctx.Done()) + supervisorInformers.Start(ctx.Done()) kubeInformers.Start(ctx.Done()) controllerlib.TestRunSynchronously(t, controller) syncCtx := controllerlib.Context{Context: ctx, Key: controllerlib.Key{}} - err := controllerlib.TestSync(t, controller, syncCtx) - require.NoError(t, err) - - require.Equal(t, tt.wantActions, fakePinnipedClient.Actions()) + if err := controllerlib.TestSync(t, controller, syncCtx); len(tt.wantErr) > 0 { + require.ErrorContains(t, err, controllerlib.ErrSyntheticRequeue.Error()) + require.ErrorContains(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + require.Equal(t, tt.wantActions, fakeSupervisorClient.Actions()) }) } } -func buildPretendHttpClient(t *testing.T, ca []byte) *http.Client { +func compareTLSClientConfigWithinHttpClients(t *testing.T, expected *http.Client, actual *http.Client) { t.Helper() - rootCAs, err := cert.NewPoolFromBytes(ca) + + require.NotEmpty(t, expected) + require.NotEmpty(t, actual) + + require.Equal(t, expected.Timeout, actual.Timeout) + + expectedConfig, err := utilnet.TLSClientConfig(expected.Transport) require.NoError(t, err) - return buildHttpClient(rootCAs) -} -func buildHttpClient(rootCAs *x509.CertPool) *http.Client { - baseRT := http.DefaultTransport.(*http.Transport).Clone() - baseRT.TLSClientConfig = ptls.Default(rootCAs) + actualConfig, err := utilnet.TLSClientConfig(actual.Transport) + require.NoError(t, err) - return &http.Client{ - Transport: baseRT, - } -} - -func compareTLSClientConfigWithinHttpClients(t *testing.T, c1 *http.Client, c2 *http.Client) { - t.Helper() - - if c1 == nil { - require.Nil(t, c2) - return - } - - t1, ok := c1.Transport.(*http.Transport) - require.True(t, ok) - require.NotNil(t, t1) - require.NotNil(t, t1.TLSClientConfig) - - t2, ok := c2.Transport.(*http.Transport) - require.True(t, ok) - require.NotNil(t, t2) - require.NotNil(t, t2.TLSClientConfig) - - require.Equal(t, t1.TLSClientConfig.ClientCAs, t2.TLSClientConfig.ClientCAs) + require.True(t, actualConfig.RootCAs.Equal(expectedConfig.RootCAs)) + actualConfig.RootCAs = expectedConfig.RootCAs + require.Equal(t, expectedConfig, actualConfig) } func TestGitHubUpstreamWatcherControllerFilterSecret(t *testing.T) { @@ -1977,13 +2223,12 @@ func TestGitHubUpstreamWatcherControllerFilterSecret(t *testing.T) { _ = New( namespace, dynamicupstreamprovider.NewDynamicUpstreamIDPProvider(), - pinnipedfake.NewSimpleClientset(), - pinnipedinformers.NewSharedInformerFactory(pinnipedfake.NewSimpleClientset(), 0).IDP().V1alpha1().GitHubIdentityProviders(), + supervisorfake.NewSimpleClientset(), + pinnipedinformers.NewSharedInformerFactory(supervisorfake.NewSimpleClientset(), 0).IDP().V1alpha1().GitHubIdentityProviders(), secretInformer, logger, observableInformers.WithInformer, clock.RealClock{}, - phttp.Default, ) unrelated := &corev1.Secret{} @@ -2041,19 +2286,18 @@ func TestGitHubUpstreamWatcherControllerFilterGitHubIDP(t *testing.T) { var log bytes.Buffer logger := plog.TestLogger(t, &log) - gitHubIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactory(pinnipedfake.NewSimpleClientset(), 0).IDP().V1alpha1().GitHubIdentityProviders() + gitHubIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactory(supervisorfake.NewSimpleClientset(), 0).IDP().V1alpha1().GitHubIdentityProviders() observableInformers := testutil.NewObservableWithInformerOption() _ = New( namespace, dynamicupstreamprovider.NewDynamicUpstreamIDPProvider(), - pinnipedfake.NewSimpleClientset(), + supervisorfake.NewSimpleClientset(), gitHubIdentityProviderInformer, k8sinformers.NewSharedInformerFactoryWithOptions(kubernetesfake.NewSimpleClientset(), 0).Core().V1().Secrets(), logger, observableInformers.WithInformer, clock.RealClock{}, - phttp.Default, ) unrelated := &v1alpha1.GitHubIdentityProvider{} diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 2a7d3d739..1afe994cf 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -68,7 +68,6 @@ import ( "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/leaderelection" - "go.pinniped.dev/internal/net/phttp" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/pversion" "go.pinniped.dev/internal/secret" @@ -334,7 +333,6 @@ func prepareControllers( plog.New(), controllerlib.WithInformer, clock.RealClock{}, - phttp.Default, ), singletonWorker). WithController( diff --git a/test/integration/supervisor_github_idp_test.go b/test/integration/supervisor_github_idp_test.go index 608ae66c2..5192f5442 100644 --- a/test/integration/supervisor_github_idp_test.go +++ b/test/integration/supervisor_github_idp_test.go @@ -7,6 +7,8 @@ import ( "context" "encoding/base64" "fmt" + "os" + "path/filepath" "testing" "time" @@ -16,6 +18,7 @@ import ( "k8s.io/utils/ptr" idpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" + "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/test/testlib" @@ -254,6 +257,76 @@ func TestGitHubIDPStaticValidationOnCreate_Parallel(t *testing.T) { } } +func TestGitHubIDPSetsDefaultsWithKubectl_Parallel(t *testing.T) { + env := testlib.IntegrationEnv(t) + + adminClient := testlib.NewKubernetesClientset(t) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + t.Cleanup(cancel) + + namespaceClient := adminClient.CoreV1().Namespaces() + + ns, err := namespaceClient.Create(ctx, &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: generateNamePrefix, + }, + }, metav1.CreateOptions{}) + require.NoError(t, err) + + t.Cleanup(func() { + require.NoError(t, namespaceClient.Delete(ctx, ns.Name, metav1.DeleteOptions{})) + }) + t.Logf("Created namespace %s", ns.Name) + + idpName := generateNamePrefix + testlib.RandHex(t, 16) + + githubIDPYaml := []byte(here.Doc(fmt.Sprintf(` + --- + apiVersion: idp.supervisor.%s/v1alpha1 + kind: GitHubIdentityProvider + metadata: + name: %s + namespace: %s + spec: + allowAuthentication: + organizations: + policy: AllGitHubUsers + client: + secretName: any-secret-name`, env.APIGroupSuffix, idpName, ns.Name))) + + githubIDPYamlFilepath := filepath.Join(t.TempDir(), "github-idp.yaml") + + require.NoError(t, os.WriteFile(githubIDPYamlFilepath, githubIDPYaml, 0600)) + + stdOut, stdErr := runTestKubectlCommand(t, "create", "-f", githubIDPYamlFilepath) + + require.Equal(t, fmt.Sprintf("githubidentityprovider.idp.supervisor.%s/%s created\n", env.APIGroupSuffix, idpName), stdOut) + require.Empty(t, stdErr) + + gitHubIDPClient := testlib.NewSupervisorClientset(t).IDPV1alpha1().GitHubIdentityProviders(ns.Name) + + idp, err := gitHubIDPClient.Get(ctx, idpName, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, idpv1alpha1.GitHubIdentityProviderSpec{ + GitHubAPI: idpv1alpha1.GitHubAPIConfig{ + Host: ptr.To("github.com"), + }, + Claims: idpv1alpha1.GitHubClaims{ + Username: ptr.To(idpv1alpha1.GitHubUsernameLoginAndID), + Groups: ptr.To(idpv1alpha1.GitHubUseTeamSlugForGroupName), + }, + AllowAuthentication: idpv1alpha1.GitHubAllowAuthenticationSpec{ + Organizations: idpv1alpha1.GitHubOrganizationsSpec{ + Policy: ptr.To(idpv1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers), + }, + }, + Client: idpv1alpha1.GitHubClientSpec{ + SecretName: "any-secret-name", + }, + }, idp.Spec) +} + func TestGitHubIDPPhaseAndConditions_Parallel(t *testing.T) { // These operations must be performed in the Supervisor's namespace so that the controller can find GitHubIdentityProvider supervisorNamespace := testlib.IntegrationEnv(t).SupervisorNamespace @@ -304,6 +377,12 @@ func TestGitHubIDPPhaseAndConditions_Parallel(t *testing.T) { }, wantPhase: idpv1alpha1.GitHubPhaseReady, wantConditions: []*metav1.Condition{ + { + Type: "ClaimsValid", + Status: metav1.ConditionTrue, + Reason: "Success", + Message: "spec.claims are valid", + }, { Type: "ClientCredentialsObtained", Status: metav1.ConditionTrue, @@ -365,6 +444,12 @@ func TestGitHubIDPPhaseAndConditions_Parallel(t *testing.T) { }, wantPhase: idpv1alpha1.GitHubPhaseError, wantConditions: []*metav1.Condition{ + { + Type: "ClaimsValid", + Status: metav1.ConditionTrue, + Reason: "Success", + Message: "spec.claims are valid", + }, { Type: "ClientCredentialsObtained", Status: metav1.ConditionFalse, @@ -566,6 +651,12 @@ func TestGitHubIDPSecretInOtherNamespace_Parallel(t *testing.T) { testlib.WaitForGitHubIDPPhase(ctx, t, gitHubIDPClient, created.Name, idpv1alpha1.GitHubPhaseError) testlib.WaitForGitHubIdentityProviderStatusConditions(ctx, t, gitHubIDPClient, created.Name, []*metav1.Condition{ + { + Type: "ClaimsValid", + Status: metav1.ConditionTrue, + Reason: "Success", + Message: "spec.claims are valid", + }, { Type: "ClientCredentialsObtained", Status: metav1.ConditionFalse,