mirrors/pinniped
1
0
Fork 0
mirror of https://github.com/vmware-tanzu/pinniped.git synced 2025-12-23 06:15:47 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge pull request #1925 from vmware-tanzu/jtc/polish-up-github-validations

Browse Source 
Polish up GitHub validations
This commit is contained in:
Ben Petersen
2024-04-25 16:40:24 -04:00
committed by GitHub
parent d0bbfca831 14b1b7c862
commit 8938990c04
26 changed files with 520 additions and 127 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go.tmpl
View File

@@ -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.

1
deploy/supervisor/idp.supervisor.pinniped.dev_githubidentityproviders.yaml
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/1.21/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

1
generated/1.21/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml generated
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/1.22/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

1
generated/1.22/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml generated
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/1.23/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

1
generated/1.23/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml generated
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/1.24/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

1
generated/1.24/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml generated
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/1.25/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

1
generated/1.25/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml generated
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/1.26/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

1
generated/1.26/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml generated
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/1.27/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

1
generated/1.27/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml generated
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/1.28/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

1
generated/1.28/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml generated
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/1.29/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

1
generated/1.29/crds/idp.supervisor.pinniped.dev_githubidentityproviders.yaml generated
View File

@@ -114,6 +114,7 @@ spec:
- organizations
type: object
claims:
default: {}
description: Claims allows customization of the username and groups
claims.
properties:

2
generated/latest/apis/supervisor/idp/v1alpha1/types_githubidentityprovider.go generated
View File

@@ -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.

37
internal/controller/conditionsutil/conditions_util_test.go
View File

@@ -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"))

59
internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go
View File

@@ -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
}

426
internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go
View File

File diff suppressed because it is too large Load Diff

2
internal/supervisor/server/server.go
View File

@@ -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(

91
test/integration/supervisor_github_idp_test.go
View File

@@ -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,