Add missing attributes to /system/function/{functionName}

[ewilde]

Expected Behaviour

Attributes used to create a function with the POST endpoint should be mirrored when retrieving the function meta data using the GET endpoint

Current Behaviour

The attributes are currently as follows:

Name	POST	GET
service	- [x]	- [x] - called name
network	- [x]	- [ ] - missing
image	- [x]	- [x]
envProcess	- [x]	- [x]
envVars	- [x]	- [ ] - missing
constraints	- [x]	- [ ] - missing
labels	- [x]	- [x]
annotations	- [x]	- [x]
secrets	- [x]	- [ ] - missing
registryAuth	- [x]	- [ ] - missing but maybe not include as its sensitive
limits	- [x]	- [ ] - missing
requests	- [x]	- [ ] - missing
readOnlyRootFilesystem	- [x] added	- [ ] - missing

Possible Solution

1. swagger.yml add missing attributes to FunctionListEntry
2. gateway/requests/requests.go add missing attributes to Function type
3. faas-swarm/handlers/reader.go add missing attributes in readServices func
4. faas-netes add missing attributes
5. faas-operator add missing attributes
6. faas-cli describe verb when merged may need updating

Context

I'm writing a terraform provider for openfaas, to work well with terraform, it's best if the underlying infrastructure apis have mirrored POST/GET endpoints. This is also a very common practice in REST apis

[alexellis]

Thanks for adding the context for this.

The service list / endpoint is used for auto-scaling, event connectors and checking readiness. What kind of latency or size increase would we expect for this?

[alexellis]

@openfaas/core please take a look. I'd like your input.

[alexellis]

@ewilde this may also need adding to /system/functions

@martindekov was writing a script to add new labels to existing deployments but could not do so because if any functions had env-vars or secrets then they would be removed by the update.

[martindekov]

I can help out with testing this out later since I am familiar with the problem.

[bartsmykla]

I think that it's a good idea to mirror them, it'll improve coherence of API

[LucasRoesler]

@alexellis if we are worried about the latency/size impact, we could

1. move this to subpaths, /system/functions/spec and /system/functions/{name}/spec. So that /system/functions would just be a summary.
2. Or add a nullable spec field and only include it if a GET parameter is sent.
3. Or add a nullable spec field and a GET parameter hide_spec to explicitly hide it in those cases where you don't want it

I think option 1 is easier to document and maintain

[alexellis]

I like that option 1 is a non-breaking change that achieves what we need for Ed's terraform use-case and for anyone else who wants to make an update to an existing deployment who may not have all the original source data. With OpenFaaS Cloud for example we have all the data i.e. lists of secrets/envs so there it hasn't been an issue yet.

For Ed and for @martindekov who was writing a script over the last few days to migrate labels this would be ideal.

[ewilde]

Happy to go with option 1. However; looks like it would be a breaking change though, if I've understood the proposal correctly

To create a new function resource you would POST to /system/functions/spec and read the resource GET from /system/functions/{name}/spec. Presumably older versions of the cli would still POST to /system/functions and other components would read from /system/functions/{name}?

If we are to go with option 1, I would prefer to GET from /system/functions/spec/{name}, instead of /system/functions/{name}/spec/. Think that more closely aligns with a restful API. You are creating a function spec resource and reading the spec resource with the identifier {name}

[LucasRoesler]

I like this

GET from /system/functions/spec/{name}, instead of /system/functions/{name}/spec/.

[alexellis]

What if we extend the main existing API to return all available data and then add a summary APi if needed for internal components later?

[LucasRoesler]

It is a larger potentially breaking change if we do it that way. But it might be worth it for API consistency

[ameier38]

Hi all, I would love to help pick this back up as I am trying to use the openfaas terraform provider but I cannot get some things in the provider to work without the information described in this issue. See below for my thoughts on how to solve. I am happy to create the PRs but will definitely need some guidance on the right approach.

Goal: Update the gateway to return all the information about a function in order to support terraform and pulumi providers.

Suggested Steps

1. faas: Update swagger.yml and Function in requests.go
2. faas-swarm: Update reader.go
3. faas-netes: Update reader.go
4. openfaas-operator: Update replicas.go and list.go
5. terraform-provider-openfaas: Update structure.go
6. pulumi-openfaas: Update client.go

Searching for where Function is used It doesn't seem like we would need to update faas-cli. Let me know if you think I am missing any other references.

Resources

• Original PR: https://github.com/openfaas/faas/pull/849

[ameier38]

Also related https://github.com/pulumi/pulumi-openfaas (specifically https://github.com/pulumi/pulumi-openfaas/blob/master/pkg/client/client.go#L89)

[LucasRoesler]

@ameier38 the steps you layout look good and if we just extend the current model with the missing fields per https://github.com/openfaas/faas/issues/848#issuecomment-423802184 then it should be straightforward and compatible with the current implementations.

[alexellis]

We will be discussing this issue in our next members call. Will let you know where we get.

[alexellis]

Related work: https://github.com/openfaas/faas/issues/784

^ this may allow for "exporting" of functions.

[alexellis]

Part of the challenge with this issue is that we encode the "missing" data in the container spec, so limits/requests are parsed and then implemented on the spec.

It's possible that we could do a non-lossy conversion back, but it's more likely that to achieve this what we really need to do is to store the JSON that was used to create / update the function.

i.e.

{
  "service": "nodeinfo",
  "image": "functions/nodeinfo:burner",
  "envProcess": "node main.js",
  "labels": {
    "com.openfaas.scale.min": "2",
    "com.openfaas.scale.max": "15"
  },
  "environment": {
    "output": "verbose",
    "debug": "true"
  }
}

The above would be put into an annotation called spec, so that the REST API could return this when requested.

We also need to separate out desired state vs status. I.e. readyReplicas cannot be set by the deployment request, but it is going to be returned on the function endpoint.

How do we want to address that? ^

[ewilde]

@ameier38 we discussed this issue at our members meeting this evening on zoom. @alexellis suggested again the approach in the above comment.

Openfaas-operator is already serialising the full function specification in an annotation see: deployment.go#L183

We could adapt this approach to other providers: faas-netes and faas-swarm. Maybe start with a openfaas-operator?

[alexellis]

We also need to separate out desired state vs status. I.e. readyReplicas cannot be set by the deployment request, but it is going to be returned on the function endpoint.

Thoughts on this?

[ameier38]

@alexellis Could faas.requests.Function be refactored into something like:

type FunctionResponse struct {
    Name            string  `json:"name"`
    Image           string  `json:"image"`
    Replicas        uint64  `json:"replicas"`
    EnvProcess      string  `json:"envProcess"`
    Secrets     []string `json:"secrets"`
    Labels      *map[string]string `json:"labels"`
    Annotations     *map[string]string `json:"annotations"`
    Limits      *FunctionResources `json:"limits"`
    Requests    *FunctionResources `json:"requests"`

    AvailableReplicas uint64 `json:"availableReplicas"`
    InvocationCount   uint64 `json:"invocationCount"` // not sure this is needed
}

Already started in the corresponding PR

As you mentioned we could create an annotation by serializing CreateFunctionRequest. This could then be used to populate FunctionResponse when requesting a function (similar to this) along with AvailableReplicas and InvocationCount

[alexellis]

I think this is also related: https://github.com/openfaas/faas/issues/565

[alexellis]

Do you think all of the data inputted can be retrieved from Kubernetes and Swarm objects, or is using an annotation the only way to make this work reliably?

[LucasRoesler]

I think everything in https://github.com/openfaas/faas/issues/848#issuecomment-503369088 except InvocationCount could come from the k8s/swarm objects

[ameier38]

@LucasRoesler I checked the k8s Deployment type and I think you are right. I also like the idea of just pulling the info from the pod.

Given that, I don't think we would need to update faas.request.Function but rather create a new type in faas-netes/faas-swarm, say FunctionSpec, with Function embeded plus the additional fields. We could then use FunctionSpec here and here instead of Function. After Function is moved to faas-provider we could then re-sync.

Let me know what you think.

[LucasRoesler]

@ameier38 i am not sure about using an embedding, but passing around FunctionSpec sounds like a good idea to me

[alexellis]

I would like to see this approached iteratively - we can start with fixing envProcess whilst we figure out the game-plan for the bigger picture (not trivial by the looks of it)

[alexellis]

@ameier38 agreed to put up a PR to fix faas-netes reading the envProcess. Let us know when that's ready for review.

[alexellis]

https://github.com/openfaas/faas/issues/848#issuecomment-508377961

@LucasRoesler is the above done now? I think I merged a PR from you for it?

[alexellis]

That part of the issue appears to have been addressed in: https://github.com/openfaas/faas-netes/pull/549

@martindekov I heard you had an interest in this issue, do you want to go about making some changes and PRs? What's your availability like?

[alexellis]

@ameier38 please look at the new types that were spun out into the faas-provider project. https://github.com/openfaas/faas-provider/tree/master/types

[martindekov]

I can do some support work or small PRs by the middle of next month. Still finishing two school projects and it is the time of the year where the exams are incoming.

[martindekov]

As far as I remember when you create a function you could have done that with less information, less configuration. Once the function was deployed additional fields were added by Kubernetes/Swarm I will need to sit down and check whether the function definition on deployment(POST JSON) and on listing(GET JSON) match. It would be ideal for the GET request to return exactly what is needed for a POST request.

I see there have been quite some advancements with this and I am not sure if my concern is still relevant, so I might be talking nonsense ATM, just a note on the comment.

[LucasRoesler]

#848 (comment)

@LucasRoesler is the above done now? I think I merged a PR from you for it?

Yes, I am remember doing that

[ameier38]

@alexellis Sorry I haven't had the time to look at this lately. I hope to free up in the next few weeks and I can take a look at this again. The new types look great and should make it easy to update this endpoint (and ultimately update the terraform and pulumi providers)

[alexellis]

Let's move this forward for faasd and faas-netes. Cc @Waterdrips @LucasRoesler