Add first golang-only test

[uncleDecart]

What

Instead of using escript to write tests, one now can write tests fully in golang and run those like this

cd tests/sec
go test

So there's no layered complexity anymore, compared to calling calling forked from golang test binary, which is written by us which actually interprets text files as bash commands, execs those via exec and sometimes substitute some configuration with that's been written in specification below or call arbitratry bash scripts, or compare values from stdout and file using one character command !

Instead, using openevec package and evetestkit one can write test, which would look something like this

package sec_test

import (
	"os"
	"path/filepath"
	"strings"
	"testing"

	"github.com/lf-edge/eden/pkg/defaults"
	tk "github.com/lf-edge/eden/pkg/evetestkit"
	"github.com/lf-edge/eden/pkg/openevec"
	log "github.com/sirupsen/logrus"
)

const projectName = "security-test"
const appArmorStatus = "/sys/module/apparmor/parameters/enabled"

var eveNode *tk.EveNode

func TestMain(m *testing.M) {
	log.Println("Security Test Suite started")
	defer log.Println("Security Test Suite finished")

	currentPath, err := os.Getwd()
	if err != nil {
		log.Fatal(err)
	}
	twoLevelsUp := filepath.Dir(filepath.Dir(currentPath))

	cfg := openevec.GetDefaultConfig(twoLevelsUp)

	if err = openevec.ConfigAdd(cfg, cfg.ConfigName, "", false); err != nil {
		log.Fatal(err)
	}

	evec := openevec.CreateOpenEVEC(cfg)
	configDir := filepath.Join(twoLevelsUp, "eve-config-dir")
	if err := evec.SetupEden("config", configDir, "", "", "", []string{}, false, false); err != nil {
		log.Fatalf("Failed to setup Eden: %v", err)
	}
	if err := evec.StartEden(defaults.DefaultVBoxVMName, "", ""); err != nil {
		log.Fatalf("Start eden failed: %s", err)
	}
	if err := evec.OnboardEve(cfg.Eve.CertsUUID); err != nil {
		log.Fatalf("Eve onboard failed: %s", err)
	}

	node, err := tk.InitilizeTestFromConfig(projectName, cfg, tk.WithControllerVerbosity("debug"))
	if err != nil {
		log.Fatalf("Failed to initialize test: %v", err)
	}

	eveNode = node
	res := m.Run()
	os.Exit(res)
}

func TestAppArmorEnabled(t *testing.T) {
	log.Println("TestAppArmorEnabled started")
	defer log.Println("TestAppArmorEnabled finished")
	t.Parallel()

	out, err := eveNode.EveReadFile(appArmorStatus)
	if err != nil {
		t.Fatal(err)
	}

	exits := strings.TrimSpace(string(out))
	if exits != "Y" {
		t.Fatal("AppArmor is not enabled")
	}
}

Why

The advantages are

1. you need golang competences, no escript competencies
2. you can trace the whole test within one debug session, WYSIWYG
3. dev tools like profiling, debugger are available out of the box.

How

First couple of commits are solely focused on removing viper from different parts of openevec. This is needed because throughout the code base viper is treated as shared state between multiple programs, like escript, changers for controller, eden and some parts of openevec. The problem is when you want to have eden as library, i.e. call functions from golang code you pretty much want to have configuration passed as parameter (or be wrapped around methods, like it's done in openevec) and you'd expect it to use those values, so it's easier to track, debug, understand the code (in modern editors you can just call the is referred), but unfortunately, in some places we just assume that some value is written in the configuration file and we just open the file from a "default" path and read its' value, which could be useful, and it certainly gives you better function readability (you don't need to pass parameters) but you don't know what this function depends on, and that actually can be very confusing. And in the scope of this PR it could lead to some weird behavior. Unfortunately, I couldn't remove everything, changers still rely on this, that's why we create configuration and write it to disc. Fortunately, we only consume configuration, so shouldn't be any trickery for now, but we need to get rid of it (see next section)
That actually also led to moving .sh and .csh templates from files to constants inside golang. Easier to debug it that way, plus I want to use config object, not read things from viper config saved somewhere.

Second high-level change is in 96517 commit is moving testcontexts to projects to avoid circular dependency cycles. Again, we don't want to rely on viper in evetestkit to create testcontext, we want it to create it from a configuration object we specify. So some jumps in hoops were needed. But that cleared the way to write golang only test

Third major thing is backward compatibility with existing tests. Leaving burning ground behind, would be a barbaric thing to do. Because of changes introduced in configuration file (namely we write config now ourselves, without using any templates WSYSIWYG) broke some (all) tests, so last commit is about making CI/CD work :)

Important (caveats and hoops I had to jump through)

• I removed sec test from escript since it's now part of NeoEden, before any release we should setup GHA pipeline to run NeoEden alongside escript tests.
• Until we remove all the viper dependencies, NeoEden tests have to start from directory up, meaning in eden folder, assuming they're located in test folder

Is it bright future? (Future work)

I believe that with this patch the entry level to write eden tests drops significantly, now you'll be confused by golang code, but you'll be armed with all the tools and the community to improve the code and I do believe there's still big place to improve, but including new people into this will be significantly easier now (he said in naive hope).

Big things that we have to still do after we merge this (help wanted)

• Mental health day off
• Transfer escript tests to NeoEden (@shjala did amazing work, but still help wanted, no better way to understand component of EVE than write a test for it)
• Deploy Eden on baremetal (I believe @christoph-zededa is on it)
• Refactor 🎨 the way we store state in Adam. I believe there's a huge space for improvement to reduce time to get states, maybe improving adam and moving parts of the code there to create an SDK out of it, those are just ideas)
• Get rid of viper from changers. That will easy way to support multiple EVE instances with just handling config file

Yours truly with some notes of graphomania

[uncleDecart]

The pitfall is that we still write config, couldn't figure this one out. Time to fight the my way through CI/CD

[uncleDecart]

This should close #828

[uncleDecart]

Need to change project in tests...

[shjala]

This is a big one!

[uncleDecart]

locally smoke tests passed, removed sec_test because it's now part of NeoEden. Let's see how pipeline will behave 🤞

[uncleDecart]

any zfs experts or any advice on how I can run things locally to debug this thing?

[uncleDecart]

Okay, ZFS is not working, one smoke test failed on shutdown, I remember it was flaky. I'll look into zfs now, but it's getting closer

[uncleDecart]

Seems promising, hitting TOOMANYREQUESTS on User app test

[giggsoff]

Looks like you disable setup in workflows and move its logic here. Do you have any reason for that?

[giggsoff]

Will we keep our collect-info and other debugging logic in case of failure on onboarding stage, which is quite predictable?

[uncleDecart]

Hey @giggsoff , nice seeing you here :) So I did it purely from backward compatibility. For some reason escript tests didn't want to onboard through test.

