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

Update for Netbox 2.10.5 #18

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Mordecaine
Copy link
Contributor

@Mordecaine Mordecaine commented Mar 9, 2021

  • Added ssl verify option. Now its possible to disable ssl verification.
  • Added Tag Split to key value
  • Changed Tag handling after netbox update

We updated our netbox instance. After that the following error occurred:
could not be converted to string

The problem is, that they changed the api response of a tag:

Before Update (2.8.8):

[...]
[dns_name] =>
[description] => azure_reserved
[tags] => Array
	(
		[0] => tag1
		[1] => tag2
	)

[custom_fields] => stdClass Object
	(
	)

After Update (Version: 2.10.5):

[...]
[dns_name] =>
[description] => azure_reserved
[tags] => Array
	(
		[0] => stdClass Object
			(
				[id] => 67
				[url] => https://10.138.1.131/api/extras/tags/67/
				[name] => azure_reserved
				[slug] => azure_reserved
				[color] => 9e9e9e
			)

	)

[custom_fields] => stdClass Object
	(
	)

I'm no php programmer (to be honest, I'm not a programmer at all), so my code is really bad. Maybe one of the maintainer is able to
translate my code (my bad code -> really good code)

Thanks a lot
Cheers Mordecaine

mordecaine added 3 commits March 9, 2021 17:48
- Added Tag Split to key value
- Changed Tag handling after netbox update

We updated out netbox instance. After that the following error occured:
could not be converted to string

The problem is, that they changed the api response of a tag:

[dns_name] =>
[description] => azure_reserved
[tags] => Array
	(
		[0] => tag1
		[1] => tag2
	)

[custom_fields] => stdClass Object
	(
	)

[dns_name] =>
[description] => azure_reserved
[tags] => Array
	(
		[0] => stdClass Object
			(
				[id] => 67
				[url] => https://10.138.1.131/api/extras/tags/67/
				[name] => azure_reserved
				[slug] => azure_reserved
				[color] => 9e9e9e
			)

	)

[custom_fields] => stdClass Object
	(
	)

I'm no php programmer (to be honest, I'm not a programmer at all), so my code is really bad. Maybe one of the maintainer is able to
translate my code (my bad code -> really good code)

Thanks alot
Cheers Mordecaine
- Added Tag Split to key value
- Changed Tag handling after netbox update

We updated out netbox instance. After that the following error occured:
could not be converted to string

The problem is, that they changed the api response of a tag:

Before Update (2.8.8):
[...]
[dns_name] =>
[description] => azure_reserved
[tags] => Array
	(
		[0] => tag1
		[1] => tag2
	)

[custom_fields] => stdClass Object
	(
	)

After Update (Version: 2.10.5):
[...]
[dns_name] =>
[description] => azure_reserved
[tags] => Array
	(
		[0] => stdClass Object
			(
				[id] => 67
				[url] => https://10.138.1.131/api/extras/tags/67/
				[name] => azure_reserved
				[slug] => azure_reserved
				[color] => 9e9e9e
			)

	)

[custom_fields] => stdClass Object
	(
	)

I'm no php programmer (to be honest, I'm not a programmer at all), so my code is really bad. Maybe one of the maintainer is able to
translate my code (my bad code -> really good code)

Thanks alot
Cheers Mordecaine
@Mordecaine Mordecaine marked this pull request as ready for review March 9, 2021 17:12
Copy link
Member

@luto luto left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I added two questions as comments.

@@ -27,6 +28,12 @@ private function apiRequest($method, $url, $get_params) {
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);

// If ssl_verify is set, check ssl certificate from netbox host
if ($this->ssl_verify === "0") {
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, false);
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this. Certificates for HTTPS are free and plentiful these days. You can even get them for air gapped machines using DNS validation. If you need to use an internal CA, that can be added to the certificate store of the icinga machine or PHP.

Which case or setup makes it necessary to skip certificate validation entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cloned our netbox instance and updated it to test our dependencies. We didn't wanted to create a new certificate for our clone system. So I added this option.
Generally I'm with you. Maybe the option should be enabled as default (to be honest, I didn't know how to set this option as default).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's okay to have this option - a lot of software at least offer this option - as long as we keep "sane defaults" (i.e. ssl_verify ON) i see no problem here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the option. Now its "Ignore SSL Verification". The default is false ;)

README.md Outdated
````
thisisatag::true
````
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. Do you mind having another look at these conflict markers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found it and resolved it

… So the default is false.

- Deleted merge conflict
@Mordecaine Mordecaine requested a review from luto March 11, 2021 14:47
@Mordecaine
Copy link
Contributor Author

Is there an update for this pull request?

@luto
Copy link
Member

luto commented Mar 23, 2021

github claims that "this branch cannot be rebased due to conflicts", so I held off for now. Didn't take a look at the actual problem yet, though.

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