Use instance metadata canonical region name in metadata (#273)

This change simplifies the canonical region name lookup by pulling directly from instance metadata.

Author: owainlewis

pkg/cloudprovider/providers/oci/ccm.go

@@ -102,7 +102,7 @@ func NewCloudProvider(config *providercfg.Config) (cloudprovider.Interface, erro
 		if err != nil {
 			return nil, err
 		}
-		config.CompartmentID = metadata.CompartmentOCID
+		config.CompartmentID = metadata.CompartmentID
 	}
 
 	if !config.LoadBalancer.Disabled && config.VCNID == "" {

pkg/cloudprovider/providers/oci/config/config_test.go

@@ -126,7 +126,7 @@ func TestReadConfigShouldHaveNoDefaultRegionIfNoneSpecified(t *testing.T) {
 	}
 }
 
-func TestReadConfigShouldSetCompartmentOCIDWhenProvidedValidConfig(t *testing.T) {
+func TestReadConfigShouldSetCompartmentIDWhenProvidedValidConfig(t *testing.T) {
 	cfg, err := ReadConfig(strings.NewReader(validConfig))
 	if err != nil {
 		t.Fatalf("expected no error but got '%+v'", err)

pkg/flexvolume/block/config.go

@@ -21,7 +21,6 @@ import (
 	"io/ioutil"
 	"log"
 	"os"
-	"strings"
 
 	providercfg "github.com/oracle/oci-cloud-controller-manager/pkg/cloudprovider/providers/oci/config"
 	"github.com/oracle/oci-cloud-controller-manager/pkg/oci/instance/metadata"
@@ -31,13 +30,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
-var ociRegions = map[string]string{
-	"phx": "us-phoenix-1",
-	"iad": "us-ashburn-1",
-	"fra": "eu-frankfurt-1",
-	"lhr": "uk-london-1",
-}
-
 // Config holds the configuration for the OCI flexvolume driver.
 type Config struct {
 	providercfg.Config `yaml:",inline"`
@@ -89,23 +81,23 @@ func ConfigFromFile(path string) (*Config, error) {
 }
 
 func (c *Config) setDefaults() error {
-	if c.Auth.Region == "" || c.Auth.CompartmentID == "" {
+	if c.Auth.Region == "" || c.Auth.RegionKey == "" || c.Auth.CompartmentID == "" {
 		meta, err := c.metadata.Get()
 		if err != nil {
 			return err
 		}
 
 		if c.Auth.Region == "" {
-			c.Auth.Region = meta.Region
+			c.Auth.Region = meta.CanonicalRegionName
 		}
-		if c.Auth.CompartmentID == "" {
-			c.Auth.CompartmentID = meta.CompartmentOCID
+
+		if c.Auth.RegionKey == "" {
+			c.Auth.RegionKey = meta.Region
 		}
-	}
 
-	err := c.setRegionFields(c.Auth.Region)
-	if err != nil {
-		return fmt.Errorf("setting config region fields: %v", err)
+		if c.Auth.CompartmentID == "" {
+			c.Auth.CompartmentID = meta.CompartmentID
+		}
 	}
 
 	if c.Auth.Passphrase == "" && c.Auth.PrivateKeyPassphrase != "" {
@@ -116,34 +108,6 @@ func (c *Config) setDefaults() error {
 	return nil
 }
 
-// setRegionFields accepts either a region short name or a region long name and
-// sets both the Region and RegionKey fields.
-func (c *Config) setRegionFields(region string) error {
-	input := region
-	region = strings.ToLower(region)
-
-	var name, key string
-	name, ok := ociRegions[region]
-	if !ok {
-		for key, name = range ociRegions {
-			if name == region {
-				ok = true
-				break
-			}
-		}
-		if !ok {
-			return fmt.Errorf("tried to connect to unsupported OCI region %q", input)
-		}
-	} else {
-		key = region
-	}
-
-	c.Auth.Region = name
-	c.Auth.RegionKey = key
-
-	return nil
-}
-
 // validate checks that all required fields are populated.
 func (c *Config) validate() error {
 	return ValidateConfig(c).ToAggregate()

pkg/flexvolume/block/config_test.go

@@ -24,14 +24,15 @@ import (
 )
 
 func TestConfigDefaulting(t *testing.T) {
-	expectedCompartmentOCID := "ocid1.compartment.oc1..aaaaaaaa3um2atybwhder4qttfhgon4j3hcxgmsvnyvx4flfjyewkkwfzwnq"
+	expectedCompartmentID := "ocid1.compartment.oc1..aaaaaaaa3um2atybwhder4qttfhgon4j3hcxgmsvnyvx4flfjyewkkwfzwnq"
 	expectedRegion := "us-phoenix-1"
 	expectedRegionKey := "phx"
 
 	cfg := &Config{metadata: metadata.NewMock(
 		&metadata.InstanceMetadata{
-			CompartmentOCID: expectedCompartmentOCID,
-			Region:          expectedRegionKey, // instance metadata API only returns the region key
+			CompartmentID:       expectedCompartmentID,
+			CanonicalRegionName: expectedRegion,
+			Region:              expectedRegionKey, // instance metadata API only returns the region key
 		},
 	)}
 
@@ -43,59 +44,13 @@ func TestConfigDefaulting(t *testing.T) {
 	if cfg.Auth.Region != expectedRegion {
 		t.Fatalf("Expected cfg.Region = %q, got %q", cfg.Auth.Region, expectedRegion)
 	}
+
 	if cfg.Auth.RegionKey != expectedRegionKey {
 		t.Fatalf("Expected cfg.RegionKey = %q, got %q", cfg.Auth.RegionKey, expectedRegionKey)
 	}
 
-	if cfg.Auth.CompartmentID != expectedCompartmentOCID {
-		t.Fatalf("Expected cfg.CompartmentOCID = %q, got %q", cfg.Auth.CompartmentID, expectedCompartmentOCID)
-	}
-}
-
-func TestConfigSetRegion(t *testing.T) {
-	var testCases = []struct {
-		in          string
-		region      string
-		shortRegion string
-		shouldErr   bool
-	}{
-		{"us-phoenix-1", "us-phoenix-1", "phx", false},
-		{"US-PHOENIX-1", "us-phoenix-1", "phx", false},
-		{"phx", "us-phoenix-1", "phx", false},
-		{"PHX", "us-phoenix-1", "phx", false},
-
-		{"us-ashburn-1", "us-ashburn-1", "iad", false},
-		{"US-ASHBURN-1", "us-ashburn-1", "iad", false},
-		{"iad", "us-ashburn-1", "iad", false},
-		{"IAD", "us-ashburn-1", "iad", false},
-
-		{"eu-frankfurt-1", "eu-frankfurt-1", "fra", false},
-		{"EU-FRANKFURT-1", "eu-frankfurt-1", "fra", false},
-		{"fra", "eu-frankfurt-1", "fra", false},
-		{"FRA", "eu-frankfurt-1", "fra", false},
-
-		// error cases
-		{"us-east", "", "", true},
-		{"", "", "", true},
-	}
-
-	for _, tt := range testCases {
-		t.Run(tt.in, func(t *testing.T) {
-			cfg := &Config{}
-			err := cfg.setRegionFields(tt.in)
-			if err != nil {
-				if !tt.shouldErr {
-					t.Errorf("SetRegionFields(%q) unexpected error: %v", tt.in, err)
-				}
-			}
-
-			if cfg.Auth.Region != tt.region {
-				t.Errorf("SetRegionFields(%q) => {Region: %q}; want {Region: %q}", tt.in, cfg.Auth.Region, tt.region)
-			}
-			if cfg.Auth.RegionKey != tt.shortRegion {
-				t.Errorf("SetRegionFields(%q) => {RegionShortName: %q}; want {RegionShortName: %q}", tt.in, cfg.Auth.RegionKey, tt.shortRegion)
-			}
-		})
+	if cfg.Auth.CompartmentID != expectedCompartmentID {
+		t.Fatalf("Expected cfg.CompartmentID = %q, got %q", cfg.Auth.CompartmentID, expectedCompartmentID)
 	}
 }
 

pkg/flexvolume/block/driver.go

@@ -158,23 +158,14 @@ func (d OCIFlexvolumeDriver) Init() flexvolume.DriverStatus {
 	return flexvolume.Succeed()
 }
 
-// deriveVolumeOCID will figure out the correct OCID for a volume
-// based solely on the region key and volumeName. Because of differences
-// across regions we need to impose some awkward logic here to get the correct
-// OCID or if it is already an OCID then return the OCID.
+// deriveVolumeOCID will expand a partial OCID to a full OCID
+// based on the region key and volume name.
 func deriveVolumeOCID(regionKey string, volumeName string) string {
 	if strings.HasPrefix(volumeName, ocidPrefix) {
 		return volumeName
 	}
 
-	var volumeOCID string
-	if regionKey == "fra" {
-		volumeOCID = fmt.Sprintf(volumeOCIDTemplate, "eu-frankfurt-1", volumeName)
-	} else {
-		volumeOCID = fmt.Sprintf(volumeOCIDTemplate, regionKey, volumeName)
-	}
-
-	return volumeOCID
+	return fmt.Sprintf(volumeOCIDTemplate, regionKey, volumeName)
 }
 
 // constructKubeClient uses a kubeconfig layed down by a secret via deploy.sh to return

pkg/flexvolume/block/driver_test.go

@@ -26,7 +26,8 @@ var volumeOCIDTests = []struct {
 }{
 	{"phx", "aaaaaa", "ocid1.volume.oc1.phx.aaaaaa"},
 	{"iad", "aaaaaa", "ocid1.volume.oc1.iad.aaaaaa"},
-	{"fra", "aaaaaa", "ocid1.volume.oc1.eu-frankfurt-1.aaaaaa"},
+	{"eu-frankfurt-1", "aaaaaa", "ocid1.volume.oc1.eu-frankfurt-1.aaaaaa"},
+	{"uk-london-1", "aaaaaa", "ocid1.volume.oc1.uk-london-1.aaaaaa"},
 }
 
 func TestDeriveVolumeOCID(t *testing.T) {

pkg/oci/instance/metadata/instance_metadata.go

@@ -30,8 +30,9 @@ const (
 // local OCI instance metadata API endpoint.
 // https://docs.us-phoenix-1.oraclecloud.com/Content/Compute/Tasks/gettingmetadata.htm
 type InstanceMetadata struct {
-	CompartmentOCID string `json:"compartmentId"`
-	Region          string `json:"region"`
+	CompartmentID       string `json:"compartmentId"`
+	Region              string `json:"region"`
+	CanonicalRegionName string `json:"canonicalRegionName"`
 }
 
 // Interface defines how consumers access OCI instance metadata.

pkg/oci/instance/metadata/instance_metadata_test.go

@@ -32,6 +32,7 @@ const exampleResponse = `{
     "ssh_authorized_keys" : "ssh-rsa some-key-data tlangfor@tlangfor-mac\n"
   },
   "region" : "phx",
+  "canonicalRegionName" : "us-phoenix-1",
   "shape" : "VM.Standard1.1",
   "state" : "Provisioning",
   "timeCreated" : 1496415602152
@@ -49,8 +50,9 @@ func TestGetMetadata(t *testing.T) {
 	}
 
 	expected := &InstanceMetadata{
-		CompartmentOCID: "ocid1.compartment.oc1..aaaaaaaa3um2atybwhder4qttfhgon4j3hcxgmsvnyvx4flfjyewkkwfzwnq",
-		Region:          "phx",
+		CompartmentID:       "ocid1.compartment.oc1..aaaaaaaa3um2atybwhder4qttfhgon4j3hcxgmsvnyvx4flfjyewkkwfzwnq",
+		Region:              "phx",
+		CanonicalRegionName: "us-phoenix-1",
 	}
 	if !reflect.DeepEqual(meta, expected) {
 		t.Errorf("Get() => %+v, want %+v", meta, expected)

pkg/volume/provisioner/block/block_test.go

@@ -272,11 +272,6 @@ func (p *MockProvisionerClient) Timeout() time.Duration {
 	return 30 * time.Second
 }
 
-// CompartmentOCID mocks client CompartmentOCID implementation
-func (p *MockProvisionerClient) CompartmentOCID() (compartmentOCID string) {
-	return ""
-}
-
 // TenancyOCID mocks client TenancyOCID implementation
 func (p *MockProvisionerClient) TenancyOCID() string {
 	return "ocid1.tenancy.oc1..aaaaaaaatyn7scrtwtqedvgrxgr2xunzeo6uanvyhzxqblctwkrpisvke4kq"

pkg/volume/provisioner/core/provisioner.go

@@ -106,8 +106,8 @@ func NewOCIProvisioner(
 		if metadata == nil {
 			return nil, errors.Wrap(mdErr, "unable to get compartment OCID")
 		}
-		logger.With("compartmentID", metadata.CompartmentOCID).Infof("'CompartmentID' not given. Using compartment OCID from instance metadata.")
-		cfg.CompartmentID = metadata.CompartmentOCID
+		logger.With("compartmentID", metadata.CompartmentID).Infof("'CompartmentID' not given. Using compartment OCID from instance metadata.")
+		cfg.CompartmentID = metadata.CompartmentID
 	}
 
 	cp, err := newConfigurationProvider(logger, cfg)