Yes, we keep all collect-info logic there, no changes in that regard

[deitch]

Much better. I can read the docs without too much prior knowledge and get a feel how to do things.

I made some suggestions related to typos and clarifying minor things.

Additional points:

• what about teardown? Don't I need to use evec for that? Shouldn't that be at the end of TestMain?
• People using this might (or might not) be used to TestMain, so it is worth putting in the m.Run() in the example. In that light, maybe link to the TestMain official docs?
• Is there a better way to pass the node around than is a package-level variable? If not, ok, but worth asking.
• Are there concerns about tests run in parallel? I would think they conflict with a single controller, so it might be worth stating explicitly not to invoke any .Parallel()
• I still am a little confused at the border between openevec and evetestkit. It is related to the context-passing question. I see very clearly how I use openevec to initialize things. But then I use evetestkit to get a node, and use that node to do things on it. Would "get a node" be part of openevec, and then evetestkit is just a set of convenient library functions? I am not sure I get where the border is.

[deitch]

It would be great if this (and many other files) were parametrized, so they could be a property of some library. But that is beyond the scope of here.

[uncleDecart]

what about teardown? Don't I need to use evec for that? Shouldn't that be at the end of TestMain?

We can execute openevec.EdenClean(), ideally we should discuss how we should proceed with it. Because technically make clean does more than EdenClean(), it removes files as well, we need to fix it separately, this vicious cycle shouldn't be there (i.e. you should have teardown in tests). Hope that clarifies a bit :D

People using this might (or might not) be used to TestMain, so it is worth putting in the m.Run() in the example. In that light, maybe link to the TestMain official docs?

Done

Is there a better way to pass the node around than is a package-level variable? If not, ok, but worth asking.

It's a good question: I would actually gather people to chat about evetestkit and how better to structure things there.

Are there concerns about tests run in parallel? I would think they conflict with a single controller, so it might be worth stating explicitly not to invoke any .Parallel()

Nice catch! Done

I still am a little confused at the border between openevec and evetestkit. It is related to the context-passing question. I see very clearly how I use openevec to initialize things. But then I use evetestkit to get a node, and use that node to do things on it. Would "get a node" be part of openevec, and then evetestkit is just a set of convenient library functions? I am not sure I get where the border is.

The problem is we haven't defined the border and now would be a really good time to discuss where this border should be.

On an all I don't now if this PR is the best place to do so, but I want to talk about design of golang test framework and two entities constituting Eden and what separation would be the best. We surely could (and I think should) start here to get some pointers on that discussion.

My take: openevec is about "basic" operations, which is: create EVE image, start controller, start EVE, enable networking with specific configuration, deploy specific App to EVE, ssh to EVE. Sort of create a setup and provide tools to access to components of that configuration based on configuration itself (I want to ssh to EVE, but actually if SDN is enabled I need to forward my request through it I want this logic to be hidden behind openevec, where I just say ssh to EVE; same applies to access controller, could be adam, could be commercial controller, could be anything else, I just want to access controller)

Then there's evetestkit which utilizes in some way openevec and provide functions of convenience, like ReadFile (which is actually Ssh to EVE and run cat command) or check that this App on Controller is in UP state (which is actually access controller, how to do that is hidden behind openevec)

[uncleDecart]

@deitch third take on docs answered your questions above ^^^

[deitch]

It's a good question: I would actually gather people to chat about evetestkit and how better to structure things there.

