Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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

Open
LucasRoesler wants to merge 2 commits into openfaas:master
base: master
Choose a base branch
Loading
from LucasRoesler:fix-922-return-full-status-object-from-info-endpoint

Conversation

@LucasRoesler
Copy link
Member

@LucasRoesler LucasRoesler commented Feb 26, 2022
edited
Loading

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月11日T14: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月11日T14: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 <roesler.lucas@gmail.com>
}

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

@alexellis alexellis Mar 23, 2022

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

@alexellis alexellis Mar 23, 2022

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

@alexellis alexellis Mar 23, 2022

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

@LucasRoesler LucasRoesler Mar 24, 2022

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

@LucasRoesler LucasRoesler Mar 24, 2022

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

@LucasRoesler LucasRoesler Mar 24, 2022

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.

alexellis reacted with thumbs up emoji
Copy link
Member

@alexellis alexellis Jun 9, 2022

Choose a reason for hiding this comment

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

Thank you

@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 <roesler.lucas@gmail.com>
@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
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??

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

Reviewers

@alexellis alexellis Awaiting requested review from alexellis

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: function info endpoint does not return the env variables

AltStyle によって変換されたページ (->オリジナル) /