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

fix: return full status object from the info endpoint #923

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LucasRoesler
Copy link
Member

@LucasRoesler LucasRoesler commented Feb 26, 2022

Description

Ensure that all fields of the FunctionStatus are populated based on
the function Deployment. In particular, this change ensures that the
env variables, constrains, and the read-only root filesystem values are
populated correctly.

Found while updating the faas-cli, see openfaas/faas-cli#915

Motivation and Context

  • I have raised an issue to propose this change (required)

Resolves #922

How Has This Been Tested?

The extended unit tests fully cover the new code.

As stated in the PR title, this adds missing fields to the function info endpoint: /system/function/{function_name}

Without this change, the endpoint misses values like readOnlyFilesystem

$ curl -s http://localhost:8080/system/function/echo | jq
{
  "name": "echo",
  "image": "ghcr.io/lucasroesler/echo:latest",
  "namespace": "openfaas-fn",
  "labels": {
    "faas_function": "echo",
    "uid": "351420882"
  },
  "annotations": {
    "foo": "bar",
    "prometheus.io.scrape": "false"
  },
  "replicas": 1,
  "availableReplicas": 1,
  "createdAt": "2022-06-11T14:40:28Z"
}

With this change

$ curl -s http://localhost:8080/system/function/echo | jq
{
  "name": "echo",
  "image": "ghcr.io/lucasroesler/echo:latest",
  "namespace": "openfaas-fn",
  "labels": {
    "faas_function": "echo",
    "uid": "351420882"
  },
  "annotations": {
    "foo": "bar",
    "prometheus.io.scrape": "false"
  },
  "readOnlyRootFilesystem": true,
  "replicas": 1,
  "availableReplicas": 1,
  "createdAt": "2022-06-11T14:40:28Z"
}

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Ensure that all fields of the `FunctionStatus` are populated based on
the function `Deployment`. In particular, this change ensures that the
env variables, constrains, and the read-only root filesystem values are
populuated correctly.

Signed-off-by: Lucas Roesler <[email protected]>
}

func hasReadOnlyRootFilesystem(function appsv1.Deployment) bool {
securityContext := function.Spec.Template.Spec.Containers[0].SecurityContext
Copy link
Member

Choose a reason for hiding this comment

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

Is there a chance that the Istio sidecar could take position 0?

Copy link
Member

Choose a reason for hiding this comment

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

Could it be indexed upon some kind of property or name in a range loop?

Copy link
Member

Choose a reason for hiding this comment

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

Could SecurityContext ever be nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it can be nil, but this is checked immediately on the next line.

Copy link
Member Author

Choose a reason for hiding this comment

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

regarding the position of the container:

Both the controller and the handler put the labels on the Pod, but not the container. We can use the container Name though, it will equal the deployment.Name.

As a side note, it would be really nice if we could refactor the an unify the generation of the Deployment, the function Factory doesn't actually generate it, both the controller and the handler have different code for the construction of the Deployment

Copy link
Member Author

Choose a reason for hiding this comment

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

@alexellis i have updated the code and replaced all instances of Containers[0] with a proper FunctionContainer() implementation that finds the correct index and container spec.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you

@LucasRoesler LucasRoesler requested a review from alexellis March 24, 2022 20:16
@LucasRoesler LucasRoesler force-pushed the fix-922-return-full-status-object-from-info-endpoint branch from aafc518 to 5795ba5 Compare March 24, 2022 20:19
Add a utility method for correctly finding the index of the function
Container inside a Deployment spec.

Signed-off-by: Lucas Roesler <[email protected]>
@LucasRoesler LucasRoesler force-pushed the fix-922-return-full-status-object-from-info-endpoint branch from 5795ba5 to 591ec14 Compare March 25, 2022 10:39
@alexellis
Copy link
Member

Can you remind me which endpoint this is for by showing an example of before/after with curl under How Has This Been Tested??

@LucasRoesler
Copy link
Member Author

@alexellis I have updated the PR description with the curl examples. As stated in the PR title this impacts the function info endpoint: /system/function/{function_name}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: function info endpoint does not return the env variables
2 participants