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

[resource_arm_function_app] Adding function keys to output #4066

Closed

Conversation

lawrencegripper
Copy link
Contributor

@lawrencegripper lawrencegripper commented Aug 12, 2019

This aims to resolve #699 by adding a computed output onto the functions resource which contains the keys for each function and the trigger_url for calling the function.

Would be great to get thoughts on this early to so I don't waist time if this isn't an approach that can be merged.

Other considered approaches

I considered was adding a generic resource_arm_list_keys resource so it could be used on any resource and have a more dynamic output but I figured this is more complex and likely a tougher sell to get merged.

Another option I looked at was having this added functionality as a data_source_arm_function_keys but this seemed at odds with how other resources like storage_account do things so didn't want to make it confusing.

Todo

  • Test timing... the keys and functions are only available to be listed once the functions runtime has started up. I have a fear that I'll need to do some polling to wait for this to happen... I'm not sure how I can differentiate between an intentionally empty function app and one that is just starting up.
  • Devise a meaningful integration test (requires deploying some sample code so functions are present)
  • Test locally (currently coded but not ran)

@ghost ghost added the size/M label Aug 12, 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.

Thank you for the PR @lawrencegripper,

In addition to the comments i've left inline, could we update the docs with the new property?

azurerm/resource_arm_function_app.go Outdated Show resolved Hide resolved
azurerm/resource_arm_function_app.go Outdated Show resolved Hide resolved
@katbyte katbyte added this to the v1.33.0 milestone Aug 13, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.33.0, v1.34.0 Aug 21, 2019
@lawrencegripper
Copy link
Contributor Author

@katbyte Thanks for taking a look. Things got busy my end so hoping to get back around to fixing up the feedback in the next few weeks - sorry for the delay!

@lawrencegripper
Copy link
Contributor Author

lawrencegripper commented Sep 4, 2019

@katbyte and @tombuildsstuff I'm a little worried about continuing to add this into the main functions resource due to some issues I think we'll see with timing.

My proposal is to move this code out of the resource into a data type of azurerm_function_key like so

data "azurerm_functions_key" "test" {
  name                = "function_app_name_here"
  resource_group_name = "resource_group_here"
  function_name = "name_of_function_you_want_key_for_here"
}

This way the data type is explicit about which function key you want and can block until the function is found (within reason) and this change doesn't slow current users of the resource down.

Details

For the code to be able to retrieve the keys for the functions the functionsRuntime must have spun up the functions. Each function app can host multiple functions which are pushed up as a ZIP with URL to it included in the Environment vars.

This happens an indeterminate amount of time after the resource is published based on my experience so far. I looks like the CLI tooling uses a sync triggers API to ensure the publish step has finished.

https://github.com/Azure/azure-functions-core-tools/blob/9cc58db73732bf17364c151be822036f52c78cd4/src/Azure.Functions.Cli/Helpers/AzureHelper.cs#L273

Alongside this we have to differentiate between a deployment which contains functions and a deployments which is creating an empty functions resource. I won't want to wait for functions to appear that never will as the user has created the resource without any functions in it.

@ghost ghost removed the waiting-response label Sep 4, 2019
@ghost ghost added size/L and removed size/M labels Sep 6, 2019
@lawrencegripper
Copy link
Contributor Author

I've started work on the data_source version and think it's the right route to go - unless anyone objects.

Unfortunately it looks like the SwaggerDefinition/SDK for the endpoints needed is incorrect, trying to work around it now. SDK issue: Azure/azure-rest-api-specs#7143

@tombuildsstuff tombuildsstuff removed this from the v1.34.0 milestone Sep 13, 2019
@tombuildsstuff tombuildsstuff added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Sep 13, 2019
@lawrencegripper
Copy link
Contributor Author

Tested this out with the PR build of the SDK and it does what is needed. Going to sit tight and wait for the change to get merged then this can go in, unless any other feedback?

I'm happy with the datasource approach it's nice and clean imo.

Will finish off docs tomorrow so it's ready to go once SDK change is in.

@ghost ghost added the documentation label Sep 16, 2019
@lawrencegripper
Copy link
Contributor Author

Updated the docs and tweaks some naming to hopefully make things more usable. Hopefully now just needs SDK update and it's ready to go.

@lawrencegripper
Copy link
Contributor Author

Quick update. Now waiting on this REST Spec change to be merged and then updated GoSDK to be merged Azure/azure-rest-api-specs#7174

@lawrencegripper
Copy link
Contributor Author

lawrencegripper commented Oct 18, 2019

@katbyte and @tombuildsstuff The upstream SDK update is now merged so I'm hoping to be able to start work on finishing this off.

Azure/azure-sdk-for-go#6078

Is there a protocol on how to update the Azure Go SDK or do I do an update in my PR branch through Go mod and push here?

@katbyte
Copy link
Collaborator

katbyte commented Oct 18, 2019

That's great news @lawrencegripper, basically once its been released to a named version you can update go.mod and run go mod vendor in a new branch/PR and once that is merged to master here rebase/merge master on this one.

@lawrencegripper
Copy link
Contributor Author

@katbyte So I tried to give this one a go, it looks like the SDK has removed all the devspaces apis used by resource_arm_devspace_controller in release v34

https://github.com/Azure/azure-sdk-for-go/releases/tag/v34.0.0

Whats the way forward here do I create a PR updating the SDK and removing the devspaces resources as they're no longer in the SDK? Seems a bit brutal.

@ghost ghost removed the waiting-response label Oct 18, 2019
@lawrencegripper
Copy link
Contributor Author

Turns out Tom already did all that hard work for me and I was a bit behind on master do did it all again before I realized: #4609

@tombuildsstuff
Copy link
Contributor

Dependent on #4775 (since there's vendor changes)

@jackbatzner
Copy link
Contributor

Hi @lawrencegripper , #4775 has been merged. Do you have any time to work on this?

I'd love to make some changes related to this work. I don't want to make similar changes to what your doing and potentially cause rebase/merge issues 😄 .

@ghost ghost removed the waiting-response label Nov 26, 2019
@lawrencegripper
Copy link
Contributor Author

@Brunhil I merged the upstream changes but the bug in the spec remains meaning the GoSDK is still incorrect and cannot be used to list the functions keys.

Details: Azure/azure-rest-api-specs#7174 (comment)

Given the team have been unable to help over a number of months my proposal is to close this PR.

@ghost
Copy link

ghost commented Mar 29, 2020

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 Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies documentation enhancement service/functions Function Apps size/L upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: export Functions host keys
4 participants