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

Verify deploy validation for missing secrets #49

Conversation

LucasRoesler
Copy link
Member

What

  • Check that function deployment is rejected when it requires a function
    that is not available

Resolves #26

**What**
- Check that function deployment is rejected when it requires a function
  that is not available

Resolves openfaas#26

Signed-off-by: Lucas Roesler <[email protected]>
@LucasRoesler LucasRoesler requested a review from alexellis April 10, 2020 13:43
@@ -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

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.

Add test that shows function is rejected when secret is missing
2 participants