✨ Add feature to create ports with custom options by macaptain · Pull Request #876 · kubernetes-sigs/cluster-api-provider-openstack

macaptain · 2021-05-21T12:03:02Z

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports.

The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the networks: field.

If networks: is not specified, only the ports specified in ports: will be created and attached to the instance. If neither networks: nor ports: are specified, the instance will be connected to the default cluster network and subnet.

This feature is very much based on the work on @jsen-, and a lot of credit goes there for the implementation:

#778

Example spec to give you some idea. Creates exactly three ports, two ports on the same network on different subnets with different VNIC types and a further example showing how to add security groups.

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
kind: OpenStackMachineTemplate
metadata:
  name: basic-1-control-plane
  namespace: default
spec:
  template:
    spec:
      ports:
      - networkId: 2f796e80-65e4-4589-b92d-77a6c019fe35
        description: primary    
        vnicType: normal
        fixedIps:
        - subnetId: 74ba4ed6-239a-44f4-81ac-95cc65b540b3
          ipAddress: 192.168.1.35
      - networkId: 2f796e80-65e4-4589-b92d-77a6c019fe35
        description: direct    
        vnicType: direct
        fixedIps:
        - subnetId: b6e81653-2bd6-4334-ab93-a7c7ec8bc66f
          ipAddress: 10.10.10.10
      - networkId: e6810d0c-85c5-41ce-b1d4-c2795611b9a0
        description: secondary
        vnicType: direct
        securityGroups:
        - one
        - two
      cloudName: openstack
      cloudsSecret:
        name: basic-1-cloud-config
      flavor: 2C-4GB-100GB
      image: Ubuntu_20.04_node

What this PR does / why we need it:
I'd like to be able to have instances on different networks with different settings per port. In particular, customizable VNIC types and customizable security groups.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #788

Special notes for your reviewer:
This PR is about trying to get #778 merged to master, so firstly, huge credit + thanks to @jsen- for the implementation (plus the OpenShift fork was a very helpful reference so thanks to those working on that too). I've taken on board the comments in the original PR and:

Rebased on master after 🏃 Add some events and refactor related to instance #862 was merged
Kept the API camel case instead of using gophercloud's structs
Fixed the API generation warnings from v1alpha4 to v1alpha3

In addition, I've changed the behavior slightly with regard to defaults:

Usually if networks: isn't specified, you get your port on the default cluster network. Now if neither networks: nor ports: is specified, you get this default.
For a port, the field NetworkID used to be required. Now if unspecified, it defaults to the default cluster network.
For a port, the field NameSuffix used to be mandatory. Now if unspecified, it defaults to the 0-based index of the port of the instance.

I've tried to keep the API consistent with this project, but also similar to the OpenShift fork, maybe @iamemilio could comment on that.

TODOs:

squashed commits
verify that the API change meets all my requirements
manual testing
add e2e tests: TODO in follow-up PR
rebase after 🏃 Refactor: use Service struct field more #871 is merged
if necessary:
- includes documentation: TODO in follow-up PR
- adds unit tests

/hold

k8s-ci-robot · 2021-05-21T12:03:10Z

Welcome @macaptain!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-05-21T12:03:10Z

Hi @macaptain. 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.

Details

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.

api/v1alpha4/types.go

hidekazuna · 2021-05-22T01:27:40Z

pkg/cloud/services/compute/instance.go

+		// inherit port security groups from the instance if not explicitly specified
+		securityGroups := net.PortOpts.SecurityGroups
+		if securityGroups == nil {
+			securityGroups = instanceSecurityGroups
+		}


Can this logic move to before 126? it is easier to understand constructing security groups(from line 114-126) then construct nets(from line 128-164).

The security groups can be set on a port and can be different per port, so I don't think we can put this before 126, before we've looped through the nets and ports in the spec.

SecurityGroups could be added as a field to the Network struct and we could set this default in lines 128-164, instead of in getOrCreatePort. I wonder also if constructing nets should be pulled into a separate function, as I agree it takes some understanding, and could also use some tests. Let me make those changes and you can tell me what you think.

Thanks, I understand we can not move this before 126. Please make a separate function as you think.

pkg/cloud/services/compute/instance.go

sbueringer · 2021-05-24T13:27:49Z

/ok-to-test

sbueringer

I gave it a first high-level review, overall lgtm.

@iamemilio if you have some time to spare to review this, that would be great :)

api/v1alpha4/types.go

pkg/cloud/services/compute/instance.go

sbueringer · 2021-05-24T13:33:30Z

pkg/cloud/services/compute/instance.go

+	// no port found, so create the port
+	description := net.PortOpts.Description
+	if description == "" {
+		description = fmt.Sprintf("Created by cluster-api-provider-openstack cluster %s", clusterName)


Should be adjusted once: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/870/files is merged (which should be soon)

I've updated to use this new function to get the default description (if none is provided in the port options).

pkg/cloud/services/compute/instance.go

api/v1alpha4/types.go

sbueringer · 2021-05-25T05:31:41Z

@macaptain I think we will get #863 and #870 merged soon, then you're a bit safer against rebase conflicts

macaptain · 2021-05-25T05:36:37Z

@macaptain I think we will get #863 and #870 merged soon, then you're a bit safer against rebase conflicts

Sure, no problem, I don't mind rebasing when it's needed.

sbueringer · 2021-05-25T05:40:56Z

@macaptain I think we will get #863 and #870 merged soon, then you're a bit safer against rebase conflicts

Sure, no problem, I don't mind rebasing when it's needed.

Yup, just wanted to let you know in case you want to wait a day or so with the rebase so you only have to do it once. Not sure how problematic to rebase the conflicts will be.

hidekazuna · 2021-05-31T07:51:03Z

@macaptain we see stacktrace in https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/876/pull-cluster-api-provider-openstack-e2e-test/1398202842900074496/artifacts/clusters/bootstrap/controllers/capo-controller-manager/capo-controller-manager-7598cb9d55-lbhgd/manager.log

We have to care about CreateBastion method as well.

macaptain · 2021-05-31T08:14:11Z

@macaptain we see stacktrace in https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/876/pull-cluster-api-provider-openstack-e2e-test/1398202842900074496/artifacts/clusters/bootstrap/controllers/capo-controller-manager/capo-controller-manager-7598cb9d55-lbhgd/manager.log

We have to care about CreateBastion method as well.

Thanks, I'll have a look at this today and try to fix up this failure.

iamemilio · 2021-06-04T15:37:55Z

lgtm :)

hidekazuna · 2021-06-04T21:54:09Z

@macaptain Is this still WIP?

jichenjc · 2021-06-07T01:10:56Z

api/v1alpha4/types.go

 	NotTagsAny      string `json:"notTagsAny,omitempty"`
 }

+type PortOpts struct {


as follow up, we may need consider add some samples in how to use this opts (not this PR)

I can certainly add additional documentation in a follow up PR.

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports. The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the `networks:` field. If `networks:` is not specified, only the ports specified in `ports:` will be created and attached to the instance. If neither `networks:` nor `ports:` are specified, the instance will be connected to the default cluster network and subnet. This feature is very much based on the work on jsen-, and a lot of credit goes there for the implementation: kubernetes-sigs#778

api/v1alpha4/types.go

macaptain · 2021-06-07T08:09:05Z

api/v1alpha4/types.go

+}
+
+type FixedIP struct {
+	SubnetID  string `json:"subnetId"`


One thought: it would be possible here is to make the type SubnetParam instead of specifying the subnet only with SubnetID. Then (with additional code) it would be possible to specify this subnet with a filter, like the networks can be specified with a filter. Would that be desirable? It would be different from the OpenShift interface. I also think it would be possible to add a SubnetFilter as an additional optional field in the future if we wanted.

That would be cool. That is probably a good idea for a lot of these fields that only take IDs as an input, but might be content for another iteration

macaptain · 2021-06-07T13:46:10Z

@macaptain Is this still WIP?

I've taken WIP off the PR. I've been testing this manually and I'm happy with the implementation. The feature meets my requirements.

The only question for me is would you prefer I use 'Filters' for NetworkId / SubnetId / SecurityGroups so that these fields can be chosen by details other than Id. If you want, I can look at implementing that before the API gets fixed in place. If not, let's look to merge this as it is.

Thanks very much!

hidekazuna · 2021-06-07T14:12:20Z

The only question for me is would you prefer I use 'Filters' for NetworkId / SubnetId / SecurityGroups so that these fields can be chosen by details other than Id. If you want, I can look at implementing that before the API gets fixed in place. If not, let's look to merge this as it is.

I do not need filters for these fields. Another PR would be proposed if someone wants 😃

/approve

k8s-ci-robot · 2021-06-07T14:12:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna, macaptain

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [hidekazuna]

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

macaptain · 2021-06-07T14:23:45Z

/remove-hold

hidekazuna · 2021-06-08T01:14:58Z

@sbueringer @jichenjc PTAL

jichenjc · 2021-06-08T01:26:52Z

/lgtm

let's merge this and continue evolution

k8s-ci-robot requested review from hidekazuna and sbueringer May 21, 2021 12:03

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 21, 2021

macaptain force-pushed the add-ports branch 2 times, most recently from b09e153 to efe3693 Compare May 21, 2021 12:05

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 21, 2021

hidekazuna reviewed May 22, 2021

View reviewed changes

macaptain force-pushed the add-ports branch from efe3693 to 8efb99d Compare May 24, 2021 08:41

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 May 24, 2021

sbueringer reviewed May 24, 2021

View reviewed changes

api/v1alpha4/types.go Outdated Show resolved Hide resolved

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

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

sbueringer reviewed May 24, 2021

View reviewed changes

iamemilio reviewed May 24, 2021

View reviewed changes

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

iamemilio reviewed May 24, 2021

View reviewed changes

api/v1alpha4/types.go Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

macaptain changed the title ~~✨ Add feature to create ports with custom options~~ ✨ WIP: Add feature to create ports with custom options May 28, 2021

macaptain force-pushed the add-ports branch from 8efb99d to 14f14bf Compare May 28, 2021 09:01

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2021

macaptain force-pushed the add-ports branch 2 times, most recently from 3f65607 to 7281a49 Compare May 31, 2021 15:12

macaptain closed this May 31, 2021

macaptain reopened this May 31, 2021

macaptain force-pushed the add-ports branch from 7281a49 to 607d731 Compare June 1, 2021 12:14

jichenjc reviewed Jun 7, 2021

View reviewed changes

macaptain commented Jun 7, 2021

View reviewed changes

macaptain force-pushed the add-ports branch from 607d731 to d32407d Compare June 7, 2021 09:52

macaptain changed the title ~~✨ WIP: Add feature to create ports with custom options~~ ✨ Add feature to create ports with custom options Jun 7, 2021

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 7, 2021

k8s-ci-robot assigned jichenjc Jun 8, 2021

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

k8s-ci-robot merged commit 62092ce into kubernetes-sigs:master Jun 8, 2021

macaptain deleted the add-ports branch June 8, 2021 05:48

This was referenced Jun 8, 2021

Document the custom ports feature #899

Closed

📖 Document custom ports feature #900

Merged

sbueringer mentioned this pull request Jun 9, 2021

✨ create ports #778

Closed

macaptain mentioned this pull request Jun 10, 2021

Port/Trunk Re-use Logic Bugs #834

Closed

Xenwar mentioned this pull request Jun 11, 2021

Add e2e test for port creation with custom options #907

Closed

macaptain mentioned this pull request Oct 6, 2021

REQUEST: New membership for macaptain kubernetes/org#3030

Closed

7 tasks

Conversation

macaptain commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 21, 2021

Uh oh!

k8s-ci-robot commented May 21, 2021

Uh oh!

Uh oh!

hidekazuna May 22, 2021

Choose a reason for hiding this comment

Uh oh!

macaptain May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidekazuna May 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbueringer commented May 24, 2021

Uh oh!

sbueringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbueringer May 24, 2021

Choose a reason for hiding this comment

Uh oh!

macaptain Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbueringer commented May 25, 2021

Uh oh!

macaptain commented May 25, 2021

Uh oh!

sbueringer commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidekazuna commented May 31, 2021

Uh oh!

macaptain commented May 31, 2021

Uh oh!

iamemilio commented Jun 4, 2021

Uh oh!

hidekazuna commented Jun 4, 2021

Uh oh!

jichenjc Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

macaptain Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

macaptain Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

iamemilio Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macaptain commented Jun 7, 2021

Uh oh!

hidekazuna commented Jun 7, 2021

Uh oh!

k8s-ci-robot commented Jun 7, 2021

Uh oh!

macaptain commented Jun 7, 2021

Uh oh!

hidekazuna commented Jun 8, 2021

Uh oh!

jichenjc commented Jun 8, 2021

Uh oh!

Reviewers

Assignees

Labels

macaptain commented May 21, 2021 •

edited

Loading

macaptain May 24, 2021 •

edited

Loading

sbueringer commented May 25, 2021 •

edited

Loading

iamemilio Jun 7, 2021 •

edited

Loading