Bug 2037209: refactor for AlibabaResourceReference changes #26
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,8 +115,11 @@ func runInstances(machine *machinev1beta1.Machine, machineProviderConfig *machin | |
| runInstancesRequest.RegionId = machineProviderConfig.RegionID | ||
|
|
||
| // ResourceGroupID | ||
| if groupId, err := getResourceGroupId(machineProviderConfig); err != nil { | ||
| klog.Errorf("Unable to determine resource group ID for machine %q, err %q", machine.Name, err) | ||
| return nil, mapierrors.InvalidMachineConfiguration("Unable to determine resource group ID for machine: %q", machine.Name) | ||
| } else { | ||
| runInstancesRequest.ResourceGroupId = groupId | ||
| } | ||
|
|
||
| // SecurityGroupIDs | ||
|
|
@@ -367,16 +370,21 @@ func getSecurityGroupIDs(machine runtimeclient.ObjectKey, machineProviderConfig | |
| } | ||
|
|
||
| for _, sg := range machineProviderConfig.SecurityGroups { | ||
| switch sg.Type { | ||
| case machinev1.AlibabaResourceReferenceTypeID: | ||
| if sg.ID != nil && *sg.ID != "" { | ||
| securityGroupIDs = append(securityGroupIDs, *sg.ID) | ||
| } else { | ||
| return nil, mapierrors.InvalidMachineConfiguration("No security group ID provided") | ||
| } | ||
| case machinev1.AlibabaResourceReferenceTypeTags: | ||
| ids, err := getSecurityGroupIDByTags(machine, machineProviderConfig, sg.Tags, client) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| securityGroupIDs = append(securityGroupIDs, ids...) | ||
| default: | ||
| return nil, mapierrors.InvalidMachineConfiguration("Unknown security group resource reference type: %s", sg.Type) | ||
| } | ||
| } | ||
| if len(securityGroupIDs) == 0 { | ||
|
|
@@ -385,12 +393,20 @@ func getSecurityGroupIDs(machine runtimeclient.ObjectKey, machineProviderConfig | |
| return &securityGroupIDs, nil | ||
| } | ||
|
|
||
| func getSecurityGroupIDByTags(machine runtimeclient.ObjectKey, machineProviderConfig *machinev1.AlibabaCloudMachineProviderConfig, tags *[]machinev1.Tag, client alibabacloudClient.Client) ([]string, error) { | ||
| if tags == nil { | ||
| return nil, mapierrors.InvalidMachineConfiguration("No tags provided for security group ID search for machine: %q", machine.Name) | ||
| } | ||
| request := ecs.CreateDescribeSecurityGroupsRequest() | ||
| request.VpcId = machineProviderConfig.VpcID | ||
| if groupId, err := getResourceGroupId(machineProviderConfig); err != nil { | ||
| klog.Errorf("Unable to determine resource group ID for machine %q, err %q", machine.Name, err) | ||
| return nil, mapierrors.InvalidMachineConfiguration("Unable to determine resource group ID for machine: %q", machine.Name) | ||
| } else { | ||
| request.ResourceGroupId = groupId | ||
| } | ||
| request.RegionId = machineProviderConfig.RegionID | ||
| request.Tag = buildDescribeSecurityGroupsTag(*tags) | ||
| request.Scheme = "https" | ||
|
|
||
| response, err := client.DescribeSecurityGroups(request) | ||
|
|
@@ -440,28 +456,30 @@ func buildDescribeSecurityGroupsTag(tags []machinev1.Tag) *[]ecs.DescribeSecurit | |
|
|
||
| func getVSwitchID(machine runtimeclient.ObjectKey, machineProviderConfig *machinev1.AlibabaCloudMachineProviderConfig, client alibabacloudClient.Client) (string, error) { | ||
| klog.Infof("validate vswitch in region %s", machineProviderConfig.RegionID) | ||
| switch machineProviderConfig.VSwitch.Type { | ||
| case machinev1.AlibabaResourceReferenceTypeID: | ||
| if machineProviderConfig.VSwitch.ID != nil && *machineProviderConfig.VSwitch.ID != "" { | ||
| return *machineProviderConfig.VSwitch.ID, nil | ||
| } else { | ||
| return "", mapierrors.InvalidMachineConfiguration("No vswitch resource id provided") | ||
| } | ||
| case machinev1.AlibabaResourceReferenceTypeTags: | ||
| return getVSwitchIDFromTags(machine, machineProviderConfig, client) | ||
| default: | ||
| return "", mapierrors.InvalidMachineConfiguration("Unknown vswitch resource reference type: %s", machineProviderConfig.VSwitch.Type) | ||
| } | ||
| } | ||
|
|
||
| func getVSwitchIDFromTags(machine runtimeclient.ObjectKey, mpc *machinev1.AlibabaCloudMachineProviderConfig, client alibabacloudClient.Client) (string, error) { | ||
| if mpc.VSwitch.Tags == nil { | ||
| return "", mapierrors.InvalidMachineConfiguration("No tags provided for VSwitch ID search for machine: %q", machine.Name) | ||
| } | ||
| // Build a request to fetch the vSwitchID from the tags provided | ||
| describeVSwitchesRequest := vpc.CreateDescribeVSwitchesRequest() | ||
| describeVSwitchesRequest.Scheme = "https" | ||
| describeVSwitchesRequest.RegionId = mpc.RegionID | ||
| describeVSwitchesRequest.VpcId = mpc.VpcID | ||
| describeVSwitchesRequest.Tag = buildDescribeVSwitchesTag(*mpc.VSwitch.Tags) | ||
| describeVSwitchesResponse, err := client.DescribeVSwitches(describeVSwitchesRequest) | ||
| if err != nil { | ||
| metrics.RegisterFailedInstanceCreate(&metrics.MachineLabels{ | ||
|
|
@@ -801,3 +819,21 @@ func correctExistingTags(machine *machinev1beta1.Machine, regionID string, insta | |
|
|
||
| return nil | ||
| } | ||
|
|
||
| // getResourceGroupId takes an AlibabaCloudMachineProviderConfig and will return the | ||
| // resource group id if available, or determine the group id by using the search tags. | ||
| // An error will be returned if no group id can be found, or if multiple groups are | ||
| // found from the search tags. | ||
| func getResourceGroupId(machineProviderConfig *machinev1.AlibabaCloudMachineProviderConfig) (string, error) { | ||
| switch machineProviderConfig.ResourceGroup.Type { | ||
|
Contributor
There was a problem hiding this comment. What happens when resource group is nil? Is this allowed? IIRC someone said resource group was an optional field?
Contributor
Author
There was a problem hiding this comment. it seems like we fail if the resource group is nil currently, the way the code is structured i'm not sure how it's supposed to find the security groups without the group id. but maybe that's ok?
Contributor
Author
There was a problem hiding this comment. i asked around a little bit, and we should not allow creation of resources in the default group. the user should always specify the resource group.
Contributor
There was a problem hiding this comment. I saw a comment about most customers using the default group, are we sure this is correct?
Contributor
Author
There was a problem hiding this comment. sorry, i worded that poorly. we definitely should allow creation in the default group, but we should force the user to always specify the resource group so that there are no surprises when creating new machines. There was a problem hiding this comment. It looks to me that resource group will never be nil in a provider config https://github.com/openshift/api/blob/master/machine/v1/types_alibabaprovider.go#L140 it's not a pointer
Contributor
Author
There was a problem hiding this comment. +1, we agreed that the user should always provide this information, even in the case where they want to use the default resource group. this will help ensure that the user isn't creating resources in the wrong group by accident. |
||
| case machinev1.AlibabaResourceReferenceTypeID: | ||
| if machineProviderConfig.ResourceGroup.ID != nil && *machineProviderConfig.ResourceGroup.ID != "" { | ||
| return *machineProviderConfig.ResourceGroup.ID, nil | ||
| } else { | ||
| return "", mapierrors.InvalidMachineConfiguration("No resource group ID provided") | ||
| } | ||
| // TODO add name search lookup case here | ||
| default: | ||
| return "", mapierrors.InvalidMachineConfiguration("unknown resource group reference type: %s", machineProviderConfig.ResourceGroup.Type) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.