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

Verify deploy validation for missing secrets #49

Closed
wants to merge 1 commit into from
Closed

Verify deploy validation for missing secrets #49

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions tests/deploy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,22 @@ func Test_Deploy_WithAnnotations(t *testing.T) {
}
}

func Test_Deploy_RejectsFunctionWithMissingRequiredSecret(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

FYI this won't work with the Operator which is declarative, it does no validation at deployment time of related secrets.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that the behavior we want to allow for certification? If so, then we can just remove this. I don't think we should add a feature flag for every possible behavior but instead decide what the core behavior is and cover that.

Copy link
Member

Choose a reason for hiding this comment

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

To be fair, we could check the info endpoint first and only execute this against faas-netes, swarm and potentially faasd. The info endpoint gives back a name of the provider.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought is that the certifier should define the baseline integration tests required to say something is an OpenFaaS provider. I don't think it is good for us to add several different switches for each endpoint to handle the differences between swarm/netes/operator/containerd/rancher/etc

In this case, I think we say that this is not specified behavior and leave it to the provider to have its own tests for this kind of behavior. Adding this kind of validation unit test to swarm/netes is straightforward and easily fits within their existing test suites.

In general, we should not treat certifier as an e2e test for the entire project, we will end up with too many flags and code branches to cover everything. Or we say this is the e2e for the project and this means the Operator is out of spec and has a bug.

Copy link
Member

Choose a reason for hiding this comment

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

We will need some divergence for faasd where scaling is capped at 1/1. I think this test is useful to include, but should be ignored for the operator. Can you think of or suggest any ways that might be done and not fall into the pitfall you mentioned?

we will end up with too many flags and code branches to cover everything

functionRequest := types.FunctionDeployment{
Image: "functions/alpine:latest",
Service: "test-secret-crud",
Network: "func_functions",
EnvProcess: "cat /var/openfaas/secrets/secret-name",
Secrets: []string{"does-not-exist-secret"},
}

gwURL := gatewayUrl(t, "/system/functions", "")
_, res := request(t, gwURL, http.MethodPost, makeReader(functionRequest))
if res.StatusCode != http.StatusBadRequest {
t.Errorf("got %d, wanted %d", res.StatusCode, http.StatusBadRequest)
}
}

func strMapEqual(mapName string, got map[string]string, wanted map[string]string) error {
// Can't assert length is equal as some providers i.e. faas-swarm add their own labels during
// deployment like 'com.openfaas.function' and 'function'
Expand Down