From 65dc0882c9bb4496f1c4b2e0deb730e775724c82 Mon Sep 17 00:00:00 2001 From: Alex Collins Date: Fri, 1 Apr 2022 08:42:44 -0700 Subject: [PATCH] feat: Fail on invalid config. (#8295) Signed-off-by: Alex Collins --- config/config.go | 3 ++ config/controller.go | 16 +++++----- config/controller_test.go | 4 +-- config/rbac.go | 9 ++++++ config/sso.go | 31 +++++++++++++++++++ server/apiserver/argoserver.go | 6 ++-- server/apiserver/config.go | 12 ------- server/auth/rbac/config.go | 9 ------ server/auth/sso/sso.go | 30 +++--------------- test/e2e/fixtures/e2e_suite.go | 3 +- .../mixins/workflow-controller-configmap.yaml | 1 - workflow/controller/controller.go | 3 +- 12 files changed, 64 insertions(+), 63 deletions(-) create mode 100644 config/rbac.go create mode 100644 config/sso.go delete mode 100644 server/apiserver/config.go delete mode 100644 server/auth/rbac/config.go diff --git a/config/config.go b/config/config.go index e79e7666df53..5fe7fa14942c 100644 --- a/config/config.go +++ b/config/config.go @@ -105,6 +105,9 @@ type Config struct { // NavColor is an ui navigation bar background color NavColor string `json:"navColor,omitempty"` + + // SSO in settings for single-sign on + SSO SSOConfig `json:"sso,omitempty"` } func (c Config) GetContainerRuntimeExecutor(labels labels.Labels) (string, error) { diff --git a/config/controller.go b/config/controller.go index 3b569a521337..490ae3c37bdd 100644 --- a/config/controller.go +++ b/config/controller.go @@ -7,14 +7,13 @@ import ( log "github.com/sirupsen/logrus" apiv1 "k8s.io/api/core/v1" - apierr "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" "sigs.k8s.io/yaml" ) type Controller interface { - Get(context.Context, interface{}) error + Get(context.Context) (*Config, error) } type controller struct { @@ -33,7 +32,7 @@ func NewController(namespace, name string, kubeclientset kubernetes.Interface) C } } -func parseConfigMap(cm *apiv1.ConfigMap, config interface{}) error { +func parseConfigMap(cm *apiv1.ConfigMap, config *Config) error { // The key in the configmap to retrieve workflow configuration from. // Content encoding is expected to be YAML. rawConfig, ok := cm.Data["config"] @@ -50,15 +49,16 @@ func parseConfigMap(cm *apiv1.ConfigMap, config interface{}) error { } } } - err := yaml.Unmarshal([]byte(rawConfig), config) + err := yaml.UnmarshalStrict([]byte(rawConfig), config) return err } -func (cc *controller) Get(ctx context.Context, config interface{}) error { +func (cc *controller) Get(ctx context.Context) (*Config, error) { + config := &Config{} cmClient := cc.kubeclientset.CoreV1().ConfigMaps(cc.namespace) cm, err := cmClient.Get(ctx, cc.configMap, metav1.GetOptions{}) - if err != nil && !apierr.IsNotFound(err) { - return err + if err != nil { + return nil, err } - return parseConfigMap(cm, config) + return config, parseConfigMap(cm, config) } diff --git a/config/controller_test.go b/config/controller_test.go index 123c658ddbcb..d1ce72b5f611 100644 --- a/config/controller_test.go +++ b/config/controller_test.go @@ -38,9 +38,9 @@ func Test_parseConfigMap(t *testing.T) { assert.NotEmpty(t, c.ArtifactRepository) } }) - t.Run("IgnoreGarbage", func(t *testing.T) { + t.Run("Garbage", func(t *testing.T) { c := &Config{} err := parseConfigMap(&apiv1.ConfigMap{Data: map[string]string{"garbage": "garbage"}}, c) - assert.NoError(t, err) + assert.Error(t, err) }) } diff --git a/config/rbac.go b/config/rbac.go new file mode 100644 index 000000000000..8cdf3e8d3250 --- /dev/null +++ b/config/rbac.go @@ -0,0 +1,9 @@ +package config + +type RBACConfig struct { + Enabled bool `json:"enabled,omitempty"` +} + +func (c *RBACConfig) IsEnabled() bool { + return c != nil && c.Enabled +} diff --git a/config/sso.go b/config/sso.go new file mode 100644 index 000000000000..4c1a18254e2f --- /dev/null +++ b/config/sso.go @@ -0,0 +1,31 @@ +package config + +import ( + "time" + + apiv1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +type SSOConfig struct { + Issuer string `json:"issuer"` + IssuerAlias string `json:"issuerAlias,omitempty"` + ClientID apiv1.SecretKeySelector `json:"clientId"` + ClientSecret apiv1.SecretKeySelector `json:"clientSecret"` + RedirectURL string `json:"redirectUrl"` + RBAC *RBACConfig `json:"rbac,omitempty"` + // additional scopes (on top of "openid") + Scopes []string `json:"scopes,omitempty"` + SessionExpiry metav1.Duration `json:"sessionExpiry,omitempty"` + // customGroupClaimName will override the groups claim name + CustomGroupClaimName string `json:"customGroupClaimName,omitempty"` + UserInfoPath string `json:"userInfoPath,omitempty"` + InsecureSkipVerify bool `json:"insecureSkipVerify,omitempty"` +} + +func (c SSOConfig) GetSessionExpiry() time.Duration { + if c.SessionExpiry.Duration > 0 { + return c.SessionExpiry.Duration + } + return 10 * time.Hour +} diff --git a/server/apiserver/argoserver.go b/server/apiserver/argoserver.go index e5c7c8ae09e5..8792b80856a6 100644 --- a/server/apiserver/argoserver.go +++ b/server/apiserver/argoserver.go @@ -127,8 +127,7 @@ func NewArgoServer(ctx context.Context, opts ArgoServerOpts) (*argoServer, error var resourceCache *cache.ResourceCache = nil ssoIf := sso.NullSSO if opts.AuthModes[auth.SSO] { - c := &Config{} - err := configController.Get(ctx, c) + c, err := configController.Get(ctx) if err != nil { return nil, err } @@ -173,8 +172,7 @@ var backoff = wait.Backoff{ } func (as *argoServer) Run(ctx context.Context, port int, browserOpenFunc func(string)) { - config := &Config{} - err := as.configController.Get(ctx, config) + config, err := as.configController.Get(ctx) if err != nil { log.Fatal(err) } diff --git a/server/apiserver/config.go b/server/apiserver/config.go deleted file mode 100644 index 9913233e6c0e..000000000000 --- a/server/apiserver/config.go +++ /dev/null @@ -1,12 +0,0 @@ -package apiserver - -import ( - "github.com/argoproj/argo-workflows/v3/config" - "github.com/argoproj/argo-workflows/v3/server/auth/sso" -) - -type Config struct { - config.Config - // SSO in settings for single-sign on - SSO sso.Config `json:"sso,omitempty"` -} diff --git a/server/auth/rbac/config.go b/server/auth/rbac/config.go deleted file mode 100644 index 51b7534f5104..000000000000 --- a/server/auth/rbac/config.go +++ /dev/null @@ -1,9 +0,0 @@ -package rbac - -type Config struct { - Enabled bool `json:"enabled,omitempty"` -} - -func (c *Config) IsEnabled() bool { - return c != nil && c.Enabled -} diff --git a/server/auth/sso/sso.go b/server/auth/sso/sso.go index 70591a5a6063..efa9f5e67463 100644 --- a/server/auth/sso/sso.go +++ b/server/auth/sso/sso.go @@ -12,6 +12,8 @@ import ( "strings" "time" + "github.com/argoproj/argo-workflows/v3/config" + pkgrand "github.com/argoproj/pkg/rand" "github.com/coreos/go-oidc/v3/oidc" "github.com/go-jose/go-jose/v3" @@ -23,7 +25,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" - "github.com/argoproj/argo-workflows/v3/server/auth/rbac" "github.com/argoproj/argo-workflows/v3/server/auth/types" ) @@ -45,6 +46,8 @@ type Interface interface { var _ Interface = &sso{} +type Config = config.SSOConfig + type sso struct { config *oauth2.Config issuer string @@ -54,7 +57,7 @@ type sso struct { secure bool privateKey crypto.PrivateKey encrypter jose.Encrypter - rbacConfig *rbac.Config + rbacConfig *config.RBACConfig expiry time.Duration customClaimName string userInfoPath string @@ -64,29 +67,6 @@ func (s *sso) IsRBACEnabled() bool { return s.rbacConfig.IsEnabled() } -type Config struct { - Issuer string `json:"issuer"` - IssuerAlias string `json:"issuerAlias,omitempty"` - ClientID apiv1.SecretKeySelector `json:"clientId"` - ClientSecret apiv1.SecretKeySelector `json:"clientSecret"` - RedirectURL string `json:"redirectUrl"` - RBAC *rbac.Config `json:"rbac,omitempty"` - // additional scopes (on top of "openid") - Scopes []string `json:"scopes,omitempty"` - SessionExpiry metav1.Duration `json:"sessionExpiry,omitempty"` - // customGroupClaimName will override the groups claim name - CustomGroupClaimName string `json:"customGroupClaimName,omitempty"` - UserInfoPath string `json:"userInfoPath,omitempty"` - InsecureSkipVerify bool `json:"insecureSkipVerify,omitempty"` -} - -func (c Config) GetSessionExpiry() time.Duration { - if c.SessionExpiry.Duration > 0 { - return c.SessionExpiry.Duration - } - return 10 * time.Hour -} - // Abstract methods of oidc.Provider that our code uses into an interface. That // will allow us to implement a stub for unit testing. If you start using more // oidc.Provider methods in this file, add them here and provide a stub diff --git a/test/e2e/fixtures/e2e_suite.go b/test/e2e/fixtures/e2e_suite.go index 7734bcc8b753..584ecfa13ded 100644 --- a/test/e2e/fixtures/e2e_suite.go +++ b/test/e2e/fixtures/e2e_suite.go @@ -63,8 +63,9 @@ func (s *E2ESuite) SetupSuite() { configController := config.NewController(Namespace, common.ConfigMapName, s.KubeClient) ctx := context.Background() - err = configController.Get(ctx, &s.Config) + c, err := configController.Get(ctx) s.CheckError(err) + s.Config = c s.wfClient = versioned.NewForConfigOrDie(s.RestConfig).ArgoprojV1alpha1().Workflows(Namespace) s.wfebClient = versioned.NewForConfigOrDie(s.RestConfig).ArgoprojV1alpha1().WorkflowEventBindings(Namespace) s.wfTemplateClient = versioned.NewForConfigOrDie(s.RestConfig).ArgoprojV1alpha1().WorkflowTemplates(Namespace) diff --git a/test/e2e/manifests/mixins/workflow-controller-configmap.yaml b/test/e2e/manifests/mixins/workflow-controller-configmap.yaml index 8501ec97f48f..314d9311bd63 100644 --- a/test/e2e/manifests/mixins/workflow-controller-configmap.yaml +++ b/test/e2e/manifests/mixins/workflow-controller-configmap.yaml @@ -23,4 +23,3 @@ data: completed: 10 failed: 2 errored: 2 - kubeletInsecure: "true" diff --git a/workflow/controller/controller.go b/workflow/controller/controller.go index f29060d5d459..439a31772d46 100644 --- a/workflow/controller/controller.go +++ b/workflow/controller/controller.go @@ -421,10 +421,11 @@ func (wfc *WorkflowController) createClusterWorkflowTemplateInformer(ctx context } func (wfc *WorkflowController) UpdateConfig(ctx context.Context) { - err := wfc.configController.Get(ctx, &wfc.Config) + c, err := wfc.configController.Get(ctx) if err != nil { log.Fatalf("Failed to register watch for controller config map: %v", err) } + wfc.Config = *c err = wfc.updateConfig() if err != nil { log.Fatalf("Failed to update config: %v", err)