Allow for possibility of private IP #172

djberg96 · 2017-11-22T21:17:41Z

Handle the possibility that the user may not want to assign a Public IP to a VM when provisioning since the Azure portal allows this.

~~WIP for now until I know for sure what the UI option is. I'll update the specs once that's settled.~~

Addresses https://bugzilla.redhat.com/show_bug.cgi?id=1497202

On a side note, I also scoped out the Armrest related Azure classes to avoid any potential class name conflicts. We already do this in the refresh parser.

miq-bot · 2017-12-13T19:38:55Z

This pull request is not mergeable. Please rebase and repush.

Redefine allowed_floating_ip_addresses. Use integers instead of strings to avoid sorting errors on UI side, and update text slightly. Update nic_id handling based on floating_ip value. Remove explicit 'None' option since UI already effectively provides it. Update nic_id handling, force scope for Azure::Armrest classes. Update expected count, and add a test, for allowed floating ips. Update test for change to cloning rules. Use options hash to distinguish between npublic IP and private IP. Default to creating a new NIC if there is a floating IP but no associated port can be found.

miq-bot · 2017-12-18T17:03:36Z

Checked commit https://github.com/djberg96/manageiq-providers-azure/commit/70fbc2f62b293ad1490a1530b98ae28473e6632a with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 1 offense detected

spec/models/manageiq/providers/azure/cloud_manager/provision_spec.rb

❗ - Line 102, Col 60 - Style/BracesAroundHashParameters - Redundant curly braces around a hash parameter.

bronaghs

I have inserted a few questions inline

bronaghs · 2017-12-18T19:21:24Z

app/models/manageiq/providers/azure/cloud_manager/provision/cloning.rb

+    # desire for a new Public IP address vs a private IP.
+    #
+    if floating_ip
+      nic_id = associated_nic || create_nic(true)


@djberg96 - In what scenario will create_nic on this line be called? Looks like this line will only be executed if floating_ip is not nil, which means a valid floating IP was selected and there is no need to create a new one.

This was something the specs caught. It's possible that a public IP exists, but isn't actually associated with a NIC yet.

Ah I see. So this means when the user selects a floating ip from the drop down that is not associated with a NIC we create a new IP called "<dest_name>-PublicIp" which isn't what the user intended.
This is what we have always done, I wonder should we at least add some info logging since we are doing something the user didn't intend?
Or should we exclude any floating IPs without a NIC since they cannot be used.

The other option is to just create the NIC dynamically if it's not associated with a NIC, and then associate it with the newly created NIC. Excluding them would be easier, though.

In any case, I think that's something that should be in a separate PR. I don't get the impression that people create public IP's independently of a NIC much in practice.

yeah it has always been there so i think its fine if it is addressed in a separate PR.

bronaghs · 2017-12-18T19:43:35Z

app/models/manageiq/providers/azure/cloud_manager/provision/cloning.rb

+      nic_id = associated_nic || create_nic(true)
+    else
+      public_ip = options[:floating_ip_address].first == -1
+      nic_id = create_nic(public_ip)


so the else statement will be executed if floating_ip is nil which would mean either:

the floating_ip dropdown was ignored altogether
or

"New" was selected and therefore the look up in the options helper returns nil - refer to lookup here

bronaghs · 2017-12-18T19:46:04Z

app/models/manageiq/providers/azure/cloud_manager/provision/cloning.rb

+    if floating_ip
+      nic_id = associated_nic || create_nic(true)
+    else
+      public_ip = options[:floating_ip_address].first == -1


@djberg96 - Can you point me to where the options hash gets populated? What does this line mean?

It's coming from the UI. The line in question is https://github.com/djberg96/manageiq-providers-azure/blob/70fbc2f62b293ad1490a1530b98ae28473e6632a/app/models/manageiq/providers/azure/cloud_manager/provision_workflow.rb#L30

I was looking for the code that populates the options hash with all the valid floating Ips.
Does this line return what the user selected from the floating_ip drop down?

I'm not sure where it's populated originally, somewhere on the UI side. Yes, that line returns what the user selected as a key/value pair on a typical Rails dropdown menu. I interjected the -1 there for us to distinguish between "create new" and "not selected".

I would have preferred to use a string key to be more descriptive, but the UI code blew up when it tried to sort it.

So the reason I was looking for the code that populates the options hash is because this line returns an array of floating ips, so its a multi-select? Will first always be -1?

Oops I misspoke. The options[:floating_ip_address] will be a two element array of the currently selected item, i.e. [-1, "New"]. If the user actually selects something, then it will be something else.

bronaghs · 2017-12-18T19:46:21Z

app/models/manageiq/providers/azure/cloud_manager/provision/cloning.rb

        }
      }
    }

+    # The -1 value is set in ProvisionWorkflow to distinguish between the
+    # desire for a new Public IP address vs a private IP.


Is the word "private" correct here?

Not sure what you're asking. If a Public IP isn't assigned, Azure will automatically assign a private IP.

In reality, just poking around on the Azure portal, it appears that NIC's with a public IP still have a private IP as well.

bronaghs · 2017-12-18T20:17:14Z

Can the key to the "New" value also be "New" instead of -1?

djberg96 · 2017-12-18T20:18:46Z

Nope, the UI blew up when I tried that due to some sorting code. I started with that. :)

