Skip to content
This repository has been archived by the owner on Jul 16, 2024. It is now read-only.

OpenStack: Filter fixed IP addresses with CIDR notation #317

Merged
merged 1 commit into from
Mar 14, 2023
Merged

OpenStack: Filter fixed IP addresses with CIDR notation #317

merged 1 commit into from
Mar 14, 2023

Conversation

ederst
Copy link
Contributor

@ederst ederst commented Jun 14, 2022 •
edited
Loading

Currently, etcd-manager returns the first found fixed IP address on VMs with multiple IP addresses in OpenStack.

However, there are multiple implications, for example:

  • The first found IP address might not be the right one
  • The first found IP address can vary everytime etcd-manager is executed due to inconsistent sorting of the results returned by the OpenStack compute API
  • ETCD members can choose IP addresses located in different subnets which can lead to a non-functioning cluster

To mitigate this, let users specify a filter with CIDR notation so that instead of the first found fixed IP address the first IP address matching the CIDR notation will be returned.

When no filter is specified etcd-manager will still return the first found fixed IP address.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 14, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @ederst!

It looks like this is your first PR to kubernetes-sigs/etcdadm 🎉. 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/etcdadm 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 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 Jun 14, 2022
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 14, 2022
@ederst ederst force-pushed the os-restrict-fixed-ip-range branch from bcaa6d7 to 66c7f80 Compare June 15, 2022 10:44
@ederst ederst changed the title WIP: Filter fixed IP in OpenStack with CIDR Filter fixed IP addresses with CIDR notation Jun 15, 2022
@ederst ederst marked this pull request as ready for review June 15, 2022 10:54
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2022
@k8s-ci-robot k8s-ci-robot requested a review from hakman June 15, 2022 10:55
@ederst ederst force-pushed the os-restrict-fixed-ip-range branch from 66c7f80 to 1083fdf Compare June 15, 2022 10:57
@ederst ederst mentioned this pull request Jun 20, 2022
Open
@ederst ederst force-pushed the os-restrict-fixed-ip-range branch 2 times, most recently from 70e68af to 49de004 Compare June 23, 2022 21:12
@ederst ederst mentioned this pull request Jun 29, 2022
Closed
@ederst
Copy link
Contributor Author

ederst commented Jul 21, 2022

@justinsb, @hakman, @olemarkus anyone?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2022
@ederst ederst force-pushed the os-restrict-fixed-ip-range branch from b4bc58c to 71c0731 Compare August 8, 2022 11:25
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2022
@ederst ederst force-pushed the os-restrict-fixed-ip-range branch from 71c0731 to b1e37b2 Compare August 8, 2022 13:06
@ederst
Copy link
Contributor Author

ederst commented Aug 8, 2022 •
edited
Loading

@hakman, @justinsb I rebased the PR, renamed the option, and made it possible to specify multiple CIDRs.

@ederst ederst requested review from hakman and removed request for olemarkus and justinsb October 17, 2022 14:42
@ederst
Copy link
Contributor Author

ederst commented Dec 14, 2022 •
edited
Loading

@hakman, @justinsb, @olemarkus: I still want this to be merged, or a definitive no :D

@hakman
Copy link
Contributor

hakman commented Dec 14, 2022

Hey @ederst. Sorry for the delay here. I will try to put it on our discussion list at this week's call.

hakman
hakman suggested changes Mar 2, 2023
View reviewed changes
etcd-manager/cmd/etcd-manager/main.go Outdated Show resolved Hide resolved
etcd-manager/cmd/etcd-manager/main.go Outdated Show resolved Hide resolved
etcd-manager/cmd/etcd-manager/main.go Outdated Show resolved Hide resolved
@ederst ederst requested a review from hakman March 2, 2023 14:33
@ederst ederst force-pushed the os-restrict-fixed-ip-range branch from d073858 to b9cf184 Compare March 10, 2023 08:22
@hakman
Copy link
Contributor

hakman commented Mar 14, 2023

/ok-to-test

@k8s-ci-robot 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 Mar 14, 2023
hakman
hakman suggested changes Mar 14, 2023
View reviewed changes
etcd-manager/pkg/volumes/openstack/util.go Outdated Show resolved Hide resolved
etcd-manager/cmd/etcd-manager/main.go Outdated Show resolved Hide resolved
@ederst ederst force-pushed the os-restrict-fixed-ip-range branch from ad85f72 to d27b1d2 Compare March 14, 2023 12:47
@ederst ederst changed the title Filter fixed IP addresses with CIDR notation OpenStack: Filter fixed IP addresses with CIDR notation Mar 14, 2023
@ederst
OpenStack: Filter fixed IP addresses with CIDR notation
fd04eb3 
This allows filtering the fixed IP addresses returned by the OpenStack
compute API with a filter defined as CIDR notation.

When specifying the filter then the first fixed IP address matching the
CIDR notation will be returned.

If no filter is specified then the first found IP will be returned.
@ederst ederst force-pushed the os-restrict-fixed-ip-range branch from d27b1d2 to fd04eb3 Compare March 14, 2023 12:54
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ederst, hakman

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2023
@k8s-ci-robot k8s-ci-robot merged commit f089d30 into kubernetes-retired:master Mar 14, 2023
@ederst ederst mentioned this pull request Mar 24, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hakman hakman hakman approved these changes

Assignees

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ederst @k8s-ci-robot @hakman