polish pkg/runctl/cfg (#181)

- do not exclude the default network policy from
  `PipelineRunsConfigStruct.NetworkPolicies`

- do not trim network policy values (could invalidate correct original
  values, e.g. if all lines are indented)

- `LoadPipelineRunsConfig`:

    - fail if the network policies config map does not exist (having no
      network policy would forbid any network traffic for pipeline
      run pods, which in turn would always fail)

- more precise error messages

- a lot of refactoring

PipelineRunsConfigStruct: default network profile: store key instead of value

In `PipelineRunsConfigStruct` the default network policy
is stored twice:

- as entry in field `NetworkPolicies` (where it belongs to)
- in field `DefaultNetworkPolicy`

The name of the default policy is not contained at all.

Replace field `DefaultNetworkPolicy` by `DefaultNetworkProfile`, which
contains the key of the default network policy to be found in
`NetworkProfiles`.

Consumers now have both the name of the default network profile as well
as all network profiles available.

changelog.yaml

@@ -1,27 +1,26 @@
 ## -----------------------------------------
 ## Copy this template for new change entries
 ## -----------------------------------------
-#    - type: [bug, enhancement, internal]
-#      impact: [incompatible, minor, patch]
-#      title: <title>
-#      description: |-
-#        <description (markdown syntax)>
-#      warning: |-
-#        <optional warning message (markdown syntax)>
-#      upgradeNotes: |-
-#        <optional upgrade guidelines (markdown syntax)>
-#      deprecations: |-
-#        <optional deprecation notes (markdown syntax)>
-#      pullRequestNumber: <pull request number>
-#      jiraIssueNumber: <Jira issue number>
+#  - title: <title>
+#    type: [bug, enhancement, internal]
+#    impact: [incompatible, minor, patch]
+#    description: |-
+#      <description (markdown syntax)>
+#    warning: |-
+#      <optional warning message (markdown syntax)>
+#    upgradeNotes: |-
+#      <optional upgrade guidelines (markdown syntax)>
+#    deprecations: |-
+#      <optional deprecation notes (markdown syntax)>
+#    pullRequestNumber: <pull request number>
+#    jiraIssueNumber: <Jira issue number>
 
 
 # Do not modify the "NEXT" version block.
 # Do not change the order ("NEXT" needs to be on top).
 - version: "NEXT"
   date: TBD
   changes:
-
   - type: internal
     impact: patch
     title: The release pipeline is now enabled for hotfix releases
@@ -38,6 +37,17 @@
     description: |-
       - upgrade Kubernetes libs from v1.17.6 to v1.17.13 (see [K8s changelog](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.17.md))
 
+  - title: improve pkg/runctl/cfg
+    type: internal
+    impact: patch
+    description: |-
+      - fix: pipeline run fails with `error_config` if its `spec.profiles.network` is set to the name of the _default_ network profile
+      - do not trim whitespace from configured network policies, as it may destroy YAML wellformedness
+      - fail loading the pipeline runs configuration if the network policies config map does not exist
+      - give more precise error messages in case of erroneous pipeline runs configuration
+      - lots of refactoring in `pkg/runctl/cfg`
+    pullRequestNumber: 181
+
 - version: "0.6.0"
   date: 2020-11-09
   changes:

pkg/runctl/cfg/config.go

@@ -16,79 +16,164 @@ import (
 )
 
 const (
-	pipelineRunsConfigMapName            = "steward-pipelineruns"
-	pipelineRunsConfigKeyTimeout         = "timeout"
-	pipelineRunsConfigKeyLimitRange      = "limitRange"
-	pipelineRunsConfigKeyResourceQuota   = "resourceQuota"
-	pipelineRunsConfigKeyImage           = "jenkinsfileRunner.image"
-	pipelineRunsConfigKeyImagePullPolicy = "jenkinsfileRunner.imagePullPolicy"
-	pipelineRunsConfigKeyPSCRunAsUser    = "jenkinsfileRunner.podSecurityContext.runAsUser"
-	pipelineRunsConfigKeyPSCRunAsGroup   = "jenkinsfileRunner.podSecurityContext.runAsGroup"
-	pipelineRunsConfigKeyPSCFSGroup      = "jenkinsfileRunner.podSecurityContext.fsGroup"
+	mainConfigMapName            = "steward-pipelineruns"
+	mainConfigKeyTimeout         = "timeout"
+	mainConfigKeyLimitRange      = "limitRange"
+	mainConfigKeyResourceQuota   = "resourceQuota"
+	mainConfigKeyImage           = "jenkinsfileRunner.image"
+	mainConfigKeyImagePullPolicy = "jenkinsfileRunner.imagePullPolicy"
+	mainConfigKeyPSCRunAsUser    = "jenkinsfileRunner.podSecurityContext.runAsUser"
+	mainConfigKeyPSCRunAsGroup   = "jenkinsfileRunner.podSecurityContext.runAsGroup"
+	mainConfigKeyPSCFSGroup      = "jenkinsfileRunner.podSecurityContext.fsGroup"
 
 	networkPoliciesConfigMapName    = "steward-pipelineruns-network-policies"
 	networkPoliciesConfigKeyDefault = "_default"
 )
 
-// PipelineRunsConfigStruct is a struct holding the pipeline runs configuration
+// PipelineRunsConfigStruct is a struct holding the pipeline runs configuration.
 type PipelineRunsConfigStruct struct {
-	Timeout                                       *metav1.Duration
-	NetworkPolicies                               map[string]string
-	DefaultNetworkPolicy                          string
-	LimitRange                                    string
-	ResourceQuota                                 string
-	JenkinsfileRunnerImage                        string
-	JenkinsfileRunnerImagePullPolicy              string
-	JenkinsfileRunnerPodSecurityContextRunAsUser  *int64
+	// Timeout is the maximum execution time of a pipeline run.
+	// If `nil`, a default timeout should be used.
+	Timeout *metav1.Duration
+
+	// The manifest (in YAML format) of a Kubernetes LimitRange object to be
+	// applied to each pipeline run sandbox namespace.
+	// If empty, no limit range will be defined.
+	LimitRange string
+
+	// The manifest (in YAML format) of a Kubernetes ResourceQuota object to be
+	// applied to each pipeline run sandbox namespace.
+	// If empty, no resource quota will be defined.
+	ResourceQuota string
+
+	// JenkinsfileRunnerImage is the Jenkinsfile Runner container image to be
+	// used for pipeline runs.
+	// If empty, a default image will be used.
+	JenkinsfileRunnerImage string
+
+	// JenkinsfileRunnerImagePullPolicy is the pull policy for the container
+	// image defined by `JenkinsfileRunnerImage`.
+	// It defaults to `IfNotPresent`.
+	// If `JenkinsfileRunnerImage` is not set, this value is not used (it does
+	// not apply to the default image).
+	JenkinsfileRunnerImagePullPolicy string
+
+	// JenkinsfileRunnerPodSecurityContextRunAsUser is the numerical user id
+	// the Jenkinsfile Runner process is started as.
+	JenkinsfileRunnerPodSecurityContextRunAsUser *int64
+
+	// JenkinsfileRunnerPodSecurityContextRunAsGroup is the numerical group id
+	// the Jenkinsfile Runner process is started as.
 	JenkinsfileRunnerPodSecurityContextRunAsGroup *int64
-	JenkinsfileRunnerPodSecurityContextFSGroup    *int64
+
+	// JenkinsfileRunnerPodSecurityContextFSGroup is the numerical filesystem
+	// group id the Jenkinsfile Runner pod will use.
+	JenkinsfileRunnerPodSecurityContextFSGroup *int64
+
+	// DefaultNetworkProfile is the name of the network profile that should
+	// be used in case the user has not explicitly chosen one.
+	DefaultNetworkProfile string
+
+	// NetworkPolicies maps network profile names to network policies.
+	// Each value is a Kubernetes network policy manifest in YAML format.
+	NetworkPolicies map[string]string
 }
 
 // LoadPipelineRunsConfig loads the pipelineruns configuration and returns it.
 func LoadPipelineRunsConfig(clientFactory k8s.ClientFactory) (*PipelineRunsConfigStruct, error) {
-	configMapIfce := clientFactory.CoreV1().ConfigMaps(system.Namespace())
-	config := &PipelineRunsConfigStruct{}
+	dest := &PipelineRunsConfigStruct{}
 
-	configMap, err := configMapIfce.Get(pipelineRunsConfigMapName, metav1.GetOptions{})
-	if err != nil && !k8serrors.IsNotFound(err) {
-		return nil, withRecoverability(err, true)
-	}
-	if configMap != nil {
-		err = processMainConfig(configMap.Data, config)
+	for _, p := range []struct {
+		configMapName string
+		optional      bool
+		processFunc   func(map[string]string, *PipelineRunsConfigStruct) error
+	}{
+		{
+			configMapName: mainConfigMapName,
+			optional:      true,
+			processFunc:   processMainConfig,
+		},
+		{
+			configMapName: networkPoliciesConfigMapName,
+			optional:      false,
+			processFunc:   processNetworkPoliciesConfig,
+		},
+	} {
+		err := processConfigMap(
+			p.configMapName, p.optional, p.processFunc,
+			dest, clientFactory,
+		)
 		if err != nil {
-			return nil, withRecoverability(err, false)
+			return nil, err
 		}
 	}
 
-	networkMap, err := configMapIfce.Get(networkPoliciesConfigMapName, metav1.GetOptions{})
+	return dest, nil
+}
+
+func withRecoverability(err error, isInfraError bool) error {
+	return serrors.RecoverableIf(err, isInfraError || featureflag.RetryOnInvalidPipelineRunsConfig.Enabled())
+}
+
+/*
+processConfigMap is a higher-order function which calls `processFunc` to
+process the config map with the given name and enriches error messages
+with contextual information.
+`optional` indicated whether the config map may not exist, in which case
+`processFunc` is NOT called and NO error is returned.
+`dest` is the destination struct to store loaded configuration values in.
+It gets passed to `processFunc`.
+*/
+func processConfigMap(
+	configMapName string,
+	optional bool,
+	processFunc func(map[string]string, *PipelineRunsConfigStruct) error,
+	dest *PipelineRunsConfigStruct,
+	clientFactory k8s.ClientFactory,
+) error {
+
+	wrapError := func(cause error) error {
+		return errors.Wrapf(cause,
+			"invalid configuration: ConfigMap %q in namespace %q",
+			configMapName,
+			system.Namespace(),
+		)
+	}
+
+	configMapIfce := clientFactory.CoreV1().ConfigMaps(system.Namespace())
+
+	var err error
+	configMap, err := configMapIfce.Get(configMapName, metav1.GetOptions{})
 	if err != nil && !k8serrors.IsNotFound(err) {
-		return nil, withRecoverability(err, true)
+		return withRecoverability(wrapError(err), true)
 	}
 
-	if networkMap != nil {
-		err = processNetworkMap(networkMap.Data, config)
+	if configMap != nil {
+		err = processFunc(configMap.Data, dest)
 		if err != nil {
-			return nil, withRecoverability(err, false)
+			return withRecoverability(wrapError(err), false)
 		}
+	} else if !optional {
+		return withRecoverability(wrapError(errors.New("is missing")), false)
 	}
-	return config, nil
-}
 
-func withRecoverability(err error, isInfraError bool) error {
-	return serrors.RecoverableIf(err, isInfraError || featureflag.RetryOnInvalidPipelineRunsConfig.Enabled())
+	return nil
 }
 
-func processMainConfig(configData map[string]string, config *PipelineRunsConfigStruct) error {
-	config.LimitRange = configData[pipelineRunsConfigKeyLimitRange]
-	config.ResourceQuota = configData[pipelineRunsConfigKeyResourceQuota]
-	config.JenkinsfileRunnerImage = configData[pipelineRunsConfigKeyImage]
-	config.JenkinsfileRunnerImagePullPolicy = configData[pipelineRunsConfigKeyImagePullPolicy]
+func processMainConfig(configData map[string]string, dest *PipelineRunsConfigStruct) error {
+
+	wrapParseError := func(cause error, key, strVal string) error {
+		return errors.Wrapf(cause,
+			"key %q: cannot parse value %q",
+			key, strVal,
+		)
+	}
 
 	parseInt64 := func(key string) (*int64, error) {
 		if strVal, ok := configData[key]; ok && strVal != "" {
 			intVal, err := strconv.ParseInt(strVal, 10, 64)
 			if err != nil {
-				return nil, errors.Wrapf(err, "cannot parse configuration value %q at %q", strVal, key)
+				return nil, wrapParseError(err, key, strVal)
 			}
 			return &intVal, nil
 		}
@@ -99,68 +184,88 @@ func processMainConfig(configData map[string]string, config *PipelineRunsConfigS
 		if strVal, ok := configData[key]; ok && strVal != "" {
 			d, err := time.ParseDuration(strVal)
 			if err != nil {
-				return nil, errors.Wrapf(err, "cannot parse configuration value %q at %q", strVal, key)
+				return nil, wrapParseError(err, key, strVal)
 			}
 			return &metav1.Duration{Duration: d}, nil
 		}
 		return nil, nil
 	}
+
+	dest.LimitRange = configData[mainConfigKeyLimitRange]
+	dest.ResourceQuota = configData[mainConfigKeyResourceQuota]
+	dest.JenkinsfileRunnerImage = configData[mainConfigKeyImage]
+	dest.JenkinsfileRunnerImagePullPolicy = configData[mainConfigKeyImagePullPolicy]
+
 	var err error
-	if config.Timeout, err =
-		parseDuration(pipelineRunsConfigKeyTimeout); err != nil {
+
+	if dest.Timeout, err =
+		parseDuration(mainConfigKeyTimeout); err != nil {
 		return err
 	}
 
-	if config.JenkinsfileRunnerPodSecurityContextRunAsUser, err =
-		parseInt64(pipelineRunsConfigKeyPSCRunAsUser); err != nil {
+	if dest.JenkinsfileRunnerPodSecurityContextRunAsUser, err =
+		parseInt64(mainConfigKeyPSCRunAsUser); err != nil {
 		return err
 	}
-	if config.JenkinsfileRunnerPodSecurityContextRunAsGroup, err =
-		parseInt64(pipelineRunsConfigKeyPSCRunAsGroup); err != nil {
+	if dest.JenkinsfileRunnerPodSecurityContextRunAsGroup, err =
+		parseInt64(mainConfigKeyPSCRunAsGroup); err != nil {
 		return err
 	}
 
-	if config.JenkinsfileRunnerPodSecurityContextFSGroup, err =
-		parseInt64(pipelineRunsConfigKeyPSCFSGroup); err != nil {
+	if dest.JenkinsfileRunnerPodSecurityContextFSGroup, err =
+		parseInt64(mainConfigKeyPSCFSGroup); err != nil {
 		return err
 	}
 
 	return nil
 }
 
-func processNetworkMap(networkMap map[string]string, config *PipelineRunsConfigStruct) error {
+func processNetworkPoliciesConfig(configData map[string]string, dest *PipelineRunsConfigStruct) error {
+
 	isValidKey := func(key string) bool {
-		trimmedKey := strings.TrimSpace(key)
-		return trimmedKey != "" && key == trimmedKey && !strings.HasPrefix(trimmedKey, "_")
+		return key != "" && key == strings.TrimSpace(key) && !strings.HasPrefix(key, "_")
 	}
-	defaultNetworkPolicyKey := networkMap[networkPoliciesConfigKeyDefault]
-	if !isValidKey(defaultNetworkPolicyKey) {
+
+	dest.DefaultNetworkProfile = ""
+	dest.NetworkPolicies = nil
+
+	networkPolicies := map[string]string{}
+	for key, value := range configData {
+		if isValidKey(key) && strings.TrimSpace(value) != "" {
+			networkPolicies[key] = value
+		}
+	}
+
+	var (
+		defaultNetworkPolicyKey string
+		found                   bool
+	)
+
+	if defaultNetworkPolicyKey, found = configData[networkPoliciesConfigKeyDefault]; !found {
 		return fmt.Errorf(
-			"invalid configuration: ConfigMap %q in namespace %q: key %q is missing or invalid",
-			pipelineRunsConfigMapName,
-			system.Namespace(),
+			"key %q is missing",
 			networkPoliciesConfigKeyDefault,
 		)
 	}
-	if config.DefaultNetworkPolicy = strings.TrimSpace(networkMap[defaultNetworkPolicyKey]); config.DefaultNetworkPolicy == "" {
+
+	if !isValidKey(defaultNetworkPolicyKey) {
 		return fmt.Errorf(
-			"invalid configuration: ConfigMap %q in namespace %q: key %q: "+
-				"no network policy with key %q found",
-			pipelineRunsConfigMapName,
-			system.Namespace(),
+			"key %q: value %q is not a valid network policy key",
 			networkPoliciesConfigKeyDefault,
 			defaultNetworkPolicyKey,
 		)
 	}
 
-	networkPolicies := map[string]string{}
-	for key, value := range networkMap {
-		if key != defaultNetworkPolicyKey && isValidKey(key) && strings.TrimSpace(value) != "" {
-			networkPolicies[key] = value
-		}
-	}
-	if len(networkPolicies) != 0 {
-		config.NetworkPolicies = networkPolicies
+	if _, found = networkPolicies[defaultNetworkPolicyKey]; !found {
+		return fmt.Errorf(
+			"key %q: value %q does not denote an existing network policy key",
+			networkPoliciesConfigKeyDefault,
+			defaultNetworkPolicyKey,
+		)
 	}
+
+	dest.DefaultNetworkProfile = defaultNetworkPolicyKey
+	dest.NetworkPolicies = networkPolicies
+
 	return nil
 }