bronaghs · 2017-12-18T20:19:38Z

Yea the key is the floating IP Id which is an integer.

bronaghs · 2017-12-19T17:14:07Z

Looks great @djberg96 !

Allow for possibility of private IP (cherry picked from commit 9b9fba3) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1530728

simaishi · 2018-01-03T16:57:52Z

Gaprindashvili backport details:

$ git log -1
commit e58c0038317b883ff512f2ad25133c02d5fce68a
Author: Bronagh Sorota <[email protected]>
Date:   Tue Dec 19 12:13:59 2017 -0500

    Merge pull request #172 from djberg96/optional_nic
    
    Allow for possibility of private IP
    (cherry picked from commit 9b9fba302552a6d0cbceeb98b4620d02c8b3f3ee)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1530728

Allow for possibility of private IP (cherry picked from commit 9b9fba3) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1531256

simaishi · 2018-01-04T20:08:50Z

Fine backport details:

$ git log -1
commit e63af8e39e0d9283bdf42d5b3c67bad5ba379be7
Author: Bronagh Sorota <[email protected]>
Date:   Tue Dec 19 12:13:59 2017 -0500

    Merge pull request #172 from djberg96/optional_nic
    
    Allow for possibility of private IP
    (cherry picked from commit 9b9fba302552a6d0cbceeb98b4620d02c8b3f3ee)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1531256

miq-bot added the wip label Nov 22, 2017

djberg96 changed the title ~~[WIP] Allow for possibility of empty NIC~~ [WIP] Allow for possibility of private IP Nov 28, 2017

miq-bot added unmergeable and removed unmergeable labels Dec 13, 2017

djberg96 changed the title ~~[WIP] Allow for possibility of private IP~~ Allow for possibility of private IP Dec 15, 2017

djberg96 requested a review from bronaghs December 15, 2017 17:12

miq-bot removed the wip label Dec 15, 2017

djberg96 changed the title ~~Allow for possibility of private IP~~ [WIP] Allow for possibility of private IP Dec 15, 2017

miq-bot added the wip label Dec 15, 2017

djberg96 changed the title ~~[WIP] Allow for possibility of private IP~~ Allow for possibility of private IP Dec 18, 2017

miq-bot removed the wip label Dec 18, 2017

bronaghs reviewed Dec 18, 2017

View reviewed changes

bronaghs approved these changes Dec 19, 2017

View reviewed changes

bronaghs merged commit 9b9fba3 into ManageIQ:master Dec 19, 2017

bronaghs added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 19, 2017

bronaghs added the enhancement label Dec 19, 2017

djberg96 added fine/yes labels Dec 19, 2017

simaishi pushed a commit that referenced this pull request Jan 3, 2018

Merge pull request #172 from djberg96/optional_nic

e58c003

Allow for possibility of private IP (cherry picked from commit 9b9fba3) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1530728

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 3, 2018

simaishi pushed a commit that referenced this pull request Jan 4, 2018

Merge pull request #172 from djberg96/optional_nic

e63af8e

Allow for possibility of private IP (cherry picked from commit 9b9fba3) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1531256

simaishi added fine/backported and removed fine/yes labels Jan 4, 2018

djberg96 mentioned this pull request Feb 19, 2018

Allow users to choose public or private IP when provisioning multiple VMs #210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for possibility of private IP #172

Allow for possibility of private IP #172

djberg96 commented Nov 22, 2017 •

edited

Loading

miq-bot commented Dec 13, 2017

miq-bot commented Dec 18, 2017

bronaghs left a comment

bronaghs Dec 18, 2017

djberg96 Dec 18, 2017

bronaghs Dec 18, 2017 •

edited

Loading

djberg96 Dec 18, 2017

bronaghs Dec 18, 2017

bronaghs Dec 18, 2017 •

edited

Loading

djberg96 Dec 18, 2017

bronaghs Dec 18, 2017

djberg96 Dec 18, 2017

bronaghs Dec 18, 2017 •

edited

Loading

djberg96 Dec 18, 2017

bronaghs Dec 18, 2017

djberg96 Dec 18, 2017

bronaghs Dec 18, 2017

djberg96 Dec 18, 2017

bronaghs Dec 18, 2017

djberg96 Dec 18, 2017

bronaghs commented Dec 18, 2017

djberg96 commented Dec 18, 2017

bronaghs commented Dec 18, 2017

bronaghs commented Dec 19, 2017

simaishi commented Jan 3, 2018

simaishi commented Jan 4, 2018

Allow for possibility of private IP #172

Allow for possibility of private IP #172

Conversation

djberg96 commented Nov 22, 2017 • edited Loading

miq-bot commented Dec 13, 2017

miq-bot commented Dec 18, 2017

bronaghs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bronaghs Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bronaghs Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bronaghs Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bronaghs commented Dec 18, 2017

djberg96 commented Dec 18, 2017

bronaghs commented Dec 18, 2017

bronaghs commented Dec 19, 2017

simaishi commented Jan 3, 2018

simaishi commented Jan 4, 2018

djberg96 commented Nov 22, 2017 •

edited

Loading

bronaghs Dec 18, 2017 •

edited

Loading

bronaghs Dec 18, 2017 •

edited

Loading

bronaghs Dec 18, 2017 •

edited

Loading