It really isn't part of the question related to this, as it is to passing variables around between tests. When you have a TestMain, and want to pass things around to individual tests when you do m.Run(), how do you do it? If there is no clean answer right now, let it go for now. It can be addressed later, and isn't fundamental to what you have done here.

The problem is we haven't defined the border and now would be a really good time to discuss where this border should be.
I want to talk about design of golang test framework and two entities constituting Eden and what separation would be the best. We surely could (and I think should) start here to get some pointers on that discussion.

Yup, I think you started to do so in a good way:

My take: openevec is about "basic" operations, which is: create EVE image, start controller, start EVE, enable networking with specific configuration, deploy specific App to EVE, ssh to EVE. Sort of create a setup and provide tools to access to components of that configuration based on configuration itself (I want to ssh to EVE, but actually if SDN is enabled I need to forward my request through it I want this logic to be hidden behind openevec, where I just say ssh to EVE; same applies to access controller, could be adam, could be commercial controller, could be anything else, I just want to access controller)

Then there's evetestkit which utilizes in some way openevec and provide functions of convenience, like ReadFile (which is actually Ssh to EVE and run cat command) or check that this App on Controller is in UP state (which is actually access controller, how to do that is hidden behind openevec)

So is it correct to say that openevec is all things that I do in tests, whether infrastructure type stuff (set up eden, start the controller, start a virtual device, onboard a device, etc. etc.), and evetestkit is just ease-of-use utilities? I can do everything without evetestkit, if I want, it is just a bit of a pain, I have to do lots of lower-level commands repetitively. With evetestkit, I get a bunch of convenience functions. evetestkit doesn't talk to the filesystem, doesn't know about sockets or infrastructure or anything directly. It just knows how to use the lower-level functions/structs/interfaces provided by openevec, or structs/interfaces generated by it (e.g. a node) to do my stuff.

If that is correct - and I think that makes sense, although the naming "evetestkit" is a bit misleading, but not so far that it is worth changing (and I don't have a better name, so I have no right to complain), then I think it is a pretty solid line.

If that is true, though, then evetestkit should not depend on anything local or direct access to anything. The only parameters to any of its functions should be one of:

• something returned by openevec
• configuration (like the name of the app to deploy, or the file to check, etc.)
• something itself returned by evetestkit (to build higher level utilities)

If that is true, then it is a clean, easy to understand division of labour.

[uncleDecart]

It's a good question: I would actually gather people to chat about evetestkit and how better to structure things there.

It really isn't part of the question related to this, as it is to passing variables around between tests. When you have a TestMain, and want to pass things around to individual tests when you do m.Run(), how do you do it? If there is no clean answer right now, let it go for now. It can be addressed later, and isn't fundamental to what you have done here.

Seems like it's a common practice

Regarding the separation: we need to move some of the functions from there for it to be able to utilize openevec only. I suggest we also get input from @shjala and do it in a separate PR, I don't want to bloat this one, let's iterate, it's easier

[deitch]

Seems like it's a common practice

Doesn't make it a good practice. 😆
Either way, don't worry about it now. It works.

Regarding the separation: we need to move some of the functions from there for it to be able to utilize openevec only. I suggest we also get input from @shjala and do it in a separate PR, I don't want to bloat this one, let's iterate, it's easier

OK, as long as we get it done. The whole point of your excellent work here is to make it easier to use and understand.

[uncleDecart]

Okay, most of the requests are done, one left with config marshaling. Once this is merged, before releasing eden 0.1.0 we gotta do following:

1. Merge OVMF boot issue workaround #1041
2. Implement workflow to run NeoEden alongside with escript
3. Talk about what do want from evetestkit on a higher level
4. Fix evetestkit
5. Migrate some test to a new way
6. Pat ourselves on the back for a job well done

[uncleDecart]

I think I addressed all the comments, @deitch if you don't have any comments let's merge it after releasing 0.9.13 (we need hotfix for Eden tests)

[deitch]

We still have work to do to make it cleaner separation for evetestkit, but if I understand you correctly, that is beyond the scope of this PR, so good.

One final request:

func InitilizeTest()
func InitilizeTestFromConfig()

Can you please fix these typos?

func InitializeTest()
func InitializeTestFromConfig()

It confuses people, hard to search, and doesn't make us look as good.

Other than that, good to go.

[uncleDecart]

Shoot, missed the typo, thanks Avi!

[uncleDecart]

@deitch fixed the typo

[deitch]

yetus is unhappy, but other than that LGTM

[uncleDecart]

I see now only complaint to bump GHA from Yetus now

[uncleDecart]

Merging this, yetus complaints are about GHA bumps. Escript is deprecated. May the force be with us

[milan-zededa]

Nice work! (coming late to the party...)

[uncleDecart]

Not at all @milan-zededa, we need to port all those escipt tests to new fancy golang framework, want to help? :D

[milan-zededa]

Not at all @milan-zededa, we need to port all those escipt tests to new fancy golang framework, want to help? :D

That's a very tempting offer, I'll think about it ;)

[uncleDecart]

That's a very tempting offer, I'll think about it ;)

;-)