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

Filter NodeBalancers when we know their ipv4 #126

Merged
merged 4 commits into from
Mar 24, 2023

Conversation

okokes-akamai
Copy link
Contributor

General:

  • Have you removed all sensitive information, including but not limited to access keys and passwords?
  • Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

  1. Does your submission pass tests?
  2. Have you added tests?
  3. Are you addressing a single feature in this PR?
  4. Are your commits atomic, addressing one change per commit?
  5. Are you following the conventions of the language?
  6. Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. Have you explained your rationale for why this feature is needed?
  8. Have you linked your PR to an open issue

@git-it127
Copy link

I see the TODO comment in the code here about updating to LinodeGo v1. Is there a reason these were separated into two different PRs? It seems like the plan would be to keep these two together.

LinodeGo update PR: #118

@okokes-akamai
Copy link
Contributor Author

The original plan was to do these two things together (and PR 118 had these two changes baked in), but then we found out the v1 update will cause an issue wrt env variables - we can't use our current LINODE_URL env overrides, because their handling in linodego v0.3.2 and v1 is different in an incompatible way.

So in order to expedite this server-side filter, we decided to split the work in two.

Copy link

@git-it127 git-it127 left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I ran the e2e tests and they all passed. I have a few minor comments.

// return nil, err
// }
filter := fmt.Sprintf(`{"ipv4": "%v"}`, ipv4)
lbs, err := l.client.ListNodeBalancers(ctx, &linodego.ListOptions{Filter: filter})
if err != nil {
return nil, err
}

Choose a reason for hiding this comment

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

It seems the loop below this may no longer be needed if we are getting the exact ip. Or it could be adjusted to be cleaner. Although leaving it how it is I don't think will harm anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Did you have something like this in mind?

if len(lbs) == 0 {
    return nil, lbNotFoundError{serviceNn: getServiceNn(service)}
}
klog.V(2).Infof("found NodeBalancer (%d) for service (%s) via IPv4 (%s)", lbs[0].ID, getServiceNn(service), ipv4)
return &lbs[0], nil

Choose a reason for hiding this comment

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

yes I like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed.

cloud/linode/loadbalancers.go Outdated Show resolved Hide resolved
@okokes-akamai okokes-akamai mentioned this pull request Mar 14, 2023
10 tasks
Copy link

@git-it127 git-it127 left a comment

Choose a reason for hiding this comment

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

LGTM

@okokes-akamai okokes-akamai merged commit 44660b8 into linode:master Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants