Merge pull request #147 from shiftstack/merge-bot-master

Merge https://github.com/kubernetes/cloud-provider-openstack:master into master

Author: openshift-merge-robot

charts/manila-csi-plugin/templates/controllerplugin-statefulset.yaml

@@ -26,6 +26,9 @@ spec:
             {{- if $.Values.csimanila.topologyAwarenessEnabled }}
             - "--feature-gates=Topology=true"
             {{- end }}
+            {{- if $.Values.controllerplugin.provisioner.extraCreateMetadata }}
+            - "--extra-create-metadata"
+            {{- end }}
           env:
             - name: ADDRESS
               value: "unix:///var/lib/kubelet/plugins/{{ printf "%s.%s" .protocolSelector $.Values.driverName | lower }}/csi-controllerplugin.sock"

charts/manila-csi-plugin/values.yaml

@@ -79,6 +79,8 @@ controllerplugin:
       tag: v3.0.0
       pullPolicy: IfNotPresent
     resources: {}
+    # Whether to pass --extra-create-metadata flag to csi-provisioner.
+    extraCreateMetadata: false
   # CSI external-snapshotter container spec
   snapshotter:
     image:

docs/openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md

@@ -166,6 +166,13 @@ The options in `Global` section are used for openstack-cloud-controller-manager
   The name of Neutron external network. openstack-cloud-controller-manager uses this option when getting the external IP of the Kubernetes node. Can be specified multiple times. Specified network names will be ORed. Default: ""
 * `internal-network-name`
   The name of Neutron internal network. openstack-cloud-controller-manager uses this option when getting the internal IP of the Kubernetes node, this is useful if the node has multiple interfaces. Can be specified multiple times. Specified network names will be ORed. Default: ""
+* `address-sort-order`
+  This configuration key influences the way the provider reports the node addresses to the Kubernetes node resource. The default order depends on the hard-coded order the provider queries the addresses and what the cloud returns, which does not guarantee a specific order.
+
+  To override this behavior it is possible to specify a comma separated list of CIDRs. Essentially, this will sort and group all addresses matching a CIDR in a prioritized manner, where the first item having a higher priority than the last. All non-matching addresses will remain in the same order they are already in.
+
+  For example, this option can be useful when having multiple or dual-stack interfaces attached to a node and needing a user-controlled, deterministic way of sorting the addresses.
+  Default: ""
 
 ###  Load Balancer
 

pkg/csi/manila/controllerserver.go

@@ -43,6 +43,15 @@ type controllerServer struct {
 var (
 	pendingVolumes   = sync.Map{}
 	pendingSnapshots = sync.Map{}
+
+	// Recognized volume parameters passed by Kubernetes csi-provisioner sidecar
+	// when run with --extra-create-metadata flag. These are added to metadata
+	// of newly created shares if present.
+	recognizedCSIProvisionerParams = []string{
+		"csi.storage.k8s.io/pvc/name",
+		"csi.storage.k8s.io/pvc/namespace",
+		"csi.storage.k8s.io/pv/name",
+	}
 )
 
 func getVolumeCreator(source *csi.VolumeContentSource, shareOpts *options.ControllerVolumeContext, compatOpts *options.CompatibilityOptions) (volumeCreator, error) {
@@ -92,7 +101,7 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 		return nil, status.Errorf(codes.InvalidArgument, "invalid volume parameters: %v", err)
 	}
 
-	shareMetadata, err := prepareShareMetadata(shareOpts.AppendShareMetadata, cs.d.clusterID)
+	shareMetadata, err := prepareShareMetadata(shareOpts.AppendShareMetadata, cs.d.clusterID, params)
 	if err != nil {
 		return nil, err
 	}
@@ -485,16 +494,34 @@ func parseStringMapFromJSON(data string) (m map[string]string, err error) {
 	return
 }
 
-func prepareShareMetadata(appendShareMetadata, clusterID string) (map[string]string, error) {
-	shareMetadata, err := parseStringMapFromJSON(appendShareMetadata)
+func prepareShareMetadata(appendShareMetadata, clusterID string, volumeParams map[string]string) (map[string]string, error) {
+	shareMetadata := make(map[string]string)
+
+	// Get extra metadata provided by csi-provisioner sidecar if present.
+	for _, k := range recognizedCSIProvisionerParams {
+		if v, ok := volumeParams[k]; ok {
+			shareMetadata[k] = v
+		}
+	}
+
+	// Get metadata from appendShareMetadata volume parameter.
+	// It will not overwrite keys defined in recognizedCSIProvisionerParams.
+
+	appendShareMetadataMap, err := parseStringMapFromJSON(appendShareMetadata)
 	if err != nil {
 		return nil, status.Errorf(codes.InvalidArgument, "failed to parse appendShareMetadata field: %v", err)
 	}
 
-	if clusterID != "" {
-		if shareMetadata == nil {
-			shareMetadata = make(map[string]string)
+	for k, v := range appendShareMetadataMap {
+		if existingValue, ok := shareMetadata[k]; ok {
+			klog.Warningf("skip adding share metadata key %s from appendShareMetadata because it already exists with value %s", k, existingValue)
+		} else {
+			shareMetadata[k] = v
 		}
+	}
+
+	// Get cluster ID.
+	if clusterID != "" {
 		if val, ok := shareMetadata[clusterMetadataKey]; ok && val != clusterID {
 			klog.Warningf("skip adding cluster ID %v to share metadata because appended metadata already defines it as %v", clusterID, val)
 		} else {

pkg/csi/manila/controllerserver_test.go

@@ -20,21 +20,24 @@ import (
 
 func TestPrepareShareMetadata(t *testing.T) {
 	ts := []struct {
+		allVolumeParams     map[string]string
 		appendShareMetadata string
 		cluster             string
 		expectedResult      map[string]string
 		expectedError       bool
 	}{
 		{
 			// Empty metadata and cluster
+			allVolumeParams:     map[string]string{},
 			appendShareMetadata: "",
 			cluster:             "",
 			expectedResult:      nil,
 			expectedError:       false,
 		},
 		{
 			// Existing metadata and empty cluster
-			appendShareMetadata: "{\"keyA\": \"valueA\", \"keyB\": \"valueB\"}",
+			allVolumeParams:     map[string]string{"appendShareMetadata": `{"keyA": "valueA", "keyB": "valueB"}`},
+			appendShareMetadata: `{"keyA": "valueA", "keyB": "valueB"}`,
 			cluster:             "",
 			expectedResult:      map[string]string{"keyA": "valueA", "keyB": "valueB"},
 			expectedError:       false,
@@ -48,29 +51,66 @@ func TestPrepareShareMetadata(t *testing.T) {
 		},
 		{
 			// Both metadata and cluster
+			allVolumeParams:     map[string]string{"appendShareMetadata": `{"keyA": "valueA", "keyB": "valueB"}`},
 			appendShareMetadata: "{\"keyA\": \"valueA\", \"keyB\": \"valueB\"}",
 			cluster:             "MyCluster",
 			expectedResult:      map[string]string{"keyA": "valueA", "keyB": "valueB", clusterMetadataKey: "MyCluster"},
 			expectedError:       false,
 		},
 		{
 			// Overwrite cluster
+			allVolumeParams:     map[string]string{"appendShareMetadata": "{\"keyA\": \"valueA\", \"" + clusterMetadataKey + "\": \"SomeValue\"}"},
 			appendShareMetadata: "{\"keyA\": \"valueA\", \"" + clusterMetadataKey + "\": \"SomeValue\"}",
 			cluster:             "MyCluster",
 			expectedResult:      map[string]string{"keyA": "valueA", clusterMetadataKey: "SomeValue"},
 			expectedError:       false,
 		},
 		{
 			// Incorrect metadata
+			allVolumeParams:     map[string]string{"appendShareMetadata": "INVALID"},
 			appendShareMetadata: "INVALID",
 			cluster:             "MyCluster",
 			expectedResult:      nil,
 			expectedError:       true,
 		},
+		{
+			// csi-provisioner PV/PVC metadata
+			allVolumeParams: map[string]string{
+				"csi.storage.k8s.io/pvc/name":      "pvc-name",
+				"csi.storage.k8s.io/pvc/namespace": "pvc-namespace",
+				"csi.storage.k8s.io/pv/name":       "pv-name",
+			},
+			cluster: "",
+			expectedResult: map[string]string{
+				"csi.storage.k8s.io/pvc/name":      "pvc-name",
+				"csi.storage.k8s.io/pvc/namespace": "pvc-namespace",
+				"csi.storage.k8s.io/pv/name":       "pv-name",
+			},
+			appendShareMetadata: "",
+			expectedError:       false,
+		},
+		{
+			// csi-provisioner PV/PVC metadata with conflicting appendShareMetadata
+			allVolumeParams: map[string]string{
+				"csi.storage.k8s.io/pvc/name":      "pvc-name",
+				"csi.storage.k8s.io/pvc/namespace": "pvc-namespace",
+				"csi.storage.k8s.io/pv/name":       "pv-name",
+				"appendShareMetadata":              `{"csi.storage.k8s.io/pvc/name": "SomeValue", "keyX": "valueX"}`,
+			},
+			appendShareMetadata: `{"csi.storage.k8s.io/pvc/name": "SomeValue", "keyX": "valueX"}`,
+			cluster:             "",
+			expectedResult: map[string]string{
+				"csi.storage.k8s.io/pvc/name":      "pvc-name",
+				"csi.storage.k8s.io/pvc/namespace": "pvc-namespace",
+				"csi.storage.k8s.io/pv/name":       "pv-name",
+				"keyX":                             "valueX",
+			},
+			expectedError: false,
+		},
 	}
 
 	for i := range ts {
-		result, err := prepareShareMetadata(ts[i].appendShareMetadata, ts[i].cluster)
+		result, err := prepareShareMetadata(ts[i].appendShareMetadata, ts[i].cluster, ts[i].allVolumeParams)
 
 		if err != nil && !ts[i].expectedError {
 			t.Errorf("test %d: unexpected error: %v", i, err)

pkg/openstack/instances.go

@@ -17,6 +17,7 @@ limitations under the License.
 package openstack
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"net"
@@ -56,10 +57,79 @@ type Instances struct {
 const (
 	instanceShutoff       = "SHUTOFF"
 	RegionalProviderIDEnv = "OS_CCM_REGIONAL"
+	noSortPriority        = 0
 )
 
 var _ cloudprovider.Instances = &Instances{}
 
+// buildAddressSortOrderList builds a list containing only valid CIDRs based on the content of addressSortOrder.
+//
+// It will ignore and warn about invalid sort order items.
+func buildAddressSortOrderList(addressSortOrder string) []*net.IPNet {
+	var list []*net.IPNet
+	for _, item := range strings.Split(addressSortOrder, ",") {
+		item = strings.TrimSpace(item)
+
+		_, cidr, err := net.ParseCIDR(item)
+		if err != nil {
+			klog.Warningf("Ignoring invalid sort order item '%s': %v.", item, err)
+			continue
+		}
+
+		list = append(list, cidr)
+	}
+
+	return list
+}
+
+// getSortPriority returns the priority as int of an address.
+//
+// The priority depends on the index of the CIDR in the list the address is matching,
+// where the first item of the list has higher priority than the last.
+//
+// If the address does not match any CIDR or is not an IP address the function returns noSortPriority.
+func getSortPriority(list []*net.IPNet, address string) int {
+	parsedAddress := net.ParseIP(address)
+	if parsedAddress == nil {
+		return noSortPriority
+	}
+
+	for i, cidr := range list {
+		if cidr.Contains(parsedAddress) {
+			fmt.Println(i, cidr, len(list)-i)
+			return len(list) - i
+		}
+	}
+
+	return noSortPriority
+}
+
+// sortNodeAddresses sorts node addresses based on comma separated list of CIDRs represented by addressSortOrder.
+//
+// The function only sorts addresses which match the CIDR and leaves the other addresses in the same order they are in.
+// Essentially, it will also group the addresses matching a CIDR together and sort them ascending in this group,
+// whereas the inter-group sorting depends on the priority.
+//
+// The priority depends on the order of the item in addressSortOrder, where the first item has higher priority than the last.
+func sortNodeAddresses(addresses []v1.NodeAddress, addressSortOrder string) {
+	list := buildAddressSortOrderList(addressSortOrder)
+
+	sort.SliceStable(addresses, func(i int, j int) bool {
+		addressLeft := addresses[i]
+		addressRight := addresses[j]
+
+		priorityLeft := getSortPriority(list, addressLeft.Address)
+		priorityRight := getSortPriority(list, addressRight.Address)
+
+		// ignore priorities of value 0 since this means the address has noSortPriority and we need to sort by priority
+		if priorityLeft > noSortPriority && priorityLeft == priorityRight {
+			return bytes.Compare(net.ParseIP(addressLeft.Address), net.ParseIP(addressRight.Address)) < 0
+		}
+
+		return priorityLeft > priorityRight
+	})
+}
+
 // Instances returns an implementation of Instances for OpenStack.
 func (os *OpenStack) Instances() (cloudprovider.Instances, bool) {
 	return os.instances()
@@ -608,6 +678,10 @@ func nodeAddresses(srv *servers.Server, interfaces []attachinterfaces.Interface,
 		}
 	}
 
+	if networkingOpts.AddressSortOrder != "" {
+		sortNodeAddresses(addrs, networkingOpts.AddressSortOrder)
+	}
+
 	return addrs, nil
 }