[occm] Simplify Configuration for Load Balancer

[shaardie]

Is this a BUG REPORT or FEATURE REQUEST?:

Uncomment only one, leave it on its own line:

/kind bug

/kind feature

What happened:

It is pretty complicated to understand under which circumstances which configuration is used in the load balancer. IMHO there are very deep and complex ifs in the code.

What you expected to happen:

Best would be, if the creation of the configuration is flatten a lot and it should be clear on the first sight.

How to reproduce it:

Anything else we need to know?:

I know it is quite complicated to fix this for the whole code. Maybe it would simplify the situation, if there is only a single getConfig function to get all configuration options from the different sources. Something like

func (lbaas *LbaasV2) getConfig(annotation, classFieldName, configFieldName, defaultValue string,
	service *corev1.Service, svcConf *serviceConfig) (string, error) {

	// Get config value from service annotation
	if annotation != "" {
		value := getStringFromServiceAnnotation(service, annotation, "")
		if value != "" {
			return value, nil
		}
	}

	// Get config value from config class
	if classFieldName != "" {
		configClassName := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerClass, "")
		if configClassName != "" {
			lbClass := lbaas.opts.LBClasses[svcConf.configClassName]
			if lbClass == nil {
				return "", fmt.Errorf("invalid loadbalancer class %q", svcConf.configClassName)
			}
			value := reflect.ValueOf(*lbClass).FieldByName(classFieldName).String()
			if value != "" {
				return value, nil
			}
		}
	}

	// Get config value from default config
	if configFieldName != "" {
		value := reflect.ValueOf(lbaas.opts).FieldByName(configFieldName).String()
		if value != "" {
			return value, nil
		}
	}

	// return default value
	return defaultValue, nil
}

Then every configuration call could look like this:

subnetID, err:= getConfig(ServiceAnnotationLoadBalancerSubnetID, "SubnetID", "SubnetID", "")
if err != nil {
...
}
memberSubnetID, err:= getConfig(ServiceAnnotationLoadBalancerMemberSubnetID, "MemberSubnetID", "MemberSubnetID", subnetID)
if err != nil {
...
}

I know using reflection here is not the nicest thing, but this way it can generalize the current situation.

I am not sure where the best place for discussing something like this is and therefore I opened this issue. I am eager to hear what you think.

Environment:

• openstack-cloud-controller-manager(or other related binary) version: Current master (https://github.com/kubernetes/cloud-provider-openstack/commit/d1196878af7af85bc2aea06c8fa37d27ba4a1f37)
• OpenStack version:
• Others: