Merge top-level IdP config traversal functions, check for improper IdP config

pkg/operator2/configsync.go

@@ -8,11 +8,10 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 
-	configv1 "github.com/openshift/api/config/v1"
 	"github.com/openshift/library-go/pkg/operator/resourcesynccontroller"
 )
 
-func (c *authOperator) handleConfigSync(config *configv1.OAuth) ([]idpSyncData, error) {
+func (c *authOperator) handleConfigSync(data *idpSyncData) error {
 	// TODO handle OAuthTemplates
 
 	// TODO we probably need listers
@@ -21,12 +20,12 @@ func (c *authOperator) handleConfigSync(config *configv1.OAuth) ([]idpSyncData,
 
 	configMaps, err := configMapClient.List(metav1.ListOptions{})
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	secrets, err := secretClient.List(metav1.ListOptions{})
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	prefixConfigMapNames := sets.NewString()
@@ -49,16 +48,13 @@ func (c *authOperator) handleConfigSync(config *configv1.OAuth) ([]idpSyncData,
 	inUseConfigMapNames := sets.NewString()
 	inUseSecretNames := sets.NewString()
 
-	data := convertToData(config.Spec.IdentityProviders)
-	for _, d := range data {
-		for dest, src := range d.configMaps {
-			syncOrDie(c.resourceSyncer.SyncConfigMap, dest, src.src)
-			inUseConfigMapNames.Insert(dest)
-		}
-		for dest, src := range d.secrets {
-			syncOrDie(c.resourceSyncer.SyncSecret, dest, src.src)
-			inUseSecretNames.Insert(dest)
-		}
+	for dest, src := range data.configMaps {
+		syncOrDie(c.resourceSyncer.SyncConfigMap, dest, src.src)
+		inUseConfigMapNames.Insert(dest)
+	}
+	for dest, src := range data.secrets {
+		syncOrDie(c.resourceSyncer.SyncSecret, dest, src.src)
+		inUseSecretNames.Insert(dest)
 	}
 
 	notInUseConfigMapNames := prefixConfigMapNames.Difference(inUseConfigMapNames)
@@ -74,7 +70,7 @@ func (c *authOperator) handleConfigSync(config *configv1.OAuth) ([]idpSyncData,
 		syncOrDie(c.resourceSyncer.SyncSecret, dest, "")
 	}
 
-	return data, nil
+	return nil
 }
 
 type idpSyncData struct {
@@ -91,7 +87,9 @@ type sourceData struct {
 	mount  corev1.VolumeMount
 }
 
-// TODO: new source data could be generalized
+// TODO: newSourceDataIDP* could be a generic function grouping the common pieces of code
+// newSourceDataIDPSecret returns a name which is unique amongst the IdPs, and
+// sourceData which describes the volumes and mountvolumes to mount the secret to
 func newSourceDataIDPSecret(index int, secretName, idpType string) (string, sourceData) {
 	dest := getIDPName(index, secretName, idpType)
 
@@ -106,6 +104,8 @@ func newSourceDataIDPSecret(index int, secretName, idpType string) (string, sour
 	return dest, ret
 }
 
+// newSourceDataIDPConfigMap returns a name which is unique amongst the IdPs, and
+// sourceData which describes the volumes and mountvolumes to mount the ConfigMap to
 func newSourceDataIDPConfigMap(index int, cmName, idpType string) (string, sourceData) {
 	dest := getIDPName(index, cmName, idpType)
 
@@ -120,165 +120,34 @@ func newSourceDataIDPConfigMap(index int, cmName, idpType string) (string, sourc
 	return dest, ret
 }
 
-// TODO this should be combined with convertProviderConfigToOsinBytes as it would simplify how the data is shared
-func convertToData(idps []configv1.IdentityProvider) []idpSyncData {
-	out := make([]idpSyncData, 0, len(idps))
-	for i, idp := range idps {
-		pc := idp.IdentityProviderConfig
-		switch pc.Type {
-		case configv1.IdentityProviderTypeBasicAuth:
-			p := pc.BasicAuth
-
-			ca := p.CA.Name
-			caDest, caData := newSourceDataIDPConfigMap(i, ca, corev1.ServiceAccountRootCAKey)
-
-			clientCert := p.TLSClientCert.Name
-			clientCertDest, clientCertData := newSourceDataIDPSecret(i, clientCert, corev1.TLSCertKey)
-
-			clientKey := p.TLSClientKey.Name
-			clientKeyDest, clienKeyData := newSourceDataIDPSecret(i, clientKey, corev1.TLSPrivateKeyKey)
-
-			out = append(out,
-				idpSyncData{
-					configMaps: map[string]sourceData{caDest: caData},
-					secrets: map[string]sourceData{
-						clientCertDest: clientCertData,
-						clientKeyDest:  clienKeyData,
-					},
-				},
-			)
-
-		case configv1.IdentityProviderTypeGitHub:
-			p := pc.GitHub
-			ca := p.CA.Name
-			caDest, caData := newSourceDataIDPConfigMap(i, ca, corev1.ServiceAccountRootCAKey)
-
-			clientSecret := p.ClientSecret.Name
-			clientSecretDest, clientSecretData := newSourceDataIDPSecret(i, clientSecret, configv1.ClientSecretKey)
-
-			out = append(out,
-				idpSyncData{
-					configMaps: map[string]sourceData{caDest: caData},
-					secrets: map[string]sourceData{
-						clientSecretDest: clientSecretData,
-					},
-				},
-			)
-
-		case configv1.IdentityProviderTypeGitLab:
-			p := pc.GitLab
-			ca := p.CA.Name
-			caDest, caData := newSourceDataIDPConfigMap(i, ca, corev1.ServiceAccountRootCAKey)
-
-			clientSecret := p.ClientSecret.Name
-			clientSecretDest, clientSecretData := newSourceDataIDPSecret(i, clientSecret, configv1.ClientSecretKey)
-
-			out = append(out,
-				idpSyncData{
-					configMaps: map[string]sourceData{caDest: caData},
-					secrets: map[string]sourceData{
-						clientSecretDest: clientSecretData,
-					},
-				},
-			)
-
-		case configv1.IdentityProviderTypeGoogle:
-			p := pc.Google
-
-			clientSecret := p.ClientSecret.Name
-			clientSecretDest, clientSecretData := newSourceDataIDPSecret(i, clientSecret, configv1.ClientSecretKey)
-
-			out = append(out,
-				idpSyncData{
-					secrets: map[string]sourceData{
-						clientSecretDest: clientSecretData,
-					},
-				},
-			)
-
-		case configv1.IdentityProviderTypeHTPasswd:
-			p := pc.HTPasswd // TODO could panic if invalid (applies to all IDPs)
-
-			fileData := p.FileData.Name
-			dest, data := newSourceDataIDPSecret(i, fileData, configv1.HTPasswdDataKey)
-
-			out = append(out,
-				idpSyncData{secrets: map[string]sourceData{dest: data}},
-			)
+func newIDPSyncData() idpSyncData {
+	configMaps := map[string]sourceData{}
+	secrets := map[string]sourceData{}
 
-		case configv1.IdentityProviderTypeKeystone:
-			p := pc.Keystone
-
-			ca := p.CA.Name
-			caDest, caData := newSourceDataIDPConfigMap(i, ca, corev1.ServiceAccountRootCAKey)
-
-			clientCert := p.TLSClientCert.Name
-			clientCertDest, clientCertData := newSourceDataIDPSecret(i, clientCert, corev1.TLSCertKey)
-
-			clientKey := p.TLSClientKey.Name
-			clientKeyDest, clienKeyData := newSourceDataIDPSecret(i, clientKey, corev1.TLSPrivateKeyKey)
-
-			out = append(out,
-				idpSyncData{
-					configMaps: map[string]sourceData{caDest: caData},
-					secrets: map[string]sourceData{
-						clientCertDest: clientCertData,
-						clientKeyDest:  clienKeyData,
-					},
-				},
-			)
-
-		case configv1.IdentityProviderTypeLDAP:
-			p := pc.LDAP
-
-			ca := p.CA.Name
-			caDest, caData := newSourceDataIDPConfigMap(i, ca, corev1.ServiceAccountRootCAKey)
-
-			bindPassword := p.BindPassword.Name
-			bindPasswordDest, bindPasswordData := newSourceDataIDPSecret(i, bindPassword, configv1.BindPasswordKey)
-
-			out = append(out,
-				idpSyncData{
-					configMaps: map[string]sourceData{caDest: caData},
-					secrets:    map[string]sourceData{bindPasswordDest: bindPasswordData},
-				},
-			)
-
-		case configv1.IdentityProviderTypeOpenID:
-			p := pc.OpenID
-
-			ca := p.CA.Name
-			caDest, caData := newSourceDataIDPConfigMap(i, ca, corev1.ServiceAccountRootCAKey)
-
-			clientSecret := p.ClientSecret.Name
-			clientSecretDest, clientSecretData := newSourceDataIDPSecret(i, clientSecret, configv1.ClientSecretKey)
-
-			out = append(out,
-				idpSyncData{
-					configMaps: map[string]sourceData{caDest: caData},
-					secrets: map[string]sourceData{
-						clientSecretDest: clientSecretData,
-					},
-				},
-			)
+	return idpSyncData{
+		configMaps: configMaps,
+		secrets:    secrets,
+	}
+}
 
-		case configv1.IdentityProviderTypeRequestHeader:
-			p := pc.RequestHeader
+// AddSecret initializes a sourceData object with proper data for a Secret
+// and adds it among the other secrets stored here
+// Returns the key that it stored the Secret to
+func (sd *idpSyncData) AddSecret(index int, secretName, idpType string) string {
+	dest, data := newSourceDataIDPSecret(index, secretName, idpType)
+	sd.secrets[dest] = data
 
-			clientCA := p.ClientCA.Name
-			clientCADest, clientCAData := newSourceDataIDPConfigMap(i, clientCA, corev1.ServiceAccountRootCAKey)
+	return dest
+}
 
-			out = append(out,
-				idpSyncData{
-					configMaps: map[string]sourceData{clientCADest: clientCAData},
-				},
-			)
+// AddConfigMap initializes a sourceData object with proper data for a ConfigMap
+// and adds it among the other configmaps stored here
+// Returns the key that it stored the ConfigMap to
+func (sd *idpSyncData) AddConfigMap(index int, secretName, idpType string) string {
+	dest, data := newSourceDataIDPConfigMap(index, secretName, idpType)
+	sd.secrets[dest] = data
 
-		default:
-			return nil // TODO: some erroring
-		}
-	}
-	return out
+	return dest
 }
 
 const (

pkg/operator2/deployment.go

@@ -26,8 +26,12 @@ func (c *authOperator) getGeneration() int64 {
 	return deployment.Generation
 }
 
-func defaultDeployment(operatorConfig *authv1alpha1.AuthenticationOperatorConfig, syncData []idpSyncData, resourceVersions ...string) *appsv1.Deployment {
-	replicas := int32(3)
+func defaultDeployment(
+	operatorConfig *authv1alpha1.AuthenticationOperatorConfig,
+	syncData *idpSyncData,
+	resourceVersions ...string,
+) *appsv1.Deployment {
+	replicas := int32(3) // TODO configurable?
 	gracePeriod := int64(30)
 
 	var (
@@ -66,10 +70,8 @@ func defaultDeployment(operatorConfig *authv1alpha1.AuthenticationOperatorConfig
 		mounts = append(mounts, m)
 	}
 
-	for _, d := range syncData {
-		volumes, mounts = toVolumesAndMounts(d.configMaps, volumes, mounts)
-		volumes, mounts = toVolumesAndMounts(d.secrets, volumes, mounts)
-	}
+	volumes, mounts = toVolumesAndMounts(syncData.configMaps, volumes, mounts)
+	volumes, mounts = toVolumesAndMounts(syncData.secrets, volumes, mounts)
 
 	// force redeploy when any associated resource changes
 	// we use a hash to prevent this value from growing indefinitely

pkg/operator2/helpers.go

@@ -2,20 +2,10 @@ package operator2
 
 import configv1 "github.com/openshift/api/config/v1"
 
-func moveSecretFromRefToFileStringSource(syncData []idpSyncData, i int, name configv1.SecretNameReference, key string) configv1.StringSource {
+func createFileStringSource(filename string) configv1.StringSource {
 	return configv1.StringSource{
 		StringSourceSpec: configv1.StringSourceSpec{
-			File: getFilenameFromSecretNameRef(syncData, i, name, key),
+			File: filename,
 		},
 	}
 }
-
-func getFilenameFromConfigMapNameRef(syncData []idpSyncData, i int, name configv1.ConfigMapNameReference, key string) string {
-	// TODO make sure this makes sense (some things are optional)
-	return syncData[i].configMaps[getIDPName(i, name.Name, key)].path
-}
-
-func getFilenameFromSecretNameRef(syncData []idpSyncData, i int, name configv1.SecretNameReference, key string) string {
-	// TODO make sure this makes sense (some things are optional)
-	return syncData[i].secrets[getIDPName(i, name.Name, key)].path
-}