Provide error when no config was found

[jonkerj]

What this PR does / why we need it:
The control loop currently segfaults when none of the config files was found. This commit makes it give an actual error, providing the user with useful feedback.

Which issue(s) this PR fixes
If needed, I'll raise an issue, please let me know.

Special notes for your reviewer (optional):

[maiqueb]

Could you also add a couple of unit tests to https://github.com/k8snetworkplumbingwg/whereabouts/blob/master/pkg/config/config_test.go ?

Let me know if you need any help with it.

[jonkerj]

Sure, if I can bother you to look at #226 😉 (unrelated to this issue, but I think it deserves some attention too)

[jonkerj]

Could you also add a couple of unit tests to https://github.com/k8snetworkplumbingwg/whereabouts/blob/master/pkg/config/config_test.go ?

Let me know if you need any help with it.

Well I tried to, but then I discovered that a lot (maybe all) tests in config_test.go are failing now, because GetFlamIPAM (which throws an error when no config files are present) is invoked from LoadIPAMConfig. I'm not really sure if LoadIPAMConfig needs a bit of refactoring to detect this type of error, or if the tests need rewriting, or maybe even something else.

Do you have a hint for me?

[jonkerj]

ping @maiqueb

[maiqueb]

Could you also add a couple of unit tests to https://github.com/k8snetworkplumbingwg/whereabouts/blob/master/pkg/config/config_test.go ?
Let me know if you need any help with it.

Well I tried to, but then I discovered that a lot (maybe all) tests in config_test.go are failing now, because GetFlamIPAM (which throws an error when no config files are present) is invoked from LoadIPAMConfig. I'm not really sure if LoadIPAMConfig needs a bit of refactoring to detect this type of error, or if the tests need rewriting, or maybe even something else.

Do you have a hint for me?

Yeah, so now that you return an error when there's no config present (I agree this should happen), the tests should be re-written to ensure this configuration is available to the tests.

Let's make sure @dougbtv is on board with the conceptual change first if you don't mind.

I can guide you on adapting the tests later on.

Stay tuned.

[maiqueb]

FWIW, the changes you need to do in the tests are quite small.

Here's what you need to do (using as reference the first test, you need to do something similar for all the failing ones):

       var tmpDir string // this must be accessible from the tests
 
       BeforeEach(func() {
               var err error
               tmpDir, err = os.MkdirTemp("", "whereabouts") // must create a tmp dir to store the configs
               Expect(err).NotTo(HaveOccurred())
       })

       AfterEach(func() {
               Expect(os.RemoveAll(tmpDir)).To(Succeed()) // must clean-up the generated configs
       })

      It("can load a basic config", func() {
		conf := `...` // each test will have a different conf; leave the original configuration you have in each test

		confPath := fmt.Sprintf("%s/%s", tmpDir, "whereabouts.conf") 
		Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) // must persist the conf in the file-system

		ipamconfig, _, err := LoadIPAMConfig([]byte(conf), "", confPath) // must pass the new conf-path to the func; it only knows about the default paths
		Expect(err).NotTo(HaveOccurred())
		... // further expectations, each test looks for different stuff
	})

[maiqueb]

@jonkerj alright! @dougbtv 's on board with this change.

Please update the failing tests in a similar way to what I'm proposing in this link.

Also please add a unit test that expects an error to be thrown - because the config on disk is not available.

[jonkerj]

Hi, sounds allright, I will look into that in a week or so.

[jonkerj]

I've updated my PR with a commit that fixes those tests. All is green on my side. I am recycling the filename whereabouts.conf throughout the tests, because (if I understand everything correct) it gets reinitialized every test (BeforeEach et al) or gets overwritten anyways.

[maiqueb]

I only have two minor comments. Other than that, looks good.

[maiqueb]

You have plenty of failed unit tests in cmd/whereabouts_test.go .

You need to take care of them.

[jonkerj]

You are right, I need to fix the unit tests on way more places than I did. I only tested github.com/k8snetworkplumbingwg/whereabouts/pkg/config, previously, my bad.

[jonkerj]

OK, we are almost there. There are still 5 unit tests failing in cmd/whereabouts_test.go, and I'm again unsure how to proceed. In one of the tests (allocate and release addresses on ADD/DEL), ipamConfig(..) is called, which in turn calls config.LoadIPAMConfig(..), which now throws an error.

I think simply ignoring ConfigFileNotFoundError in cmd/whereabouts_tests.go around lines 1451-1454 would mimic the previous behavior (as no config found would be ignored anyways), right?

Also: I feel a bit awkward about both passing byte[](conf) and write it to a file and pass that to LoadIPAMConfig (feels like it's not really DRY). How do you think about that?

[maiqueb]

OK, we are almost there. There are still 5 unit tests failing in cmd/whereabouts_test.go, and I'm again unsure how to proceed. In one of the tests (allocate and release addresses on ADD/DEL), ipamConfig(..) is called, which in turn calls config.LoadIPAMConfig(..), which now throws an error.

I think simply ignoring ConfigFileNotFoundError in cmd/whereabouts_tests.go around lines 1451-1454 would mimic the previous behavior (as no config found would be ignored anyways), right?

Yeah, that was the previous behavior.

... but IMO, the goal of this PR is to get rid of the previous behavior and ensure an error is thrown when a config is not provided. This must be reflected in the unit tests.

IMO, you cannot just ignore the error; you should either do a proper setup of the tests, or mock the real component, providing an implementation that does what you want.

Maybe the later is simpler.

Also: I feel a bit awkward about both passing byte[](conf) and write it to a file and pass that to LoadIPAMConfig (feels like it's not really DRY). How do you think about that?

The whole API could be improved to be honest ...

If you choose to pursue that, please do it in a separate PR.

[jonkerj]

OK, we are almost there. There are still 5 unit tests failing in cmd/whereabouts_test.go, and I'm again unsure how to proceed. In one of the tests (allocate and release addresses on ADD/DEL), ipamConfig(..) is called, which in turn calls config.LoadIPAMConfig(..), which now throws an error.

I think simply ignoring ConfigFileNotFoundError in cmd/whereabouts_tests.go around lines 1451-1454 would mimic the previous behavior (as no config found would be ignored anyways), right?

I did something else: I os.WriteFile'd the mashalled config to a tempfile, similar to your earlier suggestion. As the function prototype of ipamConfig does not allow for passing errors currently (that needs significant rewrite of the 5 involved tests), it nils out when anything fails (which cause the test to fail). Not the prettiest, but all tests pass now.

EDIT: they pass locally. I'm working on getting them to pass in GH actions

[jonkerj]

All is OK, all tests pass now config files are created. I'll save refactoring for DRY for another rainy day ;-)

[maiqueb]

Thank you for this change, and your willingness to jump through all the hurdles I kept setting for you.

[jonkerj]

you're welcome!

[coveralls]

Pull Request Test Coverage Report for Build 4871473391

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 5 of 8 (62.5%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.1%) to 71.976%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/config/config.go	5	8	62.5%

Totals
Change from base Build 4721797056:	-0.1%
Covered Lines:	976
Relevant Lines:	1356

---

💛 - Coveralls