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.

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

Create Forwarding Rules provider #1759

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

panslava
Copy link
Contributor

@panslava panslava commented Jul 27, 2022

This separates "business" logic of creating forwarding rules for our controllers, from interacting with forwarding rules in google cloud

This removes copy pasted code between l4 ilb and l4 elb

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 27, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2022
"k8s.io/legacy-cloud-providers/gce"
)

type ForwardingRulesController struct {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps Manager instead of controller so it isn't considered the same as the actual controllers?

Copy link
Contributor Author

@panslava panslava Jul 27, 2022

Choose a reason for hiding this comment

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

Definitely agree, I actually wanted to ask for help with the name, cause also felt that in this project “controller” has its own meaning
Thanks!

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 am thinking, maybe ForwardingRulesService will be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I am still not very sure, and even Inteliji gives warning like "Name starts with the package name"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think interface should be named

ForwardingRulesManager (and it is good that it ends on "er")

and struct will be just

ForwardingRules (and even in Inteliji checker, they are happy with this name, as it matches the package)

@panslava panslava force-pushed the refactor-forwarding-rules branch 3 times, most recently from dc88c18 to b465bc0 Compare July 28, 2022 14:42
@panslava panslava changed the title Create forwarding rules controller Create forwarding rules service Jul 28, 2022
@panslava
Copy link
Contributor Author

panslava commented Jul 28, 2022

I am also not 100% sure about naming, should methods name contain "ForwardingRule", so we would call them

l.forwardingRules.DeleteForwardingRule

or it is better to not double the name, so they would be

l.forwardingRules.Delete

(personally, I think option 2 is better)

@panslava panslava force-pushed the refactor-forwarding-rules branch 2 times, most recently from 183b3a5 to 4055264 Compare July 28, 2022 19:23
@panslava
Copy link
Contributor Author

/assign cezarygerard

@panslava panslava changed the title Create forwarding rules service Create forwarding rules manager Jul 28, 2022
@panslava panslava force-pushed the refactor-forwarding-rules branch 5 times, most recently from e4b51b8 to 644a386 Compare July 29, 2022 10:34
@panslava panslava changed the title Create forwarding rules manager Create forwarding rules provider Jul 29, 2022
@panslava panslava changed the title Create forwarding rules provider Create Forwarding Rules provider Jul 29, 2022
@panslava
Copy link
Contributor Author

panslava commented Aug 2, 2022

/assign bowei

@panslava panslava force-pushed the refactor-forwarding-rules branch 2 times, most recently from 2881c5c to 8f33a15 Compare August 10, 2022 11:15
Copy link
Contributor

@cezarygerard cezarygerard left a comment

Choose a reason for hiding this comment

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

I see most of the forwarding rules logic iI see most of the forwarding rules logic is still in the
pkg/loadbalancers/forwarding_rules.go

do you think you can move more here?
or the rest of the logic is L4 specific?
if so

  • do you plan to split the pkg/loadbalancers/forwarding_rules.go into l4-/l4netlb-/l7-forwarding_rules.go files?s still in the

pkg/forwardingrules/forwarding_rules.go Outdated Show resolved Hide resolved
pkg/loadbalancers/forwarding_rules.go Outdated Show resolved Hide resolved
pkg/loadbalancers/forwarding_rules.go Show resolved Hide resolved
pkg/l4lb/l4netlbcontroller.go Outdated Show resolved Hide resolved
pkg/l4lb/l4netlbcontroller.go Outdated Show resolved Hide resolved
pkg/forwardingrules/forwarding_rules_test.go Outdated Show resolved Hide resolved
pkg/forwardingrules/forwarding_rules_test.go Outdated Show resolved Hide resolved
pkg/forwardingrules/forwarding_rules_test.go Outdated Show resolved Hide resolved
pkg/loadbalancers/forwarding_rules.go Outdated Show resolved Hide resolved
pkg/forwardingrules/interfaces.go Outdated Show resolved Hide resolved
@panslava
Copy link
Contributor Author

/hold

I will squash before merging (after approve)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2022
@cezarygerard
Copy link
Contributor

/lgtm
thanks for addressing the comments
please squash and unhold

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2022
This service encapsulates interactions with Cloud forwading rules
This removes copy pasted code between l4 ilb and l4 elb
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2022
@panslava
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2022
@cezarygerard
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, panslava

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 [cezarygerard,panslava]

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 merged commit 66e37b9 into kubernetes:master Aug 18, 2022
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. 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants