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

Test faasd with the certifier #60

Open
alexellis opened this issue Mar 11, 2021 · 20 comments
Open

Test faasd with the certifier #60

alexellis opened this issue Mar 11, 2021 · 20 comments

Comments

@alexellis
Copy link
Member

Description

Test faasd with the certifier

Details

Some tests won't work such as scaling above 1 replica, however scaling from zero will work.

Whoever takes up the task will need to recommend a way of selectively running certain tests, or selectively not running certain tests.

Context

This work will help ensure faasd has as much parity as possible with faas-netes (OpenFaaS on Kubernetes)

A GitHub Action should work here, like it did with Swarm in the past.

@kadern0
Copy link

kadern0 commented Mar 22, 2021

I've been looking into this one. Most of the tests pass, the "scaling" ones fail (I guess these should be skipped), but the test-logger function fails with:

 === RUN   Test_FunctionLogs
    Test_FunctionLogs: logs_test.go:33: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/test-logger
    Test_FunctionLogs: logs_test.go:59: got unexpected log message "Version: 0.18.8\tSHA: 76e463a7a017f81ed88ae7824d2ef7a3f60ed45e", expected "Forking fprocess" or "Wrote 132 Bytes"
Error removing existing function: Delete "http://127.0.0.1:8080/system/functions": context deadline exceeded (Client.Timeout exceeded while awaiting headers), gateway=http://127.0.0.1:8080, functionName=test-logger
    Test_FunctionLogs: panic.go:617: Delete "http://127.0.0.1:8080/system/functions": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
--- FAIL: Test_FunctionLogs (6.35s)

So the function gets deployed but the call output is the same as the stronghash while the test expects: "Forking fprocess" or "Wrote 132 Bytes". Any idea what could be the issue here, @alexellis? Should this test also be skipped?

@alexellis
Copy link
Member Author

alexellis commented Mar 24, 2021

@LucasRoesler how do have the tests run against faasd, knowing that scale to N will fail?

Could you give a few suggestions on?

  • Hardcoded and simplistic
  • Practical, extensible
  • Fancy

@kadern0
Copy link

kadern0 commented Mar 24, 2021

I guess this answer was for me instead of Lucas (: As I mentioned in my comment, the scaling tests fail so I'm ignoring them for now. I am running the tests in the same way as the Makefile although adding some regexp magic (regexp are hardcoded, practical, extensible and fancy):

go test ./tests -v -gateway=${OPENFAAS_URL} -enableAuth -run '^.+_[DHPI].+'
=== RUN   Test_Deploy_Stronghash
--- PASS: Test_Deploy_Stronghash (3.19s)
=== RUN   Test_Deploy_PassingCustomEnvVars_AndQueryString
=== RUN   Test_Deploy_PassingCustomEnvVars_AndQueryString/Empty_QueryString
    Test_Deploy_PassingCustomEnvVars_AndQueryString/Empty_QueryString: deploy_test.go:52: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/env-test
=== RUN   Test_Deploy_PassingCustomEnvVars_AndQueryString/Populated_QueryString
    Test_Deploy_PassingCustomEnvVars_AndQueryString/Populated_QueryString: deploy_test.go:60: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/env-test?testing=1
--- PASS: Test_Deploy_PassingCustomEnvVars_AndQueryString (1.46s)
    --- PASS: Test_Deploy_PassingCustomEnvVars_AndQueryString/Empty_QueryString (0.05s)
    --- PASS: Test_Deploy_PassingCustomEnvVars_AndQueryString/Populated_QueryString (0.02s)
=== RUN   Test_Deploy_WithLabels
    Test_Deploy_WithLabels: deploy_test.go:89: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/env-test-labels
--- PASS: Test_Deploy_WithLabels (1.30s)
=== RUN   Test_Deploy_WithAnnotations
    Test_Deploy_WithAnnotations: deploy_test.go:117: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/env-test-annotations
--- PASS: Test_Deploy_WithAnnotations (1.54s)
=== RUN   Test_HealthEndpoint
--- PASS: Test_HealthEndpoint (0.00s)
=== RUN   Test_ProviderInfo
--- PASS: Test_ProviderInfo (0.00s)
=== RUN   Test_InvokeNotFound
    Test_InvokeNotFound: invoke_test.go:14: [1/30] Got correct response: 404 - http://127.0.0.1:8080/function/notfound
--- PASS: Test_InvokeNotFound (0.01s)
=== RUN   Test_Invoke_With_Supported_Verbs
=== RUN   Test_Invoke_With_Supported_Verbs/GET
    Test_Invoke_With_Supported_Verbs/GET: invoke_test.go:49: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/env-test-verbs
=== RUN   Test_Invoke_With_Supported_Verbs/POST
    Test_Invoke_With_Supported_Verbs/POST: invoke_test.go:49: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/env-test-verbs
=== RUN   Test_Invoke_With_Supported_Verbs/PUT
    Test_Invoke_With_Supported_Verbs/PUT: invoke_test.go:49: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/env-test-verbs
=== RUN   Test_Invoke_With_Supported_Verbs/PATCH
    Test_Invoke_With_Supported_Verbs/PATCH: invoke_test.go:49: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/env-test-verbs
=== RUN   Test_Invoke_With_Supported_Verbs/DELETE
    Test_Invoke_With_Supported_Verbs/DELETE: invoke_test.go:49: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/env-test-verbs
--- PASS: Test_Invoke_With_Supported_Verbs (1.19s)
    --- PASS: Test_Invoke_With_Supported_Verbs/GET (0.01s)
    --- PASS: Test_Invoke_With_Supported_Verbs/POST (0.00s)
    --- PASS: Test_Invoke_With_Supported_Verbs/PUT (0.00s)
    --- PASS: Test_Invoke_With_Supported_Verbs/PATCH (0.00s)
    --- PASS: Test_Invoke_With_Supported_Verbs/DELETE (0.01s)
=== RUN   Test_InvokePropogatesRedirectToTheCaller
    Test_InvokePropogatesRedirectToTheCaller: invoke_test.go:85: [1/30] Got correct response: 302 - http://127.0.0.1:8080/function/redirector-test
--- PASS: Test_InvokePropogatesRedirectToTheCaller (1.43s)
PASS
ok  	github.com/openfaas/certifier/tests	10.128s

This regexp will ignore all scaling tests, Test_FunctionLogs and Test_SecretCRUD. This last one fails when trying to delete the function although it does create and update the secret as expected:

=== RUN   Test_SecretCRUD
    Test_SecretCRUD: secretCRUD_test.go:23: Got correct response for creating secret: 200
    Test_SecretCRUD: secretCRUD_test.go:38: Got correct response for deploying function: 200
    Test_SecretCRUD: secretCRUD_test.go:41: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/test-secret-crud
    Test_SecretCRUD: secretCRUD_test.go:63: Got correct response for updating secret: 200
    Test_SecretCRUD: secretCRUD_test.go:66: [1/30] Got correct response: 200 - http://127.0.0.1:8080/function/test-secret-crud
    Test_SecretCRUD: secretCRUD_test.go:68: got this-is-the-edited-secret-value, wanted this-is-the-edited-secret-value
Error removing existing function: Delete "http://127.0.0.1:8080/system/functions": context deadline exceeded (Client.Timeout exceeded while awaiting headers), gateway=http://127.0.0.1:8080, functionName=test-secret-crud
    Test_SecretCRUD: secretCRUD_test.go:77: Delete "http://127.0.0.1:8080/system/functions": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
--- FAIL: Test_SecretCRUD (7.94s)

@LucasRoesler
Copy link
Member

adding some regexp magic (regexp are hardcoded, practical, extensible and fancy):

@kadern0 i like your style 😄 , although i worry about the long term stability of a regex for this.

  1. But, @kadern0 does provide a great example for hard coded, for each provider we can create a regexp for the -run flag. The next iteration of this is to add a new flag for each provider -isFaasd|-isFaasNetes|.... This is done in the same way that -enableAuth was added. Then in each test we add a sequence of checks for those flags to decide if it should run or not. something like

    if (isFaasD || isOtherNaughtyProvier) {
      t.Skip("this provider does not support: <feature X>")
    }
  2. Something that would be a little more extensible is to add a set of "feature flags", so instead of -isFaasd we would have -scaling, -secrets, -logs, etc. The set of flags should only be the set of features we are willing to call optional. Once these flags are implemented we add similar checks to the start of certain tests

    if (<feature>Enabled) {
        t.Skip("<feature X> is disabled")
    }

    I think this is a bit more clear about what we allowing to be optional and what is not optional. And if we have a future provider, it doesn't need to try to hack into the certifier to add its own flag, it can select from the already implemented flags.
    Since we already have a flag to use as an example, it should be moderately straight forward to add more.

  3. The fancy version of (2) would be to define these feature flags like (2) but then add an endpoint to the provider spec and let the tests query that endpoint and get a the values for the feature flags from the provider itself. The test implementations ultimately look the same as (2). This is definitely the most flexible because the provider can self update, for example, faasd implements scaling in v5.0.0, the tests would automatically start verifying it as soon as the provider indicates that it should work. You wouldn't need to modify how the certifier is run at all. This can be good if someone is using the certifier as a kind of e2e test, the user of the certifier doesn't need to know how to run it based on the provider, the instructions are always the same go test ./... -enableAuth=true|false -gateway=${OPENFAAS_URL}

@LucasRoesler
Copy link
Member

@alexellis and @kadern0 i just want to point out that a mix of option (1) and (2) are already implemented

var (
config = Config{}
defaultNamespace = ""
swarm = flag.Bool("swarm", false, "helper flag to run only swarm-compatible tests only")
token = flag.String("token", "", "authentication Bearer token override, enables auth automatically")
)
func init() {
flag.StringVar(&config.Gateway, "gateway", "", "set the gateway URL, if empty use the gateway_url env variable")
flag.BoolVar(
&config.AuthEnabled,
"enableAuth",
false,
fmt.Sprintf("enable/disable authentication. The auth will be parsed from the default config in %s", filepath.Join(sdkConfig.DefaultDir, sdkConfig.DefaultFile)),
)
flag.BoolVar(&config.SecretUpdate, "secretUpdate", true, "enable/disable secret update tests")
flag.BoolVar(&config.ScaleToZero, "scaleToZero", true, "enable/disable scale from zero tests")
}

There is an existing flag for -swarm (ie option one) . Because swarm is archived, we should remove this.

There are also two flags -secretUpdate and -scaleToZero (ie option two) that control specific tests.

@nitishkumar71
Copy link
Member

Option 3 really looks great to me. @LucasRoesler @kadern0 did you guys have started working on it?
If no then I can work on it.

@LucasRoesler
Copy link
Member

@nitishkumar71 i haven't started yet, but i think the first step is to update the faasd CI workflow to run the certifier as is, without any special flags, just to get it setup and running and to see what fails (if anything). Then we can decide if we want to add flags or "fix" faasd :)

I haven't started anything yet, but, let me know if you want to extend the existing workflow https://github.com/openfaas/faasd/blob/master/.github/workflows/build.yaml

@nitishkumar71
Copy link
Member

Sure. I can work on it. As of now the approach would be to clone the repo and then test it. Any other approach you think we should consider?

@LucasRoesler
Copy link
Member

I swear I submitted a follow up comment the other day. The internet gremlins must have eaten it.


The first step is definitely to clone and run the tests manually. For the automation, basically the same, like you said "clone then run the tests"

@nitishkumar71
Copy link
Member

No worries will start working on it.

@nitishkumar71
Copy link
Member

So I just made some changes in a branch for certifer and faasd. These are not prepared for merge, I have just made the changes required for the test.

The result can be looked here

@LucasRoesler
Copy link
Member

It looks like it is failing due to auth

unable to List function in namspace: unauthorized access, run "faas-cli login" to setup authentication for this server

@nitishkumar71
Copy link
Member

Sorry about that, missed it somehow.

Here is the one after successful login
https://github.com/nitishkumar71/faasd/runs/3058438661?check_suite_focus=true

An observation. I wasn't able to run the faasd test with env variables asCI=true and OPENFAAS_CONFIG=.openfaas, instead i had to move .openfaas folder to home directory.

@nitishkumar71
Copy link
Member

@LucasRoesler As majority of test cases are failing due to use of namespace into url. We may have to avoid setting using namespace for faasd. So atleast one flag will be required for same.

@LucasRoesler
Copy link
Member

Sorry about that, missed it somehow.

Here is the one after successful login
https://github.com/nitishkumar71/faasd/runs/3058438661?check_suite_focus=true

An observation. I wasn't able to run the faasd test with env variables asCI=true and OPENFAAS_CONFIG=.openfaas, instead i had to move .openfaas folder to home directory.

Ineteresting, any theories on what is broken there?

@LucasRoesler
Copy link
Member

@LucasRoesler As majority of test cases are failing due to use of namespace into url. We may have to avoid setting using namespace for faasd. So atleast one flag will be required for same.

OK, so it sounds like we just can't enabled mutlinamespace at all. Good to know, I am not sure we need to add any special flags for faasd, just not adding extra namespaces should be enough, right?

@nitishkumar71
Copy link
Member

We may have to change the current code. We are appending namespace into the function calling URL.

uri := resourceURL(t, path.Join("function", fmt.Sprintf("%s.%s", function.FunctionName, function.Namespace)), query)

@LucasRoesler
Copy link
Member

@alexellis what do you think? Should we try to address this in fassd or change the certifier? I guess it would require a "noop" implementation that handles the namespace suffix in the url

@alexellis
Copy link
Member Author

I think that we should do whatever faas-netes does when it has a single namespace configuration. So probably some change in faasd to enable this usecase.

Faasd does use a namespace of openfaas-fn which cannot be changed at the moment, but is a value in containerd.

@nitishkumar71
Copy link
Member

nitishkumar71 commented Aug 12, 2021

Raised an PR in faasd to provide support for multiple namespace. Majority of test cases work for certifier with it. Those who fails, we would have to disable them for faasd.

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

No branches or pull requests

4 participants