Validate transforms const names in federation_domain_watcher.go

This commit is contained in:
Ryan Richard 2023-07-13 15:26:32 -07:00
parent 0aacedf943
commit 617f57e1c9
3 changed files with 165 additions and 5 deletions

View File

@ -43,6 +43,7 @@ const (
typeIdentityProvidersDisplayNamesUnique = "IdentityProvidersDisplayNamesUnique"
typeIdentityProvidersAPIGroupSuffixValid = "IdentityProvidersObjectRefAPIGroupSuffixValid"
typeIdentityProvidersObjectRefKindValid = "IdentityProvidersObjectRefKindValid"
typeTransformsConstantsNamesUnique = "TransformsConstantsNamesUnique"
reasonSuccess = "Success"
reasonNotReady = "NotReady"
@ -57,6 +58,7 @@ const (
reasonDuplicateDisplayNames = "DuplicateDisplayNames"
reasonAPIGroupNameUnrecognized = "APIGroupUnrecognized"
reasonKindUnrecognized = "KindUnrecognized"
reasonDuplicateConstantsNames = "DuplicateConstantsNames"
kindLDAPIdentityProvider = "LDAPIdentityProvider"
kindOIDCIdentityProvider = "OIDCIdentityProvider"
@ -318,6 +320,7 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer(
conditions = appendIdentityProviderDuplicateDisplayNamesCondition(sets.Set[string]{}, conditions)
conditions = appendIdentityProviderObjectRefAPIGroupSuffixCondition(c.apiGroup, []string{}, conditions)
conditions = appendIdentityProviderObjectRefKindCondition(c.sortedAllowedKinds(), []string{}, conditions)
conditions = appendTransformsConstantsNamesUniqueCondition(sets.Set[string]{}, conditions)
return federationDomainIssuer, conditions, nil
}
@ -374,7 +377,9 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic
idpNotFoundIndices = append(idpNotFoundIndices, index)
}
pipeline, err := c.makeTransformationPipelineForIdentityProvider(idp, federationDomain.Name)
var err error
var pipeline *idtransform.TransformationPipeline
pipeline, conditions, err = c.makeTransformationPipelineForIdentityProvider(idp, federationDomain.Name, conditions)
if err != nil {
return nil, nil, err
}
@ -455,15 +460,24 @@ func (c *federationDomainWatcherController) findIDPsUIDByObjectRef(objectRef cor
func (c *federationDomainWatcherController) makeTransformationPipelineForIdentityProvider(
idp configv1alpha1.FederationDomainIdentityProvider,
federationDomainName string,
) (*idtransform.TransformationPipeline, error) {
conditions []*configv1alpha1.Condition,
) (*idtransform.TransformationPipeline, []*configv1alpha1.Condition, error) {
pipeline := idtransform.NewTransformationPipeline()
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)
}
constNames.Insert(constant.Name)
switch constant.Type {
case "string":
consts.StringConstants[constant.Name] = constant.StringValue
@ -471,9 +485,10 @@ func (c *federationDomainWatcherController) makeTransformationPipelineForIdentit
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, nil, fmt.Errorf("one of spec.identityProvider[].transforms.constants[].type is invalid: %q", constant.Type)
}
}
conditions = appendTransformsConstantsNamesUniqueCondition(duplicateConstNames, conditions)
// Compile all the expressions and add them to the pipeline.
for idx, expr := range idp.Transforms.Expressions {
@ -490,7 +505,7 @@ func (c *federationDomainWatcherController) makeTransformationPipelineForIdentit
}
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.expressions[].type is invalid: %q", expr.Type)
return nil, nil, fmt.Errorf("one of spec.identityProvider[].transforms.expressions[].type is invalid: %q", expr.Type)
}
compiledTransform, err := c.celTransformer.CompileTransformation(rawTransform, consts)
if err != nil {
@ -580,7 +595,7 @@ func (c *federationDomainWatcherController) makeTransformationPipelineForIdentit
}
}
return pipeline, nil
return pipeline, conditions, nil
}
func (c *federationDomainWatcherController) sortedAllowedKinds() []string {
@ -647,6 +662,26 @@ func appendIdentityProviderDuplicateDisplayNamesCondition(duplicateDisplayNames
return conditions
}
func appendTransformsConstantsNamesUniqueCondition(duplicateConstNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition {
if duplicateConstNames.Len() > 0 {
conditions = append(conditions, &configv1alpha1.Condition{
Type: typeTransformsConstantsNamesUnique,
Status: configv1alpha1.ConditionFalse,
Reason: reasonDuplicateConstantsNames,
Message: fmt.Sprintf("the names specified by .spec.identityProviders[].transforms.constants[].name contain duplicates: %s",
strings.Join(sortAndQuote(duplicateConstNames.UnsortedList()), ", ")),
})
} 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 appendIssuerURLValidCondition(err error, conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition {
if err != nil {
// Note that the FederationDomainIssuer constructors only validate the Issuer URL,

View File

@ -397,6 +397,28 @@ 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(duplicateNames string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
return configv1alpha1.Condition{
Type: "TransformsConstantsNamesUnique",
Status: "False",
ObservedGeneration: observedGeneration,
LastTransitionTime: time,
Reason: "DuplicateConstantsNames",
Message: fmt.Sprintf("the names specified by .spec.identityProviders[].transforms.constants[].name contain duplicates: %s", duplicateNames),
}
}
happyAPIGroupSuffixCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
return configv1alpha1.Condition{
Type: "IdentityProvidersObjectRefAPIGroupSuffixValid",
@ -454,6 +476,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
allHappyConditionsLegacyConfigurationSuccess := func(issuer string, idpName string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition {
return sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -467,6 +490,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
allHappyConditionsSuccess := func(issuer string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition {
return sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -598,6 +622,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain,
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -644,6 +669,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain,
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -688,6 +714,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -704,6 +731,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -769,6 +797,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -785,6 +814,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -801,6 +831,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -831,6 +862,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
expectedFederationDomainStatusUpdate(federationDomain1,
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -844,6 +876,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
expectedFederationDomainStatusUpdate(federationDomain2,
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -869,6 +902,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
expectedFederationDomainStatusUpdate(federationDomain1,
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -925,6 +959,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -1037,6 +1072,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
Name: ldapIdentityProvider.Name,
},
},
{
DisplayName: "duplicate1",
ObjectRef: corev1.TypedLocalObjectReference{
APIGroup: pointer.String(apiGroupSupervisor),
Kind: "LDAPIdentityProvider",
Name: ldapIdentityProvider.Name,
},
},
{
DisplayName: "unique",
ObjectRef: corev1.TypedLocalObjectReference{
@ -1073,6 +1116,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
sadDisplayNamesUniqueCondition(`"duplicate1", "duplicate2"`, frozenMetav1Now, 123),
@ -1139,6 +1183,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
sadAPIGroupSuffixCondition(`"", "", "wrong.example.com"`, frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -1201,6 +1246,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
happyConstNamesUniqueCondition(frozenMetav1Now, 123),
sadKindCondition(`"", "wrong"`, frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
@ -1215,6 +1261,81 @@ 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: "string",
StringValue: "def",
},
{
Name: "duplicate1",
Type: "string",
StringValue: "efg",
},
{
Name: "duplicate2",
Type: "string",
StringValue: "123",
},
{
Name: "duplicate2",
Type: "string",
StringValue: "456",
},
{
Name: "unique",
Type: "string",
StringValue: "hij",
},
},
},
},
},
},
},
},
wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{},
wantStatusUpdates: []*configv1alpha1.FederationDomain{
expectedFederationDomainStatusUpdate(
&configv1alpha1.FederationDomain{
ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
},
configv1alpha1.FederationDomainPhaseError,
sortConditionsByType([]configv1alpha1.Condition{
sadConstNamesUniqueCondition(`"duplicate1", "duplicate2"`, frozenMetav1Now, 123),
happyKindCondition(frozenMetav1Now, 123),
happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123),
happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
happyIssuerURLValidCondition(frozenMetav1Now, 123),
happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123),
sadReadyCondition(frozenMetav1Now, 123),
})),
},
},
{
name: "the federation domain specifies illegal const type, which shouldn't really happen since the CRD validates it",
inputObjects: []runtime.Object{

View File

@ -141,6 +141,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
"IdentityProvidersObjectRefKindValid": v1alpha1.ConditionTrue,
"IdentityProvidersObjectRefAPIGroupSuffixValid": v1alpha1.ConditionTrue,
"IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue,
"TransformsConstantsNamesUnique": v1alpha1.ConditionTrue,
})
requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.FederationDomainPhaseError, map[string]v1alpha1.ConditionStatus{
"Ready": v1alpha1.ConditionFalse,
@ -151,6 +152,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
"IdentityProvidersObjectRefKindValid": v1alpha1.ConditionTrue,
"IdentityProvidersObjectRefAPIGroupSuffixValid": v1alpha1.ConditionTrue,
"IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue,
"TransformsConstantsNamesUnique": v1alpha1.ConditionTrue,
})
requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer6)
@ -178,6 +180,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
"IdentityProvidersObjectRefKindValid": v1alpha1.ConditionTrue,
"IdentityProvidersObjectRefAPIGroupSuffixValid": v1alpha1.ConditionTrue,
"IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue,
"TransformsConstantsNamesUnique": v1alpha1.ConditionTrue,
})
requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, badIssuer)
requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, badConfig, client, ns, scheme, addr, caBundle, badIssuer)
@ -694,6 +697,7 @@ func requireFullySuccessfulStatus(t *testing.T, client pinnipedclientset.Interfa
"IdentityProvidersObjectRefKindValid": v1alpha1.ConditionTrue,
"IdentityProvidersObjectRefAPIGroupSuffixValid": v1alpha1.ConditionTrue,
"IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue,
"TransformsConstantsNamesUnique": v1alpha1.ConditionTrue,
})
}