✨ AccessSubnetUUID: we can specify source subnet for access IP address #756

jsen- · 2021-02-24T19:49:08Z

What this PR does / why we need it:
Currently, when we want to use multiple networks, the IP address is assigned basically randomly, this PR adds the ability to specify from which subnet the IP address will be obtained.

Which issue(s) this PR fixes:
none

Special notes for your reviewer:
This is the minimal amount of changes I found to achieve the goal of getting IP address from the correct CIDR.

Release note:

Added the ability to specify source subnet of compute instance access IP address

k8s-ci-robot · 2021-02-24T19:49:16Z

Hi @jsen-. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/cloud/services/compute/instance.go

k8s-ci-robot · 2021-02-24T20:24:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsen-, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sbueringer · 2021-02-24T20:24:41Z

/ok-to-test

sbueringer · 2021-02-24T20:24:56Z

/test pull-cluster-api-provider-openstack-test

fejta-bot · 2021-02-24T23:07:27Z

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.
/check-cla

hidekazuna · 2021-02-25T00:06:21Z

api/v1alpha3/openstackmachine_types.go

@@ -57,6 +57,10 @@ type OpenStackMachineSpec struct {
 	// A networks object. Required parameter when there are multiple networks defined for the tenant.
 	// When you do not specify the networks parameter, the server attaches to the only network created for the current tenant.
 	Networks []NetworkParam `json:"networks,omitempty"`
+
+	// IP address of a port from this subnet will be marked as AccessIPv4 on the created compute instance


I wonder what happens if networks and subnet which is not part of networks are given.
If we do not accept the case comment it here. Or add the check logic.

I guess below discussion covered this? maybe need some comments update?

if AccessSubnet is set we will get a hit here and have a value for accessIPv4 otherwise accessIPv4 and we have no change to before this PR

Ok, got it.

I added f5ff1bf.
The instance creation will fail if no ports with fixed IP are found in the specified subnet. This also covers the case when this subnet does not exist or is not in .Networks.
I didn't notice the ...

Ok, got it.

... until now, so please let me know if you'd like to keep this change. I can drop this commit if not needed.

@jsen- Thanks for adding checking logic which is better to be added. Keep the commit, please.

hidekazuna · 2021-02-25T00:07:12Z

api/v1alpha3/openstackmachine_types.go

@@ -57,6 +57,10 @@ type OpenStackMachineSpec struct {
 	// A networks object. Required parameter when there are multiple networks defined for the tenant.
 	// When you do not specify the networks parameter, the server attaches to the only network created for the current tenant.
 	Networks []NetworkParam `json:"networks,omitempty"`
+
+	// IP address of a port from this subnet will be marked as AccessIPv4 on the created compute instance
+	AccessSubnetUUID string `json:"accessSubnetUuid,omitempty"`


Other variables does not start with Access. Subnet is easy to understand.

renamed AccessSubnetUUID to Subnet

@jsen- Thanks updating.

sbueringer · 2021-02-25T17:19:43Z

api/v1alpha3/types.go

@@ -132,6 +132,7 @@ type Instance struct {
 	FailureDomain  string            `json:"failureDomain,omitempty"`
 	SecurityGroups *[]string         `json:"securigyGroups,omitempty"`
 	Networks       *[]Network        `json:"networks,omitempty"`
+	AccessSubnet   string            `json:"accessSubnet,omitempty"`


hm does it make sense to call the property in instance differently than the one in the spec?

Hm maybe I missed something / or read it incorrectly. Could it be that this adds the Subnet property to the OpenStackMachine and the AccessSubnet to the Instance struct which seems to be only used for the status of the Bastion host?

Instance struct seems to be used as a status of bastion and as a holder of state of a created VM, but at the same time, it is used as an input for createInstance. I don't know why this is so, but I wanted to minimize the impact of this PR.
There are basically two possibilities how to forward the subnet UUID into the createInstance function:

add this property to this Instance struct (what I did)

add another (optional) parameter to createInstance function
The second option seemed inappropriate, taking into consideration the current signature:

func createInstance(is *Service, clusterName string, i *infrav1.Instance) (*infrav1.Instance, error)

Adding accessSubnetUUID string does not feel right, but now that I'm thinking about it, the advantage would be that it would not change the public contract in bastion host status.

Just let me know, I can add another commit and forward the subnet UUID in an additional argument.

It also might make sense to keep it as it is and add the same possibility to specify subnet UUID for bastion host as well (adding similar Subnet property to Bastion struct

cluster-api-provider-openstack/api/v1alpha3/types.go

Lines 251 to 266 in 437e82f

type Bastion struct {

//+optional

Enabled bool `json:"enabled"`

//+optional

Flavor string `json:"flavor,omitempty"`

//+optional

Image string `json:"image,omitempty"`

//+optional

SSHKeyName string `json:"sshKeyName,omitempty"`

//+optional

Networks []NetworkParam `json:"networks,omitempty"`

//+optional

FloatingIP string `json:"floatingIP,omitempty"`

//+optional

SecurityGroups []SecurityGroupParam `json:"securityGroups,omitempty"`

}

)
Then having this property in Bastion status starts making sense.

hidekazuna · 2021-02-26T00:01:41Z

api/v1alpha3/types.go

@@ -132,6 +132,7 @@ type Instance struct {
 	FailureDomain  string            `json:"failureDomain,omitempty"`
 	SecurityGroups *[]string         `json:"securigyGroups,omitempty"`
 	Networks       *[]Network        `json:"networks,omitempty"`
+	AccessSubnet   string            `json:"accessSubnet,omitempty"`


AccessSubnet is also better to be renamed to Subnet.

hidekazuna · 2021-02-26T00:02:39Z

pkg/cloud/services/compute/instance.go

@@ -221,6 +229,10 @@ func createInstance(is *Service, clusterName string, i *infrav1.Instance) (*infr
 			}
 		}
 	}
+	if i.AccessSubnet != "" && accessIPv4 == "" {
+		return nil, fmt.Errorf("no ports with fixed IPs found on AccessSubnet \"%s\"", i.AccessSubnet)


If AccessSubnet is renamed to Subnet, This comment would be updated.

hidekazuna · 2021-02-26T00:40:18Z

Looks good to me now.

jichenjc · 2021-02-26T00:49:17Z

/lgtm

sbueringer · 2021-02-26T05:02:11Z

@jichenjc @hidekazuna I would have preferred to be able to at least take a quick look at the final implementation before merge.
It's not a big problem in this case, but what do you think about defaulting to /hold on PRs to avoid instant merge on-lgtm? #758

AccessSubnet

789e80e

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 24, 2021

k8s-ci-robot requested review from dims and jichenjc February 24, 2021 19:49

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 24, 2021

sbueringer approved these changes Feb 24, 2021

View reviewed changes

pkg/cloud/services/compute/instance.go Show resolved Hide resolved

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 24, 2021

jsen- changed the title ~~AccessSubnetUUID: we can specify source subnet for access IP address~~ ✨ AccessSubnetUUID: we can specify source subnet for access IP address Feb 24, 2021

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2021

hidekazuna reviewed Feb 25, 2021

View reviewed changes

jsen- added 2 commits February 25, 2021 10:33

fail if no ports found on access subnet

f5ff1bf

rename AccessSubnetUUID to Subnet

cf21de2

sbueringer reviewed Feb 25, 2021

View reviewed changes

hidekazuna reviewed Feb 26, 2021

View reviewed changes

rename Instance.AccessSubnet to Subnet

1861ace

jsen- force-pushed the access-subnet branch from 186c5a7 to 1861ace Compare February 26, 2021 00:09

k8s-ci-robot assigned jichenjc Feb 26, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2021

k8s-ci-robot merged commit 7b62250 into kubernetes-sigs:master Feb 26, 2021

sbueringer mentioned this pull request Mar 25, 2021

✨ create ports #778

Closed

	type Bastion struct {
	//+optional
	Enabled bool `json:"enabled"`
	//+optional
	Flavor string `json:"flavor,omitempty"`
	//+optional
	Image string `json:"image,omitempty"`
	//+optional
	SSHKeyName string `json:"sshKeyName,omitempty"`
	//+optional
	Networks []NetworkParam `json:"networks,omitempty"`
	//+optional
	FloatingIP string `json:"floatingIP,omitempty"`
	//+optional
	SecurityGroups []SecurityGroupParam `json:"securityGroups,omitempty"`
	}

✨ AccessSubnetUUID: we can specify source subnet for access IP address #756

✨ AccessSubnetUUID: we can specify source subnet for access IP address #756

Uh oh!

Conversation

jsen- commented Feb 24, 2021

Uh oh!

k8s-ci-robot commented Feb 24, 2021

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 24, 2021

Uh oh!

sbueringer commented Feb 24, 2021

Uh oh!

sbueringer commented Feb 24, 2021

Uh oh!

fejta-bot commented Feb 24, 2021

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!

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!

hidekazuna commented Feb 26, 2021

Uh oh!

jichenjc commented Feb 26, 2021

Uh oh!

sbueringer commented Feb 26, 2021

Uh oh!

Uh oh!