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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdff0c4
fix: return full status object from the info endpoint
LucasRoesler Feb 26, 2022
591ec14
fix: do not assume container 0 is the function container
LucasRoesler Mar 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 37 additions & 10 deletions pkg/k8s/function_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,19 @@ func AsFunctionStatus(item appsv1.Deployment) *types.FunctionStatus {

labels := item.Spec.Template.Labels
function := types.FunctionStatus{
Name: item.Name,
Replicas: replicas,
Image: functionContainer.Image,
AvailableReplicas: uint64(item.Status.AvailableReplicas),
InvocationCount: 0,
Labels: &labels,
Annotations: &item.Spec.Template.Annotations,
Namespace: item.Namespace,
Secrets: ReadFunctionSecretsSpec(item),
CreatedAt: item.CreationTimestamp.Time,
Name: item.Name,
Replicas: replicas,
Image: functionContainer.Image,
AvailableReplicas: uint64(item.Status.AvailableReplicas),
InvocationCount: 0,
Labels: &labels,
Annotations: &item.Spec.Template.Annotations,
Namespace: item.Namespace,
Secrets: ReadFunctionSecretsSpec(item),
CreatedAt: item.CreationTimestamp.Time,
Constraints: nodeSelectorToConstraint(item),
ReadOnlyRootFilesystem: hasReadOnlyRootFilesystem(item),
EnvVars: map[string]string{},
}

req := &types.FunctionResources{Memory: functionContainer.Resources.Requests.Memory().String(), CPU: functionContainer.Resources.Requests.Cpu().String()}
Expand All @@ -48,8 +51,32 @@ func AsFunctionStatus(item appsv1.Deployment) *types.FunctionStatus {
for _, v := range functionContainer.Env {
if EnvProcessName == v.Name {
function.EnvProcess = v.Value
continue
}
function.EnvVars[v.Name] = v.Value
}

return &function
}

func nodeSelectorToConstraint(deploy appsv1.Deployment) []string {
nodeSelector := deploy.Spec.Template.Spec.NodeSelector
alexellis marked this conversation as resolved.
Show resolved Hide resolved
constraints := []string{}
for k, v := range nodeSelector {
constraints = append(constraints, k+"="+v)
}
return constraints
}

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

if securityContext == nil {
return false
}

if securityContext.ReadOnlyRootFilesystem == nil {
return false
}

return *securityContext.ReadOnlyRootFilesystem
}
79 changes: 78 additions & 1 deletion pkg/k8s/function_status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand All @@ -23,7 +24,10 @@ func Test_AsFunctionStatus(t *testing.T) {
annotations := map[string]string{"data": "datavalue"}
namespace := "func-namespace"
envProcess := "process string here"
envVars := map[string]string{"env1": "env1value", "env2": "env2value"}
secrets := []string{"0-imagepullsecret", "1-genericsecret", "2-genericsecret"}
readOnlyRootFilesystem := false
constraints := []string{"node-label=node-value"}

deploy := appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -39,6 +43,9 @@ func Test_AsFunctionStatus(t *testing.T) {
Labels: labels,
},
Spec: corev1.PodSpec{
NodeSelector: map[string]string{
"node-label": "node-value",
},
ImagePullSecrets: []corev1.LocalObjectReference{
{Name: "0-imagepullsecret"},
},
Expand All @@ -48,7 +55,8 @@ func Test_AsFunctionStatus(t *testing.T) {
Image: image,
Env: []corev1.EnvVar{
{Name: "fprocess", Value: envProcess},
{Name: "customEnv", Value: "customValue"},
{Name: "env1", Value: "env1value"},
{Name: "env2", Value: "env2value"},
},
},
},
Expand Down Expand Up @@ -122,4 +130,73 @@ func Test_AsFunctionStatus(t *testing.T) {
t.Errorf("incorrect Secrets: expected %v, got : %v", secrets, status.Secrets)
}

if !reflect.DeepEqual(status.EnvVars, envVars) {
t.Errorf("incorrect EnvVars: expected %+v, got %+v", envVars, status.EnvVars)
}

if !reflect.DeepEqual(status.Constraints, constraints) {
t.Errorf("incorrect Constraints: expected %+v, got %+v", constraints, status.Constraints)
}

if status.ReadOnlyRootFilesystem != readOnlyRootFilesystem {
t.Errorf("incorrect ReadOnlyRootFilesystem: expected %v, got : %v", readOnlyRootFilesystem, status.ReadOnlyRootFilesystem)
}

t.Run("can parse readonly root filesystem when nil", func(t *testing.T) {
readOnlyRootFilesystem := false
deployment := deploy
deployment.Spec.Template.Spec.Containers[0].SecurityContext = &corev1.SecurityContext{}

status := AsFunctionStatus(deployment)
if status.ReadOnlyRootFilesystem != readOnlyRootFilesystem {
t.Errorf("incorrect ReadOnlyRootFilesystem: expected %v, got : %v", readOnlyRootFilesystem, status.ReadOnlyRootFilesystem)
}
})

t.Run("can parse readonly root filesystem enabled", func(t *testing.T) {
readOnlyRootFilesystem := true
deployment := deploy
deployment.Spec.Template.Spec.Containers[0].SecurityContext = &corev1.SecurityContext{
ReadOnlyRootFilesystem: &readOnlyRootFilesystem,
}

status := AsFunctionStatus(deployment)
if status.ReadOnlyRootFilesystem != readOnlyRootFilesystem {
t.Errorf("incorrect ReadOnlyRootFilesystem: expected %v, got : %v", readOnlyRootFilesystem, status.ReadOnlyRootFilesystem)
}
})

t.Run("returns non-empty resource requests", func(t *testing.T) {
deployment := deploy
deployment.Spec.Template.Spec.Containers[0].Resources.Requests = corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("100m"),
corev1.ResourceMemory: resource.MustParse("100Mi"),
}

status := AsFunctionStatus(deployment)
if status.Requests.CPU != "100m" {
t.Errorf("incorrect Requests.CPU: expected %s, got %s", "100m", status.Requests.CPU)
}

if status.Requests.Memory != "100Mi" {
t.Errorf("incorrect Requests.Memory: expected %s, got %s", "100Mi", status.Requests.Memory)
}
})

t.Run("returns non-empty resource limits", func(t *testing.T) {
deployment := deploy
deployment.Spec.Template.Spec.Containers[0].Resources.Limits = corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("100m"),
corev1.ResourceMemory: resource.MustParse("100Mi"),
}

status := AsFunctionStatus(deployment)
if status.Limits.CPU != "100m" {
t.Errorf("incorrect Limits.CPU: expected %s, got %s", "100m", status.Limits.CPU)
}

if status.Limits.Memory != "100Mi" {
t.Errorf("incorrect Limits.Memory: expected %s, got %s", "100Mi", status.Limits.Memory)
}
})
}