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

✨ Add IPAM for nodes #142

Merged
merged 16 commits into from
Aug 30, 2024
Merged

✨ Add IPAM for nodes #142

merged 16 commits into from
Aug 30, 2024

Conversation

Mattes83
Copy link
Contributor

@Mattes83 Mattes83 commented Jun 6, 2024

What is the purpose of this pull request/Why do we need it?
We would like to get IPs from a fixed pool of IPs instead of relying on the DHCP.

Issue #, if available:
#130

Description of changes:

  • added IPv4PoolRef/IPv6PoolRef to both IonosCloudMachine and Network
  • workflow: IonosCloudMachine controller checks for PoolRefs and creates IPAddressClaims when needed. It then waits for an external controller to create IPAddress objects from the IPAddressClaim. Then it uses the IP from the IPAddress object to create a server via Ionos cloud api.

Special notes for your reviewer:
I did not write tests yet as I am waiting for #137 to be merged and I'd like to get some feedback about this PR first.
I am also unsure where I should put the docs, I did not find anything for the other api stuff beside the api definition itself. Maybe this is already enough?

Checklist:

  • Documentation updated
  • Unit Tests added
  • E2E Tests added
  • Includes emojis

@Mattes83 Mattes83 force-pushed the ipam branch 4 times, most recently from 30775fb to 354436f Compare June 10, 2024 09:06
@Mattes83
Copy link
Contributor Author

History should now be clean

Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@lubedacht
Copy link
Contributor

The commit history of this PR is somehow really broken. There are too many unrelated files marked as changed.

@Mattes83
Copy link
Contributor Author

For the documentation, I'd just write a new markdown file in docs/. Also have you tested your changes? If so, please proceed with e2e tests, as #137 has already been merged

There are now docs & e2e tests!

@Mattes83
Copy link
Contributor Author

I think I addressed all issues. Let me know if there is anything left todo

@lubedacht
Copy link
Contributor

Thank you. This PR will be the next priority after upgrading to 1.8.1.

There were some updates in the e2e framework, which have been addressed in #212 and I would suggest that we first merge the 1.8.1 PR and then have a final update here.

# Conflicts:
#	test/e2e/capic_test.go
#	test/e2e/config/ionoscloud.yaml
#	test/e2e/env_test.go
Copy link
Contributor

@lubedacht lubedacht left a comment

Choose a reason for hiding this comment

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

Only some nits from my side.

As we have discussed before, there might be upcoming work for us, but this is not part of this PR.

test/e2e/env_test.go Outdated Show resolved Hide resolved
internal/service/k8s/ipam.go Show resolved Hide resolved
lubedacht
lubedacht previously approved these changes Aug 29, 2024
Copy link

@lubedacht lubedacht merged commit 075f649 into ionos-cloud:main Aug 30, 2024
10 checks passed
@Mattes83 Mattes83 deleted the ipam branch September 2, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants