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

Do not try to update custom fields if no custom field definitions exist #82

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

tseeker
Copy link
Contributor

@tseeker tseeker commented Aug 20, 2023

When no custom fields are defined on the phpIPAM instance, it is impossible to read (or write) custom fields from an object. Because of this, updates to addresses (and presumably VLANs and subnets) on instances without custom fields would fail with an Error from API (404): No custom fields defined, as described in #81.

This patch ignores the error if the error above is returned by the API and no custom field values are to be set.

@pavel-z1 pavel-z1 self-assigned this Sep 5, 2023
@pavel-z1
Copy link
Collaborator

pavel-z1 commented Sep 5, 2023

Hi @tseeker

Thank you for your work.

I've tested in my environment with phpipam version 1.5.2.
Issue is reproduced.
But I receive different text in the response from phpipam API - Error getting custom fields for updating: Error from API (200): No custom fields defined:

Error: Error getting custom fields for updating: Error from API (200): No custom fields defined
│
│   with module.test-vm-01.phpipam_first_free_address.main_ip,
│   on ../../modules/vms/main.tf line 155, in resource "phpipam_first_free_address" "main_ip":
│  155: resource "phpipam_first_free_address" "main_ip" {

Can you please add handling of status code 200 too?

@pavel-z1 pavel-z1 added the bug Something isn't working label Sep 5, 2023
@tseeker
Copy link
Contributor Author

tseeker commented Sep 5, 2023

Hello,

Could you give me the Terraform source for the test you were running when receiving this message so I can test it on my end, please?

Thank you!

@pavel-z1
Copy link
Collaborator

pavel-z1 commented Sep 6, 2023

@tseeker

Issue reproduced on phpipam with version 1.4.1:

# cat main.tf
provider "phpipam" {
  app_id   = "terraform"
  endpoint = "http://10.211.55.18/phpipam/api"
  password = "pass"
  username = "Admin"
  insecure = false
}

terraform {
  required_providers {
    phpipam = {
      source = "lord-kyron/phpipam"
      version = "1.5.2"
    }
  }
}

data "phpipam_section" "section" {
  name = "Customers"
}

resource "phpipam_subnet" "subnet" {
  section_id     = data.phpipam_section.section.section_id
  subnet_address = "10.3.1.0"
  subnet_mask    = 24
}

// Get the first available address
data "phpipam_first_free_address" "next_address" {
  subnet_id = resource.phpipam_subnet.subnet.subnet_id
}


resource "phpipam_address" "newip" {
  subnet_id   = resource.phpipam_subnet.subnet.subnet_id
  ip_address  = "10.3.1.2"
  hostname    = "tf-test-host2.example.internal"
  description = "Managed by Terraform"

}

output "phpipam_address1" {
  value = resource.phpipam_address.newip
}

Updating hostname field for address:

# terraform apply
data.phpipam_section.section: Reading...
data.phpipam_section.section: Read complete after 0s [id=1]
phpipam_subnet.subnet: Refreshing state... [id=7]
data.phpipam_first_free_address.next_address: Reading...
phpipam_address.newip: Refreshing state... [id=11]
data.phpipam_first_free_address.next_address: Read complete after 0s [id=10.3.1.1]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following
symbols:
  ~ update in-place

Terraform will perform the following actions:

  # phpipam_address.newip will be updated in-place
  ~ resource "phpipam_address" "newip" {
      ~ hostname             = "tf-test-host.example.internal" -> "tf-test-host2.example.internal"
        id                   = "11"
        # (11 unchanged attributes hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Changes to Outputs:
  ~ phpipam_address1 = {
      ~ hostname             = "tf-test-host.example.internal" -> "tf-test-host2.example.internal"
        id                   = "11"
        # (18 unchanged attributes hidden)
    }

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

phpipam_address.newip: Modifying... [id=11]
╷
│ Error: Error getting custom fields for updating: Error from API (200): No custom fields defined
│
│   with phpipam_address.newip,
│   on main.tf line 34, in resource "phpipam_address" "newip":
│   34: resource "phpipam_address" "newip" {
│

I think it would be nice to add issue handling for previous versions of phpipam too.

@tseeker
Copy link
Contributor Author

tseeker commented Sep 6, 2023

OK. I'll install phpIPAM 1.4.1 and test against it as well when I have some time. I'll update the PR when ready. Thank you!

@lord-kyron
Copy link
Owner

@tseeker what is the status here? Were you able to test again on 1.4.1?

@tseeker
Copy link
Contributor Author

tseeker commented Oct 8, 2023

Hello, I'm sorry, I haven't had the time to work on it yet.

@@ -114,8 +116,13 @@ func updateCustomFields(d *schema.ResourceData, client interface{}) error {
panic(fmt.Errorf("Invalid client type passed %#v - this is a bug", client))
}
if err != nil {
return fmt.Errorf("Error getting custom fields for updating: %s", err)
if strings.Contains(err.Error(), "404") && len(customFields) == 0 {
Copy link
Collaborator

@pavel-z1 pavel-z1 Nov 2, 2023

Choose a reason for hiding this comment

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

I propose to change this condition to next to handle error from old phpipam versions:

if (strings.Contains(err.Error(), "404") || (strings.Contains(err.Error(), "200") && strings.Contains(err.Error(), "No custom fields defined")) && len(customFields) == 0 {

@tseeker you will have time to extend this condition?
This will allow as to accept your PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine by me. I won't have the time to test it properly in the foreseeable future though.

@lord-kyron
Copy link
Owner

@pavel-z1 I guess it is better you to review it finally.

@pavel-z1
Copy link
Collaborator

@tseeker Thank you for your work

@pavel-z1 pavel-z1 merged commit 87402a2 into lord-kyron:master Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants