certifier icon indicating copy to clipboard operation
certifier copied to clipboard

Certify multi-namespace support

Open LucasRoesler opened this issue 4 years ago • 11 comments

The certifier currently does not know about multi-namespace support in faas-netes. Additionally, we would like to eventually bring namespace support to faasd. We should update the certifier to validate that multi-namespace support works as expected. It is likely to find bugs for us and help verify when faasd is done with its own version of namespace support.

I think the ideal flow for this is to add a new config or env variable that is a list of namespaces, for exmaple

CERTIFIER_NAMESPACES=testa,testb,testc

when this value is set, the tests (where it makes sense) would run multiple times, one per namespace in the list.

When the value is empty, the certifier should default to its current behavior.

LucasRoesler avatar Mar 20 '21 09:03 LucasRoesler

I will work on it.

nitishkumar71 avatar Mar 22 '21 02:03 nitishkumar71

Cool, :+1: @nitishkumar71 i think the first step is to create a PR that adds a config object and parsing from env variables, something like

type Config struct {
   Namespaces []string
}

func FronEnv() Config {
   // read CERTIFIER_NAMESPACES variable, parse as csv string
}

Once we have that, we can start using it in the test cases, starting with the deploy test. I think it is simplest to just take it all in small steps, one test as a time essentially.

LucasRoesler avatar Mar 23 '21 09:03 LucasRoesler

@nitishkumar71 just want to follow up on how this is going. The expected change should be extending this section of the existing code https://github.com/openfaas/certifier/blob/2d4a282ad649eff8e75807e71e470e9216571f63/tests/main_test.go#L21-L40 to add the new parsing

We don't need to use the new value in the tests yet, we want to go through each test 1 by 1, but we can can a log line to pretty pring the Config here https://github.com/openfaas/certifier/blob/2d4a282ad649eff8e75807e71e470e9216571f63/tests/main_test.go#L72 so that we can see the parsing is working as expected.

LucasRoesler avatar Mar 27 '21 09:03 LucasRoesler

@LucasRoesler Sorry for delay. I have included logic to get namespaces name from env variables. Which test do you think would be better to first stage for it?

nitishkumar71 avatar Mar 27 '21 18:03 nitishkumar71

@LucasRoesler Sorry for delay. I have included logic to get namespaces name from env variables. Which test do you think would be better to first stage for it?

No worries, i will take a look at your PR tomorrow. I wrote up this issue as a suggested first endpoint we should verify https://github.com/openfaas/certifier/issues/63

LucasRoesler avatar Mar 27 '21 19:03 LucasRoesler

@nitishkumar71 Now that we have the verification for ListNamespaces, i think we need to look at the function lifecycle. I think there are two ways we can approach this

  1. always pick a non-default namespace, if one is provided in the config. This has the benefit of simplicity. But it means we would need to run the test suite twice to verify the behavior for the empty/default case as well.

  2. we can automatically run the whole suite twice by moving all tests to being sub-tests and then running it in a loop, essentially treating it like a table tests (see https://dave.cheney.net/2019/05/07/prefer-table-driven-tests), for example

    func TestCertify(t *testing.T) {
    
        testNamespaces := []string{""}
        if len(config.Namespaces) > 0 {
            testNamespaces = append(testNamespaces, config.Namespaces[0])
        }
    
        for _, namespace := testNamespaces {
            // all of the tests we identify to include 
            // t.Run(fmt.Sprint("test function deployment in %s", namespace), func(t *testing.T) {
            //    
            // })
        }
    
    }
    
    // then other tests that don't need to verify namespace behavior, really only those methods/endpoints that can't accept a namespace as an input
    

    this saves some repetition of logic, but is a little less friendly for running individual tests via the IDE integration, for example image

  3. a variation of (2), identify which tests need to verify namespace behavior (which is most) and then do a minim table driven test in each of those, it would look similar to (2) but per each test. This means we need to repeat the looping logic in all of these tests, but individual test cases are easier to run via the IDE.

  4. create one "happy path" test for the function life cycle with a non default namespace. So instead of doubling all of the tests. This requires the least amount of refactor to existing tests and avoids doubling the test times, while still verify most of the behavior for namespaces. However, it will probably miss some things.

  5. some other idea? I am open to alternative suggestions

LucasRoesler avatar Apr 07 '21 13:04 LucasRoesler

Ideally, Option 3 and 4 look good to me. The only Drawbacks I can think of are

  1. In option 3 we will add more time to test
  2. In Option 4 we will have to add additional repeated code, assuming this understanding is correct.

Possibly We can try to find some other application, who needs to perform similar tests and look at their approach.

nitishkumar71 avatar Apr 08 '21 09:04 nitishkumar71

Ok, Let's try option 3 then with the deploy portion of the tests, they are a natural place to start, i'll create a new issue, in the mean time we can also look at some other approaches and then compare with how we feel option 3 looks in the deploy tests

LucasRoesler avatar Apr 08 '21 11:04 LucasRoesler

@nitishkumar71 now that we have a pattern to work with based on the Deploy tests, this leaves the invoke_test.go, scaling_test.go, logs_test.go ,and secretCRUD_test.go.

Do you want to do one of those and I can take one of the others?

LucasRoesler avatar Apr 18 '21 11:04 LucasRoesler

@LucasRoesler Yes, I can. Do let me know which one should I take?

nitishkumar71 avatar Apr 18 '21 12:04 nitishkumar71

@nitishkumar71 you can to take Secrets (#67) or invoke (#69)

I will take logs (#68)

LucasRoesler avatar Apr 18 '21 15:04 LucasRoesler