ENI support #15

sameshai · 2023-01-17T12:56:24Z

Signed-off-by: Sameer Shaikh [email protected]

file/provider/create_volume_access_point.go

common/messages/messages_en.go

file/provider/create_volume_access_point.go

common/messages/messages_en.go

common/vpcclient/models/subnet.go

common/vpcclient/models/share_target.go

prankulmahajan

lgtm

Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> ENI support Signed-off-by: Sameer Shaikh <[email protected]> PrimaryIP support Signed-off-by: Sameer Shaikh <[email protected]> Review comments Signed-off-by: Sameer Shaikh <[email protected]> Review comments Signed-off-by: Sameer Shaikh <[email protected]> Review comments Signed-off-by: Sameer Shaikh <[email protected]> Review comments Signed-off-by: Sameer Shaikh <[email protected]> Review comments Signed-off-by: Sameer Shaikh <[email protected]> Review comments Signed-off-by: Sameer Shaikh <[email protected]> Review comments Signed-off-by: Sameer Shaikh <[email protected]> Review comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Rebase master Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Review Comments Signed-off-by: Sameer Shaikh <[email protected]> Adding support for GetSecurityGroupByName Signed-off-by: Sameer Shaikh <[email protected]> Adding support for GetSecurityGroupByName Signed-off-by: Sameer Shaikh <[email protected]> Adding support for GetSecurityGroupByName Signed-off-by: Sameer Shaikh <[email protected]> Adding support for GetSecurityGroupByName Signed-off-by: Sameer Shaikh <[email protected]> Adding support for GetSecurityGroupByName Signed-off-by: Sameer Shaikh <[email protected]> Adding support for GetSecurityGroupByName

arahamad

Please check if you can fix these issues of taking to another PR

common/messages/messages_en.go

common/vpcclient/vpcfilevolume/list_security_groups_test.go

common/vpcclient/vpcfilevolume/list_subnets_test.go

file/provider/get_security_group.go

file/provider/get_security_group_test.go

file/provider/get_subnet.go

file/provider/get_subnet_test.go

go.mod

Signed-off-by: Sameer Shaikh <[email protected]>

common/messages/messages_en.go

file/provider/get_security_group.go

arahamad · 2023-10-05T17:29:44Z

file/provider/get_subnet.go

+		if err != nil {
+			// API call is failed
+			userErr := userError.GetUserError("ListSubnetsFailed", err)
+			return "", userErr


can we return userError.GetUserError("ListSubnetsFailed", err) itself instead of creating another object

arahamad · 2023-10-05T17:30:53Z

file/provider/get_subnet.go

+		// Iterate over the subnet list for given volume
+		if subnets != nil {
+			subnetList := subnets.Subnets
+			for _, subnetItem := range subnetList {


I think we can avoid creating subnetList object

file/provider/util.go

Signed-off-by: Sameer Shaikh <[email protected]>

arahamad

Please check my comments

common/messages/messages_en.go

arahamad · 2023-10-10T12:40:24Z

common/vpcclient/models/constants.go

 	APIGeneration = 1

 	// UserAgent identifies IKS to the RIaaS API
 	UserAgent = "IBM-Kubernetes-Service"


what would be the impact when we remove MaturityBeta = "beta"

The impact would be that by default the ENI mode will be applied i.e When user uses dp2 profile PVC will be created with ENI mode. Earlier it was VPC mode(Non-ENI).

what would be the impact for existing PVCs?

The existing PVCS there is no impact. If user deletes the PVCS the shares and mount_targets API will be able to delete file share and file share target.

common/vpcclient/models/security_group.go

arahamad · 2023-10-10T12:49:43Z

common/vpcclient/models/subnet.go

+)
+
+// Subnet ...
+type Subnet struct {


can these 3 variable replaced with SubnetRef

We can try it out but we can take this up as improvement.

arahamad · 2023-10-10T12:58:19Z

common/vpcclient/vpcfilevolume/list_subnets.go

+	var apiErr models.Error
+
+	request := vs.client.NewRequest(operation)
+	ctxLogger.Info("Equivalent curl command", zap.Reflect("URL", request.URL()), zap.Reflect("Operation", operation))


can we write log once as I see we have same at line number 71 as well

arahamad · 2023-10-10T13:49:38Z

file/provider/create_volume_access_point.go

+			}
+
+			if len(volumeAccessPointRequest.SubnetID) != 0 {
+				vpcs.Logger.Info("Using subnet...", zap.Reflect("subnetID", volumeAccessPointRequest.SubnetID))


I don't see the use of this log

arahamad · 2023-10-10T13:50:13Z

file/provider/create_volume_access_point.go

+			}
+
+			if volumeAccessPointRequest.PrimaryIP != nil {
+				vpcs.Logger.Info("Primary IP property provided using it for virtual network interface...")


arahamad · 2023-10-10T13:53:34Z

file/provider/get_security_group.go

+	"strings"
+)
+
+// / GetSecurityGroup  get the SecurityGroup based on the request


Need to update method name here

arahamad · 2023-10-10T14:18:36Z

file/provider/get_security_group.go

+		if securityGroups != nil {
+			for _, securityGroupItem := range securityGroups.SecurityGroups {
+				// Check if securityGroup is matching with requested input securityGroup name
+				if strings.EqualFold(securityGroupRequest.Name, securityGroupItem.Name) {


FYI SG can't be in capital latter

Here we are not considering the case we need to just find the matching SG. Anyways we will be changing this code for IKS network API.

arahamad · 2023-10-10T14:33:08Z

file/provider/get_security_group.go

+	return securityGroup, err
+}
+
+func (vpcs *VPCSession) getSecurityGroupByVPCAndSecurityGroupName(securityGroupRequest provider.SecurityGroupRequest) (string, error) {


Can we have different implementation like to get SG ID by using SG name i.e ibmcloud is sg test-sg --vpc r006-412635d8-56cc-4f74-b6ce-804b7cbac4d7 and if its failed then SG not found, not sure why we need pagination here

We don't have securityGroupID with us that is the whole reason we are invoking this API :) We just know the name of securityGroup and using that we are doing Get all SecurityGroups and filter it. With IKS network this wont require pagination as it will be very efficient

Signed-off-by: Sameer Shaikh <[email protected]>

common/messages/messages_en.go

skorati

@sameshai added comments here #22 (review), please check.

arahamad

lgtm,

we need to review code again before releasing CSI driver. Please track all the comments which are not incorporated into this PR

sameshai force-pushed the eni branch from fdb22b7 to 5981c21 Compare January 18, 2023 05:37

sameshai force-pushed the eni branch 2 times, most recently from e0adeae to 0b67e83 Compare May 17, 2023 07:47

sameshai commented May 22, 2023

View reviewed changes

file/provider/create_volume_access_point.go Outdated Show resolved Hide resolved

sameshai commented May 22, 2023

View reviewed changes

file/provider/create_volume_access_point.go Outdated Show resolved Hide resolved

sameshai commented May 22, 2023

View reviewed changes

file/provider/create_volume_access_point.go Outdated Show resolved Hide resolved

sameshai commented May 22, 2023

View reviewed changes

common/messages/messages_en.go Outdated Show resolved Hide resolved

sameshai commented May 22, 2023

View reviewed changes

file/provider/create_volume_access_point.go Outdated Show resolved Hide resolved

sameshai force-pushed the eni branch from 83d3467 to 0151a07 Compare June 1, 2023 09:37

sameshai commented Jun 5, 2023

View reviewed changes

common/messages/messages_en.go Show resolved Hide resolved

sameshai force-pushed the eni branch from 0200c38 to 3e59b87 Compare June 7, 2023 09:52

sameshai commented Jun 8, 2023

View reviewed changes

common/vpcclient/models/subnet.go Outdated Show resolved Hide resolved

sameshai commented Jun 8, 2023

View reviewed changes

common/vpcclient/models/share_target.go Show resolved Hide resolved

sameshai commented Jun 8, 2023

View reviewed changes

common/vpcclient/models/share_target.go Show resolved Hide resolved

prankulmahajan approved these changes Jul 6, 2023

View reviewed changes

sameshai force-pushed the eni branch 2 times, most recently from 873fa10 to bb21ac6 Compare July 24, 2023 09:13

sameshai force-pushed the eni branch 5 times, most recently from 9f83df3 to 570a9ff Compare September 11, 2023 18:40

sameshai force-pushed the eni branch from 570a9ff to adfefb0 Compare September 11, 2023 18:42

arahamad requested changes Oct 4, 2023

View reviewed changes

Adding support for GetSecurityGroupByName

2dc2601

Signed-off-by: Sameer Shaikh <[email protected]>

sameshai requested a review from arahamad October 5, 2023 10:18

ambiknai approved these changes Oct 5, 2023

View reviewed changes

arahamad requested changes Oct 5, 2023

View reviewed changes

Adding support for GetSecurityGroupByName

e887726

Signed-off-by: Sameer Shaikh <[email protected]>

sameshai force-pushed the eni branch from 8e32696 to efffbe8 Compare October 6, 2023 14:06

arahamad reviewed Oct 10, 2023

View reviewed changes

Adding support for GetSecurityGroupByName

a3053eb

Signed-off-by: Sameer Shaikh <[email protected]>

sameshai force-pushed the eni branch from efffbe8 to 5ddfdae Compare October 10, 2023 16:22

sameshai added 2 commits October 10, 2023 22:59

Review Comments

9bd9cf4

Signed-off-by: Sameer Shaikh <[email protected]>

Review Comments

6860fff

Signed-off-by: Sameer Shaikh <[email protected]>

sameshai force-pushed the eni branch from 5ddfdae to 6860fff Compare October 10, 2023 17:29

prankulmahajan reviewed Oct 11, 2023

View reviewed changes

common/messages/messages_en.go Show resolved Hide resolved

skorati reviewed Oct 11, 2023

View reviewed changes

arahamad approved these changes Oct 12, 2023

View reviewed changes

arahamad merged commit 085b5bb into master Oct 12, 2023

ENI support #15

ENI support #15

Uh oh!

Conversation

sameshai commented Jan 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prankulmahajan left a comment

Choose a reason for hiding this comment

Uh oh!

arahamad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arahamad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!