[jclouds/jclouds] Removes check for group name when deleting (#1202)

[Daniel Estévez <no...@github.com>]

This currently fails if you use a securityGroup with Upper case letters in your subscription.
Naming check shouldn't be needed in a delete operation.
You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds/pull/1202

-- Commit Summary --

  * Removes check for group name when deleting

-- File Changes --

    M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java (5)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/1202.patch
https://github.com/jclouds/jclouds/pull/1202.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Andrew Gaul <no...@github.com>]

Closed #1202.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#event-3586273351

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Ignasi Barrera <no...@github.com>]

Thanks, @danielestevez. I've just added a comment to your commit.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-389865682

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Andrew Phillips <no...@github.com>]

> I think this change is not safe

Thanks for catching this, @nacx! Are we perhaps missing a unit test somewhere to ensure the cleanup method behaves as you described?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-386777891

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Andrew Gaul <no...@github.com>]

Please reopen against apache/jclouds if this is still relevant.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-663855703

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Ignasi Barrera <no...@github.com>]

@danielestevez any plans ti implement the cleanup improvements in this PR? If not feel free to close it and open a new one.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-400203295

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Ignasi Barrera <no...@github.com>]

Hmmmm I think it is still not the right fix. And I see two different issues here:

1. We shouldn't rely on naming validation to identify something as "created by jclouds".
2. The current validator does not match the Azure naming restrictions, so we'd better change it.

For the second point, we could replace [the default validator](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/internal/FormatSharedNamesAndAppendUniqueStringToThoseWhichRepeat.java#L79) by a more relaxed implementation that better reflects Azure naming restrictions. Once that validator is created we should be able to configure it by binding it in the HTTP API module as:

```java
bind(new TypeLiteral<Validator<String>>() {}).to(CustomNamingValidator.class).in(Scopes.SINGLETON);
```

Once we have a proper naming validator in place for Azure, we should fix how we identify the security groups we automatically created. We could do this in a similar way to what we do with the availability sets: when we create these implicit security groups [here](https://github.com/jclouds/jclouds/blob/master/providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/loaders/CreateSecurityGroupIfNeeded.java#L91), we could add a tag (say, `jclouds-<whatever is good here>`) to *mark* the group as an auto-generated one. Then the cleanup strategy, apart from getting the security group (it won't fail now as we have a proepr validator) will check if it is has the tag, and decide if it has to be deleted or not based on that.

I think this approach is much more robust and we should go for something like this.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-387002426

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Ignasi Barrera <no...@github.com>]

This PR is about fixing the cleanup. As discussed previously that should be better achieved by tagging the implicit security groups jclouds creates. I'm ok at using this PR to implent this, or to close it and open a follow-up one. Up to you @danielestevez! And thanks for all the recent patches!



-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-391516887

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Ignasi Barrera <no...@github.com>]

The DNS one is just a naming convention known to work for most providers. Being restrictive works fine for portability but conflicts when you're trying to use it with pre-existing stuff. In this case I'd say it makes sense to have a custom one for Azure, since it is only used to name things: vms, security groups, networks, etc, and all them should follow the same naming conventions... and if not, we'll find out! :)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-387073926

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Daniel Estévez <no...@github.com>]

This simple validator should work https://github.com/jclouds/jclouds/pull/1213 and we could close this PR

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-391501389

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Daniel Estévez <no...@github.com>]

Ok! I'll take a look at it again. 

When i tried it it was trying to remove a security group already existing in the user subscription, maybe the problem is there


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-386596221

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Ignasi Barrera <no...@github.com>]

I think this change is not safe. The group name the method [gets passed in](https://github.com/jclouds/jclouds/blob/master/providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/AzureComputeService.java#L120) is **not** the name of a security group. It is the name of the *jclouds group* (that "logical" group jclouds configures when calling `computeService.createNodesInGroup`.

That cleanup method is intended just to remove the security groups that are created automatically by jclouds (not directly by the user), when nodes are provisioned without specifying security groups, but using the `templateOptions.inboundPorts`. Those "implicit" securoty groups to open the configured ports are created [here](https://github.com/jclouds/jclouds/blob/master/providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CreateResourcesThenCreateNodes.java#L188-L194). Their name is derived from the "group" configured when creating nodes, and the cleanup method attemps to rebuild that name when deleting all orphaned security groups for that node.

It should only attempt to delete those "implicit" security groups, so if there is a failure there, we should handle it differently, making sure we just try to delete those groups. Makes sense?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-386527745

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Daniel Estévez <no...@github.com>]

About this, i started to implement a custom validation for ARM here extending current interfaces
https://github.com/danielestevez/jclouds/commits/groupNamingValidation
Still not sure about the implementation, and specially naming but there it is if you want to take a look

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-389671765

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Daniel Estévez <no...@github.com>]

For the first point, i supposed it made sense to delete only those "implicit" orphaned groups created by jclouds, but it's true it'd delete groups from the subscription so adding a tag would make this process indeed more robust.

For the validator, i thought there was maybe a reason to use a valid DNS ruleset so i did'nt want to change this as a first fix. If you can confirm there is not such reason i will add a new Azure-compatible validator and run the tests again!


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-387071085

Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)

[Daniel Estévez <no...@github.com>]

Checked again:

The problem happens when trying to security groups from nodes not created by jclouds and therefore, not respecting naming convention (using Upper case breaks [DnsNameValidator](https://github.com/danielestevez/jclouds/blob/9cdd53b0b7a87fa26a77b9ce370882f2a9cc7d71/core/src/main/java/org/jclouds/predicates/validators/DnsNameValidator.java#L55))

Now these security groups already existing in subscription will be ignored and only jclouds "implicit" created groups will be removed, as the original intent was.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1202#issuecomment-386639937