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

Auto add private ip annotation to nodes #182

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Conversation

rahulait
Copy link
Collaborator

@rahulait rahulait commented Feb 21, 2024

We added support for specifying Private IP by annotation for VLAN / VPC in PR #141. We wanted to further enhance it and automatically add annotations to nodes with the backend private ip address. This PR enables automatically adding the annotation by looking at node's private ip-address.

Key points

Currently, linode nodebalancer doesn't support any subnet outside of 192.168.128.0/17. Once nodebalancer adds support for other subnets, we need to update linode-ccm to support that.

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

@rahulait rahulait force-pushed the auto-add-priv-ip-annotation branch from d777c4b to 18c1797 Compare February 21, 2024 14:19
@AshleyDumaine AshleyDumaine self-requested a review February 21, 2024 15:49
README.md Outdated Show resolved Hide resolved
@glennpratt
Copy link
Contributor

glennpratt commented Feb 21, 2024

Overall, excited to have this automated.

I would wire things up using the GetInstanceIPAddresses method (and running it through the cache too), then using the pattern Terraform and CAPL use.

I'd also drop the configuration flag. This pattern is pretty heavily used already. When NodeBalancers change to support other backends, other IP ranges or IPv6, I think changing the code and expecting users to update is better than trying to guess what config flags they need in advance.

@rahulait rahulait force-pushed the auto-add-priv-ip-annotation branch from 857cb61 to 697e03b Compare February 23, 2024 15:23
@rahulait rahulait force-pushed the auto-add-priv-ip-annotation branch from 697e03b to 954ac2d Compare March 19, 2024 23:37
@rahulait
Copy link
Collaborator Author

rahulait commented Mar 19, 2024

@glennpratt We now need this feature as we have already added support to use vpc specific ip's as internal ip's if the cluster is running within the VPC (to prevent cilium from caching incorrect ips). We now return two internal ips (first VPC ip, second 192.XX subnet ip). Without this PR, linode-ccm tries to use the first internal ip we have for node with nodebalancers and keeps failing. We'll set autoAnnotateNode to true when installing clusters within VPC and it will detect an ip on default private subnet ip address (192.168.128.0/17), annotate the node using it and then CCM can configure nodebalancers successfully.

@rahulait
Copy link
Collaborator Author

@glennpratt Removed the flags and now relying on the linode API response for the private IP. PTAL 🙏

@glennpratt glennpratt merged commit 9b6ebcf into main Mar 21, 2024
4 checks passed
@glennpratt glennpratt deleted the auto-add-priv-ip-annotation branch March 21, 2024 15:29
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.

4 participants