diff --git a/manifests/cloud-provider-example.yaml b/manifests/cloud-provider-example.yaml index 7f49e19f61..15c1bbe471 100644 --- a/manifests/cloud-provider-example.yaml +++ b/manifests/cloud-provider-example.yaml @@ -35,6 +35,9 @@ loadBalancer: subnet2: ocid1.subnet.oc1.phx.aaaaaaaahuxrgvs65iwdz7ekwgg3l5gyah7ww5klkwjcso74u3e4i64hvtvq # SecurityListManagementMode configures how security lists are managed by the CCM. + # If you choose to have security lists managed by the CCM, ensure you have setup the following additional OCI policy: + # Allow dynamic-group [your dynamic group name] to manage security-lists in compartment [your compartment name] + # # "All" (default): Manage all required security list rules for load balancer services. # "Frontend": Manage only security list rules for ingress to the load # balancer. Requires that the user has setup a rule that diff --git a/pkg/oci/instances.go b/pkg/oci/instances.go index d275f3a4da..3596e4fbe3 100644 --- a/pkg/oci/instances.go +++ b/pkg/oci/instances.go @@ -97,7 +97,10 @@ func (cp *CloudProvider) NodeAddresses(ctx context.Context, name types.NodeName) // in this method to obtain nodeaddresses. func (cp *CloudProvider) NodeAddressesByProviderID(ctx context.Context, providerID string) ([]api.NodeAddress, error) { cp.logger.With("instanceID", providerID).Debug("Getting node addresses by provider id") - instanceID := util.MapProviderIDToInstanceID(providerID) + instanceID, err := util.MapProviderIDToInstanceID(providerID) + if err != nil { + return nil, errors.Wrap(err, "MapProviderIDToInstanceID") + } vnic, err := cp.client.Compute().GetPrimaryVNICForInstance(ctx, cp.config.CompartmentID, instanceID) if err != nil { return nil, errors.Wrap(err, "GetPrimaryVNICForInstance") @@ -157,7 +160,10 @@ func (cp *CloudProvider) InstanceType(ctx context.Context, name types.NodeName) func (cp *CloudProvider) InstanceTypeByProviderID(ctx context.Context, providerID string) (string, error) { cp.logger.With("instanceID", providerID).Debug("Getting instance type by provider id") - instanceID := util.MapProviderIDToInstanceID(providerID) + instanceID, err := util.MapProviderIDToInstanceID(providerID) + if err != nil { + return "", errors.Wrap(err, "MapProviderIDToInstanceID") + } inst, err := cp.client.Compute().GetInstance(ctx, instanceID) if err != nil { return "", errors.Wrap(err, "GetInstance") @@ -183,7 +189,10 @@ func (cp *CloudProvider) CurrentNodeName(ctx context.Context, hostname string) ( // instance will be immediately deleted by the cloud controller manager. func (cp *CloudProvider) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) { cp.logger.With("instanceID", providerID).Debug("Checking instance exists by provider id") - instanceID := util.MapProviderIDToInstanceID(providerID) + instanceID, err := util.MapProviderIDToInstanceID(providerID) + if err != nil { + return false, err + } instance, err := cp.client.Compute().GetInstance(ctx, instanceID) if client.IsNotFound(err) { return false, nil diff --git a/pkg/oci/load_balancer.go b/pkg/oci/load_balancer.go index 6bfb458e83..f293e9ad48 100644 --- a/pkg/oci/load_balancer.go +++ b/pkg/oci/load_balancer.go @@ -149,7 +149,15 @@ func getSubnetsForNodes(ctx context.Context, nodes []*v1.Node, client client.Int continue } - id := util.MapProviderIDToInstanceID(node.Spec.ProviderID) + if node.Spec.ProviderID == "" { + return nil, errors.Errorf(".spec.providerID was not present on node %q", node.Name) + } + + id, err := util.MapProviderIDToInstanceID(node.Spec.ProviderID) + if err != nil { + return nil, errors.Wrap(err, "MapProviderIDToInstanceID") + } + vnic, err := client.Compute().GetPrimaryVNICForInstance(ctx, compartmentID, id) if err != nil { return nil, err diff --git a/pkg/oci/util/util.go b/pkg/oci/util/util.go index 783f6185a9..b4d67bfd87 100644 --- a/pkg/oci/util/util.go +++ b/pkg/oci/util/util.go @@ -17,6 +17,7 @@ package util import ( "strings" + "github.com/pkg/errors" api "k8s.io/api/core/v1" ) @@ -28,11 +29,14 @@ const ( ) // MapProviderIDToInstanceID parses the provider id and returns the instance ocid. -func MapProviderIDToInstanceID(providerID string) string { +func MapProviderIDToInstanceID(providerID string) (string, error) { + if providerID == "" { + return providerID, errors.New("provider ID is empty") + } if strings.HasPrefix(providerID, providerPrefix) { - return strings.TrimPrefix(providerID, providerPrefix) + return strings.TrimPrefix(providerID, providerPrefix), nil } - return providerID + return providerID, nil } // NodeInternalIP returns the nodes internal ip diff --git a/pkg/oci/util/util_test.go b/pkg/oci/util/util_test.go index 189d181d06..51c05424c4 100644 --- a/pkg/oci/util/util_test.go +++ b/pkg/oci/util/util_test.go @@ -19,23 +19,34 @@ import "testing" func TestMapProviderIDToInstanceID(t *testing.T) { testCases := map[string]struct { providerID string - expected string + instanceID string + error bool }{ "no cloud prefix": { providerID: "testid", - expected: "testid", + instanceID: "testid", + error: false, }, "cloud prefix": { providerID: providerPrefix + "testid", - expected: "testid", + instanceID: "testid", + error: false, + }, + "empty string": { + providerID: "", + instanceID: "", + error: true, }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - result := MapProviderIDToInstanceID(tc.providerID) - if result != tc.expected { - t.Errorf("Expected instance id %q, but got %q", tc.expected, result) + result, err := MapProviderIDToInstanceID(tc.providerID) + if result != tc.instanceID { + t.Errorf("Expected instance id %q, but got %q", tc.instanceID, result) + } + if (err == nil && tc.error) || (!tc.error && err != nil) { + t.Errorf("Expected an error condition for input %q, but did no receive one; or received one, when not expecting", tc.providerID) } }) } diff --git a/pkg/oci/zones.go b/pkg/oci/zones.go index 06f45002e4..b02089f0b1 100644 --- a/pkg/oci/zones.go +++ b/pkg/oci/zones.go @@ -47,7 +47,10 @@ func (cp *CloudProvider) GetZone(ctx context.Context) (cloudprovider.Zone, error // particularly used in the context of external cloud providers where node // initialization must be down outside the kubelets. func (cp *CloudProvider) GetZoneByProviderID(ctx context.Context, providerID string) (cloudprovider.Zone, error) { - instanceID := util.MapProviderIDToInstanceID(providerID) + instanceID, err := util.MapProviderIDToInstanceID(providerID) + if err != nil { + return cloudprovider.Zone{}, err + } instance, err := cp.client.Compute().GetInstance(ctx, instanceID) if err != nil { return cloudprovider.Zone{}, err