diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index 81e43800..e83b3f6e 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -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, diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index 8cad14e1..70339f9b 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher_test.go @@ -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{ diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 9e2b7807..5f7a93ad 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -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, }) }