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 external ipam to subnet module #967

Merged
merged 1 commit into from
Sep 22, 2020
Merged

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Sep 21, 2020

Fixes: #966

Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

I give it a 👍 codewise.
A confirmation it solves #966 would be good.

@cmeissner
Copy link
Member

@mdellweg not sure if the change is enough. If I select External IPAM I get an extra field External IPAM Group. This is not known with your code change i guess.

@evgeni
Copy link
Member Author

evgeni commented Sep 21, 2020

what can you select there? in the API, I only see "external ipam proxy" which is the smart proxy running the ipam I guess.

here is the api documentation I am looking at: https://theforeman.org/api/2.1/apidoc/v2/subnets/create.html

@evgeni
Copy link
Member Author

evgeni commented Sep 21, 2020

Reading the code, it seems the group is not exposed via the API :(

@cmeissner
Copy link
Member

Here you see a screenshot of the UI.

image

Not so good that the field isn't exposed. So the work is harder than expected, I guess.

@evgeni
Copy link
Member Author

evgeni commented Sep 21, 2020

@cmeissner how useful (or not?) is it to set the subnet to "external" but not being able to select a group? the UI says "optional", so I'd guess it's still a bit useful?

@cmeissner
Copy link
Member

For our use case we grouping dozen of ip networks to a given site in this groups aka sections in phpIPAM. So we unfortunately need this field. :-(

@evgeni
Copy link
Member Author

evgeni commented Sep 21, 2020

but in a simpler setup, this would work without, right?

I'll open an issue against Foreman to expose that via the API, but don't expect a too quick turn around.

@cmeissner
Copy link
Member

Maybe it would work but we can't test it this way.

Thanks for opening the issue against Foreman but I am afraid that we need to find a workaround for that.

How long will it last that the above change is in a new ansible-modules version available? So we can set configure the subnets for external IPAM and its smart-proxy and only have to set the IPAM group for each by bio script.

@evgeni
Copy link
Member Author

evgeni commented Sep 21, 2020

I am planning a release (to galaxy) probably tomorrow.
Can you by any chance test before the release? Otherwise I'd merge what we have and you ping us when you installed the new version.

@mdellweg
Copy link
Member

mdellweg commented Sep 21, 2020

Is it possible that the optional option is only missing from the apipie docs? (We had that before...)

Edit: Too sad, but i found nothing in the api controller.

@evgeni
Copy link
Member Author

evgeni commented Sep 21, 2020

@mdellweg no, the controller doesn't know about it.

I've opened https://projects.theforeman.org/issues/30890

@cmeissner
Copy link
Member

Can you by any chance test before the release? Otherwise I'd merge what we have and you ping us when you installed the new version.

I can not promise anything, but I'll try it tomorrow morning to test it locally if I found time.

@cmeissner
Copy link
Member

cmeissner commented Sep 22, 2020

@evgeni I've tested your change, it works as expected.
Thanks for quick response.

As long as we don't use same subnets in different sections we can live without the group field otherwise we have to add the value by hand.

Can you give me a ping please if the new version is published to galaxy?

@evgeni
Copy link
Member Author

evgeni commented Sep 22, 2020

@cmeissner cool, thanks a ton for testing this!

I'll ping you on the release PR and you can expect it to land on galaxy something like 10 minutes after the PR is merged (yay, automation!)

@evgeni evgeni merged commit c6933c2 into theforeman:develop Sep 22, 2020
@evgeni evgeni deleted the external-ipam branch September 22, 2020 06:46
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.

Handle subnets thats use external IPAM like phpIPAM
3 participants