Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1838001: Remove remote_group_id usage at loadbalancer SGs #232

Merged
merged 1 commit into from May 20, 2020
Merged

Bug 1838001: Remove remote_group_id usage at loadbalancer SGs #232

merged 1 commit into from May 20, 2020

Conversation

luis5tb
Copy link
Contributor

@luis5tb luis5tb commented May 14, 2020

This patch removes the usage of remote_group_id at the loadbalancer
SG rules to allow the access to the namespace. It obtaines the
namespace associated range and use remote_ip_prefix instead.

Note remote_group_id is still used for the default and namespace
SG groups. We can improve it by replacing the rules inside those two
(which use remote_group_ids too) to use remote_ip_prefixes.

@openshift-ci-robot
Copy link

@luis5tb: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Remove remote_group_id usage at loadbalancer SGs

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.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2020
MaysaMacedo
MaysaMacedo reviewed May 14, 2020
View reviewed changes
Copy link
Contributor

@MaysaMacedo MaysaMacedo left a comment

Choose a reason for hiding this comment

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

Just a few questions and suggestions:

kuryr_kubernetes/controller/drivers/lbaasv2.py Show resolved Hide resolved
kuryr_kubernetes/controller/drivers/lbaasv2.py Outdated Show resolved Hide resolved
kuryr_kubernetes/controller/drivers/lbaasv2.py Outdated Show resolved Hide resolved
@luis5tb luis5tb force-pushed the remove-remote-group-ids branch 3 times, most recently from df2b9fa to 924da9f Compare May 14, 2020 16:40
@luis5tb luis5tb force-pushed the remove-remote-group-ids branch 2 times, most recently from abb7876 to 5c075f5 Compare May 18, 2020 14:28
@MaysaMacedo
Copy link
Contributor

/retest

dulek
dulek suggested changes May 18, 2020
View reviewed changes
Copy link
Contributor

@dulek dulek left a comment

Choose a reason for hiding this comment

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

Alright, looking very good. It will not cover one case, I think. I'm not sure how much we care about it really.

So basically if a global namespace is defined, but not created yet, an LB created before that ns will not have traffic from that namespace opened.

kuryr_kubernetes/controller/drivers/lbaasv2.py Outdated Show resolved Hide resolved
@luis5tb
Copy link
Contributor Author

luis5tb commented May 19, 2020

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 19, 2020
@luis5tb
Remove remote_group_id usage at loadbalancer SGs
5806814 
This patch removes the usage of remote_group_id at the loadbalancer
SG rules to allow the access to the namespace. It obtaines the
namespace associated range and use remote_ip_prefix instead.

Note remote_group_id is still used for the default and namespace
SG groups. We can improve it by replacing the rules inside those two
(which use remote_group_ids too) to use remote_ip_prefixes.

Change-Id: Ib551ac6c0e1e405e8611755f5f5a1f43f94b27bb
@luis5tb
Copy link
Contributor Author

luis5tb commented May 19, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 19, 2020
@dulek
Copy link
Contributor

dulek commented May 20, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 20, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek, luis5tb

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

@openshift-merge-robot openshift-merge-robot merged commit 374b5ba into openshift:release-3.11 May 20, 2020
@luis5tb luis5tb changed the title Remove remote_group_id usage at loadbalancer SGs Bug 1838001: Remove remote_group_id usage at loadbalancer SGs May 20, 2020
@openshift-ci-robot
Copy link

@luis5tb: All pull requests linked via external trackers have merged: . Bugzilla bug 1838001 has been moved to the MODIFIED state.

In response to this:

Bug 1838001: Remove remote_group_id usage at loadbalancer SGs

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.

@openshift-ci-robot openshift-ci-robot mentioned this pull request May 20, 2020
Merged
MaysaMacedo pushed a commit to MaysaMacedo/kuryr-kubernetes-1 that referenced this pull request Jul 1, 2021
@openshift-merge-robot
Merge pull request openshift#232 from alexanderConstantinescu/master
45b24c6 
Adding upgradeable status and setting it to always "true" for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaysaMacedo MaysaMacedo MaysaMacedo left review comments

@dulek dulek dulek approved these changes

@yboaron yboaron Awaiting requested review from yboaron

Assignees

@dulek dulek

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@luis5tb @openshift-ci-robot @MaysaMacedo @dulek @openshift-merge-robot