diff --git a/pkg/operator2/configsync.go b/pkg/operator2/configsync.go index 02bcacccb5..6bef97de73 100644 --- a/pkg/operator2/configsync.go +++ b/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 ( diff --git a/pkg/operator2/deployment.go b/pkg/operator2/deployment.go index 96b9b5fa59..9ba3e80172 100644 --- a/pkg/operator2/deployment.go +++ b/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 diff --git a/pkg/operator2/helpers.go b/pkg/operator2/helpers.go index 776d710633..0a57514db3 100644 --- a/pkg/operator2/helpers.go +++ b/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 -} diff --git a/pkg/operator2/idp.go b/pkg/operator2/idp.go index 51dff80b63..0c106e1e3c 100644 --- a/pkg/operator2/idp.go +++ b/pkg/operator2/idp.go @@ -22,67 +22,91 @@ func init() { utilruntime.Must(osinv1.Install(scheme)) } -func convertProviderConfigToOsinBytes(providerConfig *configv1.IdentityProviderConfig, syncData []idpSyncData, i int) ([]byte, error) { - // FIXME: we need validation to make sure each of the IdP fields in each case is not nil! +func convertProviderConfigToOsinBytes(providerConfig *configv1.IdentityProviderConfig, syncData *idpSyncData, i int) ([]byte, error) { + const missingProviderFmt string = "type %s was specified, but its configuration is missing" var p runtime.Object switch providerConfig.Type { case configv1.IdentityProviderTypeBasicAuth: basicAuthConfig := providerConfig.BasicAuth + if basicAuthConfig == nil { + return nil, fmt.Errorf(missingProviderFmt, providerConfig.Type) + } + p = &osinv1.BasicAuthPasswordIdentityProvider{ RemoteConnectionInfo: configv1.RemoteConnectionInfo{ URL: basicAuthConfig.URL, - CA: getFilenameFromConfigMapNameRef(syncData, i, basicAuthConfig.CA, corev1.ServiceAccountRootCAKey), + CA: syncData.AddConfigMap(i, basicAuthConfig.CA.Name, corev1.ServiceAccountRootCAKey), CertInfo: configv1.CertInfo{ - CertFile: getFilenameFromSecretNameRef(syncData, i, basicAuthConfig.TLSClientCert, corev1.TLSCertKey), - KeyFile: getFilenameFromSecretNameRef(syncData, i, basicAuthConfig.TLSClientKey, corev1.TLSPrivateKeyKey), + CertFile: syncData.AddSecret(i, basicAuthConfig.TLSClientCert.Name, corev1.TLSCertKey), + KeyFile: syncData.AddSecret(i, basicAuthConfig.TLSClientKey.Name, corev1.TLSPrivateKeyKey), }, }, } case configv1.IdentityProviderTypeGitHub: githubConfig := providerConfig.GitHub + if githubConfig == nil { + return nil, fmt.Errorf(missingProviderFmt, providerConfig.Type) + } + p = &osinv1.GitHubIdentityProvider{ ClientID: githubConfig.ClientID, - ClientSecret: moveSecretFromRefToFileStringSource(syncData, i, githubConfig.ClientSecret, configv1.ClientSecretKey), + ClientSecret: createFileStringSource(syncData.AddSecret(i, githubConfig.ClientSecret.Name, configv1.ClientSecretKey)), Organizations: githubConfig.Organizations, Hostname: githubConfig.Hostname, - CA: getFilenameFromConfigMapNameRef(syncData, i, githubConfig.CA, corev1.ServiceAccountRootCAKey), + CA: syncData.AddConfigMap(i, githubConfig.CA.Name, corev1.ServiceAccountRootCAKey), } case configv1.IdentityProviderTypeGitLab: gitlabConfig := providerConfig.GitLab + if gitlabConfig == nil { + return nil, fmt.Errorf(missingProviderFmt, providerConfig.Type) + } + p = &osinv1.GitLabIdentityProvider{ - CA: getFilenameFromConfigMapNameRef(syncData, i, gitlabConfig.CA, corev1.ServiceAccountRootCAKey), + CA: syncData.AddConfigMap(i, gitlabConfig.CA.Name, corev1.ServiceAccountRootCAKey), URL: gitlabConfig.URL, ClientID: gitlabConfig.ClientID, - ClientSecret: moveSecretFromRefToFileStringSource(syncData, i, gitlabConfig.ClientSecret, configv1.ClientSecretKey), + ClientSecret: createFileStringSource(syncData.AddSecret(i, gitlabConfig.ClientSecret.Name, configv1.ClientSecretKey)), Legacy: new(bool), // we require OIDC for GitLab now } case configv1.IdentityProviderTypeGoogle: googleConfig := providerConfig.Google + if googleConfig == nil { + return nil, fmt.Errorf(missingProviderFmt, providerConfig.Type) + } + p = &osinv1.GoogleIdentityProvider{ ClientID: googleConfig.ClientID, - ClientSecret: moveSecretFromRefToFileStringSource(syncData, i, googleConfig.ClientSecret, configv1.ClientSecretKey), + ClientSecret: createFileStringSource(syncData.AddSecret(i, googleConfig.ClientSecret.Name, configv1.ClientSecretKey)), HostedDomain: googleConfig.HostedDomain, } case configv1.IdentityProviderTypeHTPasswd: + if providerConfig.HTPasswd == nil { + return nil, fmt.Errorf(missingProviderFmt, providerConfig.Type) + } + p = &osinv1.HTPasswdPasswordIdentityProvider{ - File: getFilenameFromSecretNameRef(syncData, i, providerConfig.HTPasswd.FileData, configv1.HTPasswdDataKey), + File: syncData.AddSecret(i, providerConfig.HTPasswd.FileData.Name, configv1.HTPasswdDataKey), } case configv1.IdentityProviderTypeKeystone: keystoneConfig := providerConfig.Keystone + if keystoneConfig == nil { + return nil, fmt.Errorf(missingProviderFmt, providerConfig.Type) + } + p = &osinv1.KeystonePasswordIdentityProvider{ RemoteConnectionInfo: configv1.RemoteConnectionInfo{ URL: keystoneConfig.URL, - CA: getFilenameFromConfigMapNameRef(syncData, i, keystoneConfig.CA, corev1.ServiceAccountRootCAKey), + CA: syncData.AddConfigMap(i, keystoneConfig.CA.Name, corev1.ServiceAccountRootCAKey), CertInfo: configv1.CertInfo{ - CertFile: getFilenameFromSecretNameRef(syncData, i, keystoneConfig.TLSClientCert, corev1.TLSCertKey), - KeyFile: getFilenameFromSecretNameRef(syncData, i, keystoneConfig.TLSClientKey, corev1.TLSPrivateKeyKey), + CertFile: syncData.AddSecret(i, keystoneConfig.TLSClientCert.Name, corev1.TLSCertKey), + KeyFile: syncData.AddSecret(i, keystoneConfig.TLSClientKey.Name, corev1.TLSPrivateKeyKey), }, }, DomainName: keystoneConfig.DomainName, @@ -91,20 +115,28 @@ func convertProviderConfigToOsinBytes(providerConfig *configv1.IdentityProviderC case configv1.IdentityProviderTypeLDAP: ldapConfig := providerConfig.LDAP + if ldapConfig == nil { + return nil, fmt.Errorf(missingProviderFmt, providerConfig.Type) + } + p = &osinv1.LDAPPasswordIdentityProvider{ URL: ldapConfig.URL, BindDN: ldapConfig.BindDN, - BindPassword: moveSecretFromRefToFileStringSource(syncData, i, ldapConfig.BindPassword, configv1.BindPasswordKey), + BindPassword: createFileStringSource(syncData.AddSecret(i, ldapConfig.BindPassword.Name, configv1.BindPasswordKey)), Insecure: ldapConfig.Insecure, - CA: getFilenameFromConfigMapNameRef(syncData, i, ldapConfig.CA, corev1.ServiceAccountRootCAKey), + CA: syncData.AddConfigMap(i, ldapConfig.CA.Name, corev1.ServiceAccountRootCAKey), } case configv1.IdentityProviderTypeOpenID: openIDConfig := providerConfig.OpenID + if openIDConfig == nil { + return nil, fmt.Errorf(missingProviderFmt, providerConfig.Type) + } + p = &osinv1.OpenIDIdentityProvider{ - CA: getFilenameFromConfigMapNameRef(syncData, i, openIDConfig.CA, corev1.ServiceAccountRootCAKey), + CA: syncData.AddConfigMap(i, openIDConfig.CA.Name, corev1.ServiceAccountRootCAKey), ClientID: openIDConfig.ClientID, - ClientSecret: moveSecretFromRefToFileStringSource(syncData, i, openIDConfig.ClientSecret, configv1.ClientSecretKey), + ClientSecret: createFileStringSource(syncData.AddSecret(i, openIDConfig.ClientSecret.Name, configv1.ClientSecretKey)), ExtraScopes: openIDConfig.ExtraScopes, ExtraAuthorizeParameters: openIDConfig.ExtraAuthorizeParameters, URLs: osinv1.OpenIDURLs{ @@ -123,10 +155,14 @@ func convertProviderConfigToOsinBytes(providerConfig *configv1.IdentityProviderC case configv1.IdentityProviderTypeRequestHeader: requestHeaderConfig := providerConfig.RequestHeader + if requestHeaderConfig == nil { + return nil, fmt.Errorf(missingProviderFmt, providerConfig.Type) + } + p = &osinv1.RequestHeaderIdentityProvider{ LoginURL: requestHeaderConfig.LoginURL, ChallengeURL: requestHeaderConfig.ChallengeURL, - ClientCA: getFilenameFromConfigMapNameRef(syncData, i, requestHeaderConfig.ClientCA, corev1.ServiceAccountRootCAKey), + ClientCA: syncData.AddConfigMap(i, requestHeaderConfig.ClientCA.Name, corev1.ServiceAccountRootCAKey), ClientCommonNames: requestHeaderConfig.ClientCommonNames, Headers: requestHeaderConfig.Headers, PreferredUsernameHeaders: requestHeaderConfig.PreferredUsernameHeaders, diff --git a/pkg/operator2/oauth.go b/pkg/operator2/oauth.go index 0529804e92..8d69fa556c 100644 --- a/pkg/operator2/oauth.go +++ b/pkg/operator2/oauth.go @@ -31,7 +31,17 @@ func init() { utilruntime.Must(kubecontrolplanev1.Install(kubeControlplaneScheme)) } -func (c *authOperator) handleOAuthConfig(operatorConfig *authv1alpha1.AuthenticationOperatorConfig, route *routev1.Route, service *corev1.Service) (*configv1.OAuth, *configv1.Console, *corev1.ConfigMap, []idpSyncData, error) { +func (c *authOperator) handleOAuthConfig( + operatorConfig *authv1alpha1.AuthenticationOperatorConfig, + route *routev1.Route, + service *corev1.Service, +) ( + *configv1.OAuth, + *configv1.Console, + *corev1.ConfigMap, + *idpSyncData, + error, +) { oauthConfig, err := c.oauth.Get(globalConfigName, metav1.GetOptions{}) if err != nil { return nil, nil, nil, nil, err @@ -44,11 +54,6 @@ func (c *authOperator) handleOAuthConfig(operatorConfig *authv1alpha1.Authentica consoleConfig = &configv1.Console{} } - syncData, err := c.handleConfigSync(oauthConfig) - if err != nil { - return nil, nil, nil, nil, err - } - var accessTokenInactivityTimeoutSeconds *int32 timeout := oauthConfig.Spec.TokenConfig.AccessTokenInactivityTimeoutSeconds switch { @@ -72,8 +77,9 @@ func (c *authOperator) handleOAuthConfig(operatorConfig *authv1alpha1.Authentica } identityProviders := make([]osinv1.IdentityProvider, 0, len(oauthConfig.Spec.IdentityProviders)) + syncData := newIDPSyncData() for i, idp := range oauthConfig.Spec.IdentityProviders { - providerConfigBytes, err := convertProviderConfigToOsinBytes(&idp.IdentityProviderConfig, syncData, i) + providerConfigBytes, err := convertProviderConfigToOsinBytes(&idp.IdentityProviderConfig, &syncData, i) if err != nil { glog.Error(err) continue @@ -96,6 +102,12 @@ func (c *authOperator) handleOAuthConfig(operatorConfig *authv1alpha1.Authentica } } + // TODO maybe move the OAuth stuff up one level + err = c.handleConfigSync(&syncData) + if err != nil { + return nil, nil, nil, nil, err + } + // TODO this pretends this is an OsinServerConfig cliConfig := &kubecontrolplanev1.KubeAPIServerConfig{ GenericAPIServerConfig: configv1.GenericAPIServerConfig{ @@ -164,7 +176,7 @@ func (c *authOperator) handleOAuthConfig(operatorConfig *authv1alpha1.Authentica } // TODO update OAuth status - return oauthConfig, consoleConfig, getCliConfigMap(completeConfigBytes), syncData, nil + return oauthConfig, consoleConfig, getCliConfigMap(completeConfigBytes), &syncData, nil } func getCliConfigMap(completeConfigBytes []byte) *corev1.ConfigMap {