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

New Resource: azurerm_bastion_host #4096

Merged
merged 24 commits into from
Oct 10, 2019
Merged

New Resource: azurerm_bastion_host #4096

merged 24 commits into from
Oct 10, 2019

Conversation

DanielMabbett
Copy link
Contributor

This should fix the feature request issue:
#3829

I have:

  • Created the azurerm_bastion_host resource.
  • Added a basic test that creates the bastion host.
  • Added a Markdown file for the README content on the Terraform website.

@DanielMabbett DanielMabbett changed the title Feature/azure bastion host New Resource: azure bastion host Aug 20, 2019
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hi @DanielMabbett,

Thank you for this PR. Overall this is off to a great start but there are some things that need to be addressed. I've left some comments inline but a short summary of them is:

  • This will need master merged in or rebased fo fix the merge conflicts
  • on create/read we should use expand/flatten functions for sub blocks
  • we need to read back in all properties after creation
  • we should add a complete test that uses all properties (dns_name)

azurerm/resource_arm_bastion_host.go Outdated Show resolved Hide resolved
azurerm/resource_arm_bastion_host.go Show resolved Hide resolved
azurerm/resource_arm_bastion_host.go Show resolved Hide resolved
azurerm/resource_arm_bastion_host.go Show resolved Hide resolved
azurerm/resource_arm_bastion_host.go Show resolved Hide resolved
azurerm/resource_arm_bastion_host_test.go Outdated Show resolved Hide resolved
website/docs/r/bastion_host.html.markdown Outdated Show resolved Hide resolved
website/docs/r/bastion_host.html.markdown Outdated Show resolved Hide resolved
website/docs/r/bastion_host.html.markdown Outdated Show resolved Hide resolved
website/docs/r/bastion_host.html.markdown Outdated Show resolved Hide resolved
@DanielMabbett
Copy link
Contributor Author

Just for everyones info: This will require the network client to use the 2019-06-01 api rather than the existing 2018-12-01 version. This will have to be done in a separate PR as this will affect other network resources deployed in Azure Terraform provider.

@ghost ghost removed the waiting-response label Oct 4, 2019
@katbyte
Copy link
Collaborator

katbyte commented Oct 5, 2019

@DanielMabbett, it looks like we've already updated the network package in 1.34: #4291

@DanielMabbett
Copy link
Contributor Author

Ah perfect. I will update the code to reflect the API shortly

@ghost ghost added size/XXL and removed size/XL labels Oct 7, 2019
@DanielMabbett DanielMabbett requested a review from katbyte October 7, 2019 15:35
Copy link
Collaborator

@katbyte katbyte 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 revisions @DanielMabbett,

Aside from a couple minor comments inline this is looking good! However when i run the tests on our CI server i'm getting:

=== RUN   TestAccAzureRMBastionHost_basic
=== PAUSE TestAccAzureRMBastionHost_basic
=== CONT  TestAccAzureRMBastionHost_basic
--- FAIL: TestAccAzureRMBastionHost_basic (252.87s)
    testing.go:569: Step 0 error: errors during apply:
        
        Error: Error waiting for creation/update of Bastion Host "acctestBastion2vwe" (Resource Group "acctestRG-191007203045052833"): Code="VmssGatewayDeploymentFailed" Message="The gateway deployment operation failed due to an intermittent error. Please try again." Details=[]
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test821609061/main.tf line 29:
          (source code not available)
        
        
    testing.go:630: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during apply: 2 problems:
        
        - Error deleting Subnet "AzureBastionSubnet" (Virtual Network "acctestVNet2vwe" / Resource Group "acctestRG-191007203045052833"): network.SubnetsClient#Delete: Failure sending request: StatusCode=400 -- Original Error: Code="InUseSubnetCannotBeDeleted" Message="Subnet AzureBastionSubnet is in use by /subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-191007203045052833/providers/Microsoft.Network/bastionHosts/acctestBastion2vwe/bastionHostIpConfigurations/configuration and cannot be deleted. In order to delete the subnet, delete all the resources within the subnet. See aka.ms/deletesubnet." Details=[]
        - Error deleting Public IP "acctestBastionPIP191007203045052833" (Resource Group "acctestRG-191007203045052833"): network.PublicIPAddressesClient#Delete: Failure sending request: StatusCode=400 -- Original Error: Code="PublicIPAddressCannotBeDeleted" Message="Public IP address /subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-191007203045052833/providers/Microsoft.Network/publicIPAddresses/acctestBastionPIP191007203045052833 can not be deleted since it is still allocated to resource /subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-191007203045052833/providers/Microsoft.Network/bastionHosts/acctestBastion2vwe/bastionHostIpConfigurations/configuration. In order to delete the public IP, disassociate/detach the Public IP address from the resource.  To learn how to do this, see aka.ms/deletepublicip." Details=[]

azurerm/resource_arm_bastion_host.go Outdated Show resolved Hide resolved
azurerm/resource_arm_bastion_host.go Outdated Show resolved Hide resolved
azurerm/resource_arm_bastion_host_test.go Show resolved Hide resolved
azurerm/resource_arm_bastion_host_test.go Show resolved Hide resolved
azurerm/resource_arm_bastion_host_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_bastion_host_test.go Outdated Show resolved Hide resolved
@ghost ghost added size/XL and removed size/XXL labels Oct 8, 2019
@katbyte katbyte added this to the 1.37 milestone Oct 8, 2019
@katbyte katbyte changed the title New Resource: azure bastion host New Resource: azurerm_bastion_host Oct 10, 2019
Copy link
Collaborator

@katbyte katbyte 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 fixing that @DanielMabbett, LGTM and the tests now pass! however linting is failing because of an extra whitespace. you can fix it by running make whitespace and committing the change (i tried myself but i don't have push access to the branch)

@katbyte katbyte modified the milestones: v1.37.0, v1.36.0 Oct 10, 2019
@DanielMabbett DanielMabbett requested a review from katbyte October 10, 2019 19:19
@katbyte katbyte merged commit 93127c7 into hashicorp:master Oct 10, 2019
katbyte added a commit that referenced this pull request Oct 10, 2019
@ghost
Copy link

ghost commented Oct 29, 2019

This has been released in version 1.36.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.36.0"
}
# ... other configuration ...

@ghost ghost removed the waiting-response label Oct 29, 2019
@JamesDLD
Copy link

Hello I just did use the new resource "azurerm_bastion_host" and do thank you for the release but I faced this error, seems that some characters like "-" are prohibited, hardcoded in the resource itself. I can attest because I used to create the bastion host with arm template and creating those resource with this character is supported by Microsoft.

Error: lowercase letters, highercase letters numbers only are allowed in "name": "product-perim-npd-vnet2-bas1"

@DanielMabbett
Copy link
Contributor Author

Hi @JamesDLD , Thanks for letting us know. I will take a look at this. Can you put your comment above into a new request and I will get working on fixing that for you.

@ghost
Copy link

ghost commented Nov 10, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants