Skip to content

OpenStack: support user provided dual-stack api and ingress Port#7133

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
shiftstack:include-provided-port
Jun 12, 2023
Merged

OpenStack: support user provided dual-stack api and ingress Port#7133
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
shiftstack:include-provided-port

Conversation

@MaysaMacedo
Copy link
Contributor

@MaysaMacedo MaysaMacedo commented Apr 26, 2023

When using dual-stack installations the user needs to pre-create the api and ingress port given OpenStack does not allow direct assignment of addresses when using slaac/stateless, consequently the installer can't create those. This commit adds support to tag those Ports, assign security groups to them, attach the Floating IP when needed and allow clean up of resources.

Partially implements: openshift/enhancements#1365

@openshift-ci openshift-ci bot requested review from gryf and mdbooth April 26, 2023 10:40
@MaysaMacedo MaysaMacedo force-pushed the include-provided-port branch 3 times, most recently from a5cf3d3 to ba4a314 Compare April 27, 2023 13:32
@MaysaMacedo MaysaMacedo changed the title OpenStack: support user provided api and ingress Port OpenStack: support user provided dual-stack api and ingress Port Apr 27, 2023
@MaysaMacedo
Copy link
Contributor Author

/retest

@MaysaMacedo
Copy link
Contributor Author

MaysaMacedo commented May 3, 2023

I will add validation that requires the API VIPs being in one unique Port and the same for Ingress on the BYON PR because I want to use the Subnets ID when querying for the Port.
#6797

@EmilienM
Copy link
Member

EmilienM commented May 3, 2023

/test e2e-openstack-proxy

2 similar comments
@MaysaMacedo
Copy link
Contributor Author

/test e2e-openstack-proxy

@EmilienM
Copy link
Member

EmilienM commented May 3, 2023

/test e2e-openstack-proxy

@EmilienM
Copy link
Member

EmilienM commented May 3, 2023

@mandre @pierreprinetti I'll like your opinion as well. See my comments.

@EmilienM
Copy link
Member

EmilienM commented May 3, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2023
@MaysaMacedo MaysaMacedo force-pushed the include-provided-port branch from ba4a314 to 65f609d Compare May 5, 2023 19:50
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 5, 2023
@MaysaMacedo MaysaMacedo force-pushed the include-provided-port branch 2 times, most recently from dcdd1bc to cd4a223 Compare May 9, 2023 09:25
@MaysaMacedo
Copy link
Contributor Author

/test e2e-openstack-proxy

@MaysaMacedo
Copy link
Contributor Author

@EmilienM @mdbooth @pierreprinetti
Thanks for your review! This PR is ready for another look

@EmilienM
Copy link
Member

/lgtm
Matt's comment was well addressed, thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2023
Comment on lines +93 to +103
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are called "ports" but only hold a single port?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to follow the same logic of the example from terraform https://registry.terraform.io/providers/terraform-provider-openstack/openstack/latest/docs/data-sources/networking_port_ids_v2.

Note this is filtering Ports based on the attributes of fixed_ip and network and returns a list.

@MaysaMacedo MaysaMacedo force-pushed the include-provided-port branch from cd4a223 to 9aa9040 Compare May 15, 2023 11:02
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 15, 2023
@dulek
Copy link
Contributor

dulek commented May 15, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2023
When using dual-stack installations the user needs to pre-create
the api and ingress port given OpenStack does not allow direct
assignment of addresses when using slaac/stateless, consequently
the installer can't create those. This commit adds support to tag
those Ports, assign security groups to them, attach the Floating IP
when needed and allow clean up of resources.
@MaysaMacedo MaysaMacedo force-pushed the include-provided-port branch from 9aa9040 to 229feb0 Compare May 15, 2023 14:35
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 15, 2023
@dulek
Copy link
Contributor

dulek commented May 15, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2023
@MaysaMacedo
Copy link
Contributor Author

@EmilienM could you take another look for the /approve?

@EmilienM
Copy link
Member

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2023
@stbenjam
Copy link
Member

Presubmit failures don't look great since they're installer failures. In the feature queue spreadsheet there's 4 PR's listed - are they all co-dependent?

@MaysaMacedo
Copy link
Contributor Author

/retest

@stbenjam No, they're not dependent on one another.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 31, 2023

@MaysaMacedo: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@MaysaMacedo
Copy link
Contributor Author

@stbenjam o/ all the tests passed. Can you take another look? Thanks

@deepsm007
Copy link
Contributor

/label jira/valid-bug

@openshift-ci openshift-ci bot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Jun 12, 2023
@openshift-merge-robot openshift-merge-robot merged commit 15403a5 into openshift:master Jun 12, 2023
@MaysaMacedo MaysaMacedo deleted the include-provided-port branch June 12, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants