mirrors/pinniped
1
0
Fork 0
mirror of https://github.com/vmware-tanzu/pinniped.git synced 2026-01-06 05:27:23 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

CRD already validates that IDP transform constant names are unique

Browse Source 
- Remove that validation from the controller since the CRD already
  validates it during creates and updates.
- Also finish the supervisor_federationdomain_status_test.go by adding
  more tests for both controller validations and CRD validations
This commit is contained in:
Ryan Richard
2023-07-21 12:30:40 -07:00
parent bd5cabf0ff
commit 6d82a11645
4 changed files with 664 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files

48
internal/controller/supervisorconfig/federation_domain_watcher.go
View File

@@ -45,7 +45,6 @@ const (
typeIdentityProvidersDisplayNamesUnique = "IdentityProvidersDisplayNamesUnique"
typeIdentityProvidersAPIGroupSuffixValid = "IdentityProvidersObjectRefAPIGroupSuffixValid"
typeIdentityProvidersObjectRefKindValid = "IdentityProvidersObjectRefKindValid"
typeTransformsConstantsNamesUnique = "TransformsConstantsNamesUnique"
typeTransformsExpressionsValid = "TransformsExpressionsValid"
typeTransformsExamplesPassed = "TransformsExamplesPassed"
@@ -62,7 +61,6 @@ const (
reasonDuplicateDisplayNames = "DuplicateDisplayNames"
reasonAPIGroupNameUnrecognized = "APIGroupUnrecognized"
reasonKindUnrecognized = "KindUnrecognized"
reasonDuplicateConstantsNames = "DuplicateConstantsNames"
reasonInvalidTransformsExpressions = "InvalidTransformsExpressions"
reasonTransformsExamplesFailed = "TransformsExamplesFailed"
@@ -330,7 +328,6 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer(
conditions = appendIdentityProviderDuplicateDisplayNamesCondition(sets.Set[string]{}, conditions)
conditions = appendIdentityProviderObjectRefAPIGroupSuffixCondition(c.apiGroup, []string{}, conditions)
conditions = appendIdentityProviderObjectRefKindCondition(c.sortedAllowedKinds(), []string{}, conditions)
conditions = appendTransformsConstantsNamesUniqueCondition([]string{}, conditions)
conditions = appendTransformsExpressionsValidCondition([]string{}, conditions)
conditions = appendTransformsExamplesPassedCondition([]string{}, conditions)
@@ -431,7 +428,6 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic
conditions = appendIdentityProviderObjectRefAPIGroupSuffixCondition(c.apiGroup, badAPIGroupNames, conditions)
conditions = appendIdentityProviderObjectRefKindCondition(c.sortedAllowedKinds(), badKinds, conditions)
conditions = appendTransformsConstantsNamesUniqueCondition(validationErrorMessages.errorsForConstants, conditions)
conditions = appendTransformsExpressionsValidCondition(validationErrorMessages.errorsForExpressions, conditions)
conditions = appendTransformsExamplesPassedCondition(validationErrorMessages.errorsForExamples, conditions)
@@ -472,13 +468,10 @@ func (c *federationDomainWatcherController) makeTransformationPipelineAndEvaluat
idpIndex int,
validationErrorMessages *transformsValidationErrorMessages,
) (*idtransform.TransformationPipeline, bool, error) {
consts, errorsForConstants, err := c.makeTransformsConstantsForIdentityProvider(idp, idpIndex)
consts, err := c.makeTransformsConstantsForIdentityProvider(idp)
if err != nil {
return nil, false, err
}
if len(errorsForConstants) > 0 {
validationErrorMessages.errorsForConstants = append(validationErrorMessages.errorsForConstants, errorsForConstants)
}
pipeline, errorsForExpressions, err := c.makeTransformationPipelineForIdentityProvider(idp, idpIndex, consts)
if err != nil {
@@ -498,22 +491,17 @@ func (c *federationDomainWatcherController) makeTransformationPipelineAndEvaluat
func (c *federationDomainWatcherController) makeTransformsConstantsForIdentityProvider(
idp configv1alpha1.FederationDomainIdentityProvider,
idpIndex int,
) (*celtransformer.TransformationConstants, string, error) {
) (*celtransformer.TransformationConstants, error) {
consts := &celtransformer.TransformationConstants{
StringConstants: map[string]string{},
StringListConstants: map[string][]string{},
}
constNames := sets.Set[string]{}
duplicateConstNames := sets.Set[string]{}
// Read all the declared constants.
for _, constant := range idp.Transforms.Constants {
// The CRD requires the name field, and validates that it has at least one character,
// so here we only need to validate that they are unique.
if constNames.Has(constant.Name) {
duplicateConstNames.Insert(constant.Name)
}
// and validates that the names are unique within the list.
constNames.Insert(constant.Name)
switch constant.Type {
case "string":
@@ -522,17 +510,11 @@ func (c *federationDomainWatcherController) makeTransformsConstantsForIdentityPr
consts.StringListConstants[constant.Name] = constant.StringListValue
default:
// This shouldn't really happen since the CRD validates it, but handle it as an error.
return nil, "", fmt.Errorf("one of spec.identityProvider[].transforms.constants[].type is invalid: %q", constant.Type)
return nil, fmt.Errorf("one of spec.identityProvider[].transforms.constants[].type is invalid: %q", constant.Type)
}
}
if duplicateConstNames.Len() > 0 {
return consts, fmt.Sprintf(
"the names specified by .spec.identityProviders[%d].transforms.constants[].name contain duplicates: %s",
idpIndex, strings.Join(sortAndQuote(duplicateConstNames.UnsortedList()), ", ")), nil
}
return consts, "", nil
return consts, nil
}
func (c *federationDomainWatcherController) makeTransformationPipelineForIdentityProvider(
@@ -764,25 +746,6 @@ func appendTransformsExamplesPassedCondition(messages []string, conditions []*co
return conditions
}
func appendTransformsConstantsNamesUniqueCondition(messages []string, conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition {
if len(messages) > 0 {
conditions = append(conditions, &configv1alpha1.Condition{
Type: typeTransformsConstantsNamesUnique,
Status: configv1alpha1.ConditionFalse,
Reason: reasonDuplicateConstantsNames,
Message: strings.Join(messages, "\n\n"),
})
} else {
conditions = append(conditions, &configv1alpha1.Condition{
Type: typeTransformsConstantsNamesUnique,
Status: configv1alpha1.ConditionTrue,
Reason: reasonSuccess,
Message: "the names specified by .spec.identityProviders[].transforms.constants[].name are unique",
})
}
return conditions
}
func appendIdentityProviderDuplicateDisplayNamesCondition(duplicateDisplayNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition {
if duplicateDisplayNames.Len() > 0 {
conditions = append(conditions, &configv1alpha1.Condition{
@@ -878,7 +841,6 @@ func (c *federationDomainWatcherController) sortedAllowedKinds() []string {
}
type transformsValidationErrorMessages struct {
errorsForConstants []string
errorsForExpressions []string
errorsForExamples []string
}

81
internal/controller/supervisorconfig/federation_domain_watcher_test.go
View File

@@ -400,28 +400,6 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
}
}
happyConstNamesUniqueCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
return configv1alpha1.Condition{
Type: "TransformsConstantsNamesUnique",
Status: "True",
ObservedGeneration: observedGeneration,
LastTransitionTime: time,
Reason: "Success",
Message: "the names specified by .spec.identityProviders[].transforms.constants[].name are unique",
}
}
sadConstNamesUniqueCondition := func(errorMessages string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
return configv1alpha1.Condition{
Type: "TransformsConstantsNamesUnique",
Status: "False",
ObservedGeneration: observedGeneration,
LastTransitionTime: time,
Reason: "DuplicateConstantsNames",
Message: errorMessages,
}
}
happyTransformationExpressionsCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
return configv1alpha1.Condition{
Type: "TransformsExpressionsValid",
@@ -537,7 +515,6 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
return sortConditionsByType([]configv1alpha1.Condition{
happyTransformationExamplesCondition(frozenMetav1Now, 123),
happyTransformationExpressionsCondition(frozenMetav1Now, 123),
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@@ -1341,55 +1318,6 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
),
},
},
{
name: "the federation domain has duplicate transformation const names",
inputObjects: []runtime.Object{
oidcIdentityProvider,
&configv1alpha1.FederationDomain{
ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
Spec: configv1alpha1.FederationDomainSpec{
Issuer: "https://issuer1.com",
IdentityProviders: []configv1alpha1.FederationDomainIdentityProvider{
{
DisplayName: "name1",
ObjectRef: corev1.TypedLocalObjectReference{
APIGroup: pointer.String(apiGroupSupervisor),
Kind: "OIDCIdentityProvider",
Name: oidcIdentityProvider.Name,
},
Transforms: configv1alpha1.FederationDomainTransforms{
Constants: []configv1alpha1.FederationDomainTransformsConstant{
{Name: "duplicate1", Type: "string", StringValue: "abc"},
{Name: "duplicate1", Type: "stringList", StringListValue: []string{"def"}},
{Name: "duplicate1", Type: "string", StringValue: "efg"},
{Name: "duplicate2", Type: "string", StringValue: "123"},
{Name: "duplicate2", Type: "string", StringValue: "456"},
{Name: "uniqueName", Type: "string", StringValue: "hij"},
},
},
},
},
},
},
},
wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{},
wantStatusUpdates: []*configv1alpha1.FederationDomain{
expectedFederationDomainStatusUpdate(
&configv1alpha1.FederationDomain{
ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
},
configv1alpha1.FederationDomainPhaseError,
replaceConditions(
allHappyConditionsSuccess("https://issuer1.com", frozenMetav1Now, 123),
[]configv1alpha1.Condition{
sadConstNamesUniqueCondition(
`the names specified by .spec.identityProviders[0].transforms.constants[].name contain duplicates: "duplicate1", "duplicate2"`,
frozenMetav1Now, 123),
sadReadyCondition(frozenMetav1Now, 123),
}),
),
},
},
{
name: "the federation domain has transformation expressions which don't compile",
inputObjects: []runtime.Object{
@@ -1707,7 +1635,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
Transforms: configv1alpha1.FederationDomainTransforms{
Constants: []configv1alpha1.FederationDomainTransformsConstant{
{Name: "foo", Type: "string", StringValue: "bar"},
{Name: "foo", Type: "string", StringValue: "baz"},
{Name: "bar", Type: "string", StringValue: "baz"},
},
Expressions: []configv1alpha1.FederationDomainTransformsExpression{
{Type: "username/v1", Expression: `username + ":suffix"`},
@@ -1742,7 +1670,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
Transforms: configv1alpha1.FederationDomainTransforms{
Constants: []configv1alpha1.FederationDomainTransformsConstant{
{Name: "foo", Type: "string", StringValue: "bar"},
{Name: "foo", Type: "string", StringValue: "baz"},
{Name: "bar", Type: "string", StringValue: "baz"},
},
Expressions: []configv1alpha1.FederationDomainTransformsExpression{
{Type: "username/v1", Expression: `username + ":suffix"`},
@@ -1821,11 +1749,6 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
replaceConditions(
allHappyConditionsSuccess("https://not-unique.com", frozenMetav1Now, 123),
[]configv1alpha1.Condition{
sadConstNamesUniqueCondition(here.Doc(
`the names specified by .spec.identityProviders[0].transforms.constants[].name contain duplicates: "foo"
the names specified by .spec.identityProviders[1].transforms.constants[].name contain duplicates: "foo"`,
), frozenMetav1Now, 123),
sadAPIGroupSuffixCondition(`"this is wrong"`, frozenMetav1Now, 123),
sadDisplayNamesUniqueCondition(`"not unique"`, frozenMetav1Now, 123),
sadIdentityProvidersFoundConditionIdentityProvidersObjectRefsNotFound(here.Doc(