Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove deprecated v1 provider id format #2122

Merged
merged 1 commit into from
Jan 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ generate-go-conversions: $(CONVERSION_GEN) ## Generate conversions go code
.PHONY: generate-templates
generate-templates: $(KUSTOMIZE) ## Generate cluster templates
$(KUSTOMIZE) build $(TEMPLATES_DIR)/cluster-template --load-restrictor LoadRestrictionsNone > $(TEMPLATES_DIR)/cluster-template.yaml
$(KUSTOMIZE) build $(TEMPLATES_DIR)/cluster-template-powervs --load-restrictor LoadRestrictionsNone > $(TEMPLATES_DIR)/cluster-template-powervs.yaml
$(KUSTOMIZE) build $(TEMPLATES_DIR)/cluster-template-powervs-cloud-provider --load-restrictor LoadRestrictionsNone > $(TEMPLATES_DIR)/cluster-template-powervs-cloud-provider.yaml
$(KUSTOMIZE) build $(TEMPLATES_DIR)/cluster-template-powervs-clusterclass --load-restrictor LoadRestrictionsNone > $(TEMPLATES_DIR)/cluster-template-powervs-clusterclass.yaml
$(KUSTOMIZE) build $(TEMPLATES_DIR)/cluster-template-vpc-clusterclass --load-restrictor LoadRestrictionsNone > $(TEMPLATES_DIR)/cluster-template-vpc-clusterclass.yaml
Expand Down
4 changes: 2 additions & 2 deletions cloud/scope/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -1077,14 +1077,14 @@ func (m *MachineScope) SetNotReady() {
func (m *MachineScope) SetProviderID(id *string) error {
// Based on the ProviderIDFormat version the providerID format will be decided.
if options.ProviderIDFormatType(options.ProviderIDFormat) == options.ProviderIDFormatV2 {
accountID, err := utils.GetAccountID()
accountID, err := utils.GetAccountIDWrapper()
if err != nil {
m.Logger.Error(err, "failed to get cloud account id", err.Error())
return err
}
m.IBMVPCMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("ibm://%s///%s/%s", accountID, m.Machine.Spec.ClusterName, *id))
} else {
Prajyot-Parab marked this conversation as resolved.
Show resolved Hide resolved
m.IBMVPCMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("ibmvpc://%s/%s", m.Machine.Spec.ClusterName, m.IBMVPCMachine.Name))
return fmt.Errorf("invalid value for ProviderIDFormat")
}
return nil
}
Expand Down
25 changes: 25 additions & 0 deletions cloud/scope/machine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client/fake"

infrav1beta2 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta2"
"sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/utils"
"sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/vpc/mock"
"sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/options"

. "github.com/onsi/gomega"
)
Expand Down Expand Up @@ -124,6 +126,29 @@ func TestNewMachineScope(t *testing.T) {
}
}

func TestSetVPCProviderID(t *testing.T) {
providerID := "foo-provider-id"

t.Run("Set Provider ID in invalid format", func(t *testing.T) {
g := NewWithT(t)
scope := setupMachineScope(clusterName, machineName, mock.NewMockVpc(gomock.NewController(t)))
options.ProviderIDFormat = string("v1")
err := scope.SetProviderID(core.StringPtr(providerID))
g.Expect(err).ToNot(BeNil())
})

t.Run("Set Provider ID in valid format", func(t *testing.T) {
g := NewWithT(t)
scope := setupMachineScope(clusterName, machineName, mock.NewMockVpc(gomock.NewController(t)))
options.ProviderIDFormat = string("v2")
utils.GetAccountIDFunc = func() (string, error) {
return "dummy-account-id", nil // Return dummy value
}
err := scope.SetProviderID(core.StringPtr(providerID))
g.Expect(err).To(BeNil())
})
}

