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

Added support for updated Reserved IP behavior to existing resources (instance, network_ips) #610

Merged
merged 55 commits into from
Nov 25, 2024

Conversation

AniJ98
Copy link
Contributor

@AniJ98 AniJ98 commented Nov 8, 2024

📝 Description

This PR implements functionality and tests for managing Reserved IP addresses in the Linodego client. These changes align with the updated Linode API, providing comprehensive support for Reserved IP operations. The changes include:

Implementation of core operations:

  • Deleting a Linode while retaining the Reserved IP
  • Retrieving IP information for Linodes, including Reserved IPs
  • Listing all IP addresses, including Reserved IPs
  • Allocating and assigning new Reserved IP addresses
  • Assigning Reserved IPs to Linodes
  • Fetching particular IP address, including Reserved IPs
  • Converting between Ephemeral and Reserved IPs

Test coverage:

  • TestInstance_DeleteInstanceVariants: Verifies Reserved IP retention after Linode deletion
  • TestReservedIPAddresses_GetIPReservationStatus: Tests retrieval of Linode IP information including Reserved IPs
  • TestIPAddresses_List_smoke: Checks filtering of all IPs by the reserved field.
  • TestIPAddress_Instance_Allocate: Covers various scenarios for allocating Reserved IPs
  • TestIPAddress_Instance_ReserveIP_Assign: Tests assigning Reserved IPs to Linodes
  • TestIPAddress_GetFound: Tests fetching information about a particular IP address, including Reseved IPs
  • TestIPAddress_Update: Verifies conversion between Ephemeral and Reserved IPs

✔️ How to Test

What are the steps to reproduce the issue or verify the changes?

  1. Ensure you have a valid Linode API token.
Set up your environment:


export LINODE_TOKEN="your_token_here"

Additionally you need to set the following if you want to test it in a different environment:

export LINODE_URL="https://api.linode.com
export LINODE_API_VERSION="v4beta"
  1. Navigate to the test directory within the linodego project.
Update the LINODE_TOKEN in the Makefile:

LINODE_TOKEN="your_token_here"

Run the tests using one of the following commands:
To run all integration tests:

make testint

To run a specific test:

go test -v ./integration -run TestInstance_DeleteInstanceVariants

  1. Verify the test output for any failures or unexpected behavior.

Note:
Ensure that you have enough quota to reserve IP before adding it to an existing linode. Ensure the IPMAX limit is enough to allocate/ assign a reserved IP to a linode.

AniJ98 and others added 30 commits September 16, 2024 16:04
…d, InsuffecientPermission, ReserveIP, GetReservedIP, getReservedIPs, DeleteReservedIPs
… feature. Removed for loop to reserve IPs till limit is reached.
* build(deps): bump golang.org/x/oauth2 from 0.22.0 to 0.23.0

Bumps [golang.org/x/oauth2](https://github.com/golang/oauth2) from 0.22.0 to 0.23.0.
- [Commits](golang/oauth2@v0.22.0...v0.23.0)

---
updated-dependencies:
- dependency-name: golang.org/x/oauth2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Ran make tidy

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: ezilber-akamai <[email protected]>
* build(deps): bump golang.org/x/text from 0.17.0 to 0.18.0

Bumps [golang.org/x/text](https://github.com/golang/text) from 0.17.0 to 0.18.0.
- [Release notes](https://github.com/golang/text/releases)
- [Commits](golang/text@v0.17.0...v0.18.0)

---
updated-dependencies:
- dependency-name: golang.org/x/text
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* make tidy

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ye Chen <[email protected]>
…ing endpoints (linode#573)

* Add LKE types endpoints

* Support base struct; add NB types endpoints

* Add volume types

* Add network transfer prices

* Add price and region price structs

* Revert IPv6 fixtures

* Add missing fixtures
…ual test funcitons and removed debugging log statements
…st functions, made changes to error messages
@AniJ98 AniJ98 requested a review from a team as a code owner November 8, 2024 17:10
@AniJ98 AniJ98 requested review from lgarber-akamai and ezilber-akamai and removed request for a team November 8, 2024 17:10

reservedIP, err = createReservedIP()
if err == nil {
t.Fatalf("Expected error indicating MAX IP Reservation limit has been reached, got nil")
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically try to design tests to be account/limit-agnostic so I think we can remove this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! I will go ahead and remove this test.

// AllocateReserveIP allocates a new IPv4 address to the Account, with the option to reserve it
// and optionally assign it to a Linode.
func (c *Client) AllocateReserveIP(ctx context.Context, opts LinodeReserveIPOptions) (*InstanceIP, error) {
e := "networking/ips"
Copy link
Contributor

@lgarber-akamai lgarber-akamai Nov 14, 2024

Choose a reason for hiding this comment

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

This isn't quite related to this PR but I'm curious about what the difference between POST networking/reserved/ips and POST networking/ips (with reserved == true) is. Is the latter effectively just an alias of the former with a bit more functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good question! We are currently supporting creation of reserved IP's through both POST networking/reserved/ips and POST networking/ips (with reserved == true). The only difference is that along with creation we are also able to assign reserved IP's to linodes using POST networking/ips (with reserved == true).

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks!

@lgarber-akamai
Copy link
Contributor

lgarber-akamai commented Nov 14, 2024

This PR looks great other than a few small things!

The CI failure should have been fixed in this PR so merge from main should get the linter passing 🙂

…d field in the IPAddressUpdateOptions struct
Removed test for exceeding IP MAX and changed the type of the reserve…
network_ips.go Outdated
@@ -16,6 +17,14 @@ type LinodeIPAssignment struct {
LinodeID int `json:"linode_id"`
}

type LinodeReserveIPOptions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Could this be renamed to AllocateReserveIPOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I will make the change now

Copy link
Contributor

@lgarber-akamai lgarber-akamai left a comment

Choose a reason for hiding this comment

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

All tests are passing on my end, great work and thanks for the contribution!

Copy link
Contributor

@ezilber-akamai ezilber-akamai left a comment

Choose a reason for hiding this comment

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

Looks good and tests are passing locally. Thanks for the contribution!

@lgarber-akamai lgarber-akamai merged commit 90fe250 into linode:main Nov 25, 2024
7 checks passed
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.

5 participants