func TestCreateMachine(t *testing.T) {
setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
t.Helper()
Expand Down
5 changes: 2 additions & 3 deletions cloud/scope/powervs_machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -954,9 +954,8 @@ func (m *PowerVSMachineScope) GetServiceInstanceID() (string, error) {
// SetProviderID will set the provider id for the machine.
func (m *PowerVSMachineScope) SetProviderID(instanceID string) error {
// Based on the ProviderIDFormat version the providerID format will be decided.
if options.ProviderIDFormatType(options.ProviderIDFormat) == options.ProviderIDFormatV1 {
m.IBMPowerVSMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("ibmpowervs://%s/%s", m.Machine.Spec.ClusterName, m.IBMPowerVSMachine.Name))
return nil
if options.ProviderIDFormatType(options.ProviderIDFormat) != options.ProviderIDFormatV2 {
return fmt.Errorf("invalid value for ProviderIDFormat")
}
m.V(3).Info("setting provider id in v2 format")

Expand Down
10 changes: 4 additions & 6 deletions cloud/scope/powervs_machine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -912,14 +912,12 @@ func TestGetMachineInternalIP(t *testing.T) {
func TestSetProviderID(t *testing.T) {
providerID := "foo-provider-id"

t.Run("Set Provider ID in v1 format", func(t *testing.T) {
t.Run("Set Provider ID in invalid format", func(t *testing.T) {
g := NewWithT(t)
scope := setupPowerVSMachineScope(clusterName, machineName, ptr.To(pvsImage), ptr.To(pvsNetwork), true, nil)
options.ProviderIDFormat = string(options.ProviderIDFormatV1)
err := scope.SetProviderID("foo-providerID")
expectedProviderID := ptr.To(fmt.Sprintf("ibmpowervs://%s/%s", scope.Machine.Spec.ClusterName, scope.IBMPowerVSMachine.Name))
g.Expect(*scope.IBMPowerVSMachine.Spec.ProviderID).To(Equal(*expectedProviderID))
g.Expect(err).To(BeNil())
options.ProviderIDFormat = string("v1")
err := scope.SetProviderID(providerID)
g.Expect(err).ToNot(BeNil())
})

t.Run("failed to get service instance ID", func(t *testing.T) {
Prajyot-Parab marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
2 changes: 2 additions & 0 deletions controllers/ibmpowervsmachine_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
"sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/powervs"
"sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/powervs/mock"
mockVPC "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/vpc/mock"
"sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/options"

"github.com/IBM-Cloud/power-go-client/power/models"
"github.com/IBM/go-sdk-core/v5/core"
Expand Down Expand Up @@ -374,6 +375,7 @@ func TestIBMPowerVSMachineReconciler_ReconcileOperations(t *testing.T) {
mockCtrl.Finish()
}
t.Run("Reconciling creating IBMPowerVSMachine ", func(t *testing.T) {
options.ProviderIDFormat = string("v2")
t.Run("Should requeue if Cluster infrastructure status is not ready", func(t *testing.T) {
g := NewWithT(t)
setup(t)
Expand Down
5 changes: 5 additions & 0 deletions controllers/ibmvpcmachine_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
infrav1beta2 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta2"
"sigs.k8s.io/cluster-api-provider-ibmcloud/cloud/scope"
gtmock "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/globaltagging/mock"
"sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/utils"
vpcmock "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/vpc/mock"

. "github.com/onsi/gomega"
Expand Down Expand Up @@ -327,6 +328,10 @@ func TestIBMVPCMachineLBReconciler_reconcile(t *testing.T) {
return gomock.NewController(t), mockvpc, mockgt, machineScope, reconciler
}

utils.GetAccountIDFunc = func() (string, error) {
return "dummy-account-id", nil // Return dummy value
}

t.Run("Reconcile creating IBMVPCMachine associated with LoadBalancer", func(t *testing.T) {
instancelist := &vpcv1.InstanceCollection{
Instances: []vpcv1.Instance{
Expand Down
10 changes: 3 additions & 7 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,10 @@ func initFlags(fs *pflag.FlagSet) {
}

func validateFlags() error {
switch options.ProviderIDFormatType(options.ProviderIDFormat) {
// Deprecated: ProviderIDFormatV1 is deprecated and will be removed in a future release.
case options.ProviderIDFormatV1:
setupLog.Info("Using v1 version of ProviderID format.V1 is deprecated and will be removed in a future release.Instead use V2.")
case options.ProviderIDFormatV2:
if options.ProviderIDFormatType(options.ProviderIDFormat) == options.ProviderIDFormatV2 {
setupLog.Info("Using v2 version of ProviderID format")
default:
return fmt.Errorf("invalid value for flag provider-id-fmt: %s, Supported values: %s, %s ", options.ProviderIDFormat, options.ProviderIDFormatV1, options.ProviderIDFormatV2)
} else {
return fmt.Errorf("invalid value for flag provider-id-fmt: %s, Only supported value is %s", options.ProviderIDFormat, options.ProviderIDFormatV2)
}

if err := logsv1.ValidateAndApply(logOptions, nil); err != nil {
Expand Down
8 changes: 8 additions & 0 deletions pkg/cloud/services/utils/accounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@ func GetAccount(auth core.Authenticator) (string, error) {
return token.Claims.(jwt.MapClaims)["account"].(map[string]interface{})["bss"].(string), nil
}

// GetAccountIDFunc is a variable that will hold the function reference.
var GetAccountIDFunc = GetAccountID // Default to the original function

// GetAccountIDWrapper is a function that calls GetAccountIDFunc.
func GetAccountIDWrapper() (string, error) {
return GetAccountIDFunc() // Call the function that GetAccountIDFunc points to
}

// GetAccountID will parse and returns user cloud account ID.
func GetAccountID() (string, error) {
auth, err := authenticator.GetAuthenticator()
Expand Down
7 changes: 0 additions & 7 deletions pkg/options/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,6 @@ package options
type ProviderIDFormatType string

const (
// ProviderIDFormatV1 will set provider id to machine as follows
// For VPC machines: ibmvpc://<cluster_name>/<vm_hostname>
// For Power VS machines: ibmpowervs://<cluster_name>/<vm_hostname>
// Deprecated: ProviderIDFormatV1 is deprecated and will be removed in a future release. see https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/issues/1868 for more details.
// use ProviderIDFormatV2 instead ProviderIDFormatV1.
ProviderIDFormatV1 ProviderIDFormatType = "v1"

// ProviderIDFormatV2 will set provider id to machine as follows
// For VPC machines: ibm://<account_id>///<cluster_id>/<vpc_machine_id>
// For Power VS machines: ibmpowervs://<region>/<zone>/<service_instance_id>/<powervs_machine_id>
Expand Down
Loading