Refactor enirouting package to reduce code interdependence and add test coverage #11208

christarazi · 2020-04-28T22:56:41Z

See commit messages.

Follow-up refactor from previous PR: #11073

christarazi · 2020-04-28T23:00:15Z

test-me-please

Edit: build went green, just retriggering because I needed to rebase

coveralls · 2020-04-28T23:24:25Z

Coverage increased (+0.03%) to 44.463% when pulling fe777ed47acd9badcdb010e026ec4b6da3b206ec on christarazi:pr/christarazi/refactor-enirouting-pkg into 3ec3d25 on cilium:master.

christarazi · 2020-04-29T00:24:37Z

pkg/aws/eni/routing/info.go

+// NewRoutingInfo creates a new RoutingInfo struct from a source which adheres
+// to the getter interface. The data from the source is then parsed and
+// validated.
+func NewRoutingInfo(getter getter) (*RoutingInfo, error) {


@joestringer I've tried out your suggestion from: #11073 (comment)

After implementing it, I'm not sure what benefits we get from abstracting away the source of the info via the getter interface. I think we'd get the same functionality and non-code-interdependence from just having the NewRoutingInfo take the raw values as parameters.

Is there something I'm missing that the interface would buy us?

Thanks for trying it out, good question :)

Looking closer, in this case the ability to fetch the routing info, ie the implementation of getter is trivial; it's just taking the API response structure and returning the values inside. So in this current implementation I'm inclined to agree that the benefit is not obvious. Right now, it allows pkg/aws/eni/routing to avoid importing the API structures, but I don't think that's a big deal; we're already assuming a model around the cloud IPAM API responses in all of the ENI code structure anyway so the abstraction doesn't help.

If the full implementation of that getter was more complex (For example actually involved pulling values from an API) then I think it would make a bigger difference.

Looks like we got more value out of the routingConfigurer above, which allowed us to remove ENI references from that package (module the variable names, which should probably be simplified).

Oh, perhaps the unsaid piece above: With this style of implementation, you can create your own dummy getter and actually unit-test the parse() function below trivially. In this specific case, if we remove it, then you just have to instantiate IPAM API response structures in the tests which is probably still straightforward.

If the getter was more complex then just to unit test this functionality it would be more work.

Right. The retrieval of these ENI routing values is trivial and will probably continue to be, since it's inherently a cloud API interaction. So our getter isn't helping by providing any useful abstraction.

I'm going to take out the getter and replace the constructor with raw string values as input instead. This will still allow us to unit-test parse easily since it will just take those values as well.

(module the variable names, which should probably be simplified).

Which variable names need to be simplified?

Oh, I mean eniHealthRouting -> routeConfiguration. This assumes an eni implementation but it doesn't make sense to assume that when a generic routeConfigurer is handed down.

christarazi · 2020-04-29T01:09:03Z

test-me-please

christarazi · 2020-04-29T19:34:43Z

test-me-please

joestringer · 2020-05-05T20:34:32Z

What's the intent behind marking this for backport? Usually we have slightly stricter backport criteria for existing releases:
https://docs.cilium.io/en/v1.7/contributing/release/backports/#backport-criteria-for-current-minor-release

We may make exceptions in cases where we for example increase testing coverage, or where we expect that operating Cilium or debugging Cilium may become easier because the codepaths are more similar between versions. Just want to make sure that we've thought through why we're backporting this refactoring change.

Also is this close? Seems like the CI is passing at least, but it's currently marked as Draft.

christarazi · 2020-05-05T20:44:33Z

The intent is to ensure the code doesn't diverge too much from master. Also, I am working on adding tests as part of this PR. I should've clarified that in the description.

Yes it is close, I'm wrapping up the tests and should have this PR done by end-of-day.

christarazi · 2020-05-06T01:07:14Z

test-me-please

christarazi · 2020-05-06T01:12:21Z

While adding test coverage, I realized that we needed to return an error if deleting a rule fails. The error is non-fatal as the deletion code is made at the endpoint-level, meaning that the scaffolding around this will aggregate all errors that occur and show a log message. The reason for adding this now is because previously, the log messages could have been misleading if an error occurs. Also, it was difficult to assert the correct behavior in all cases. See commit 7eb1c8c.

christarazi · 2020-05-06T01:15:01Z

test-me-please

Previously, we relied on a log message to indicate whether a deletion of a rule failed. Relying on the log message can be potentially misleading as endpoint deletion is best-effort and errors are ignored in the end. The user may see log message indicating a rule was deleted, but later in the endpoint deletion it would show as an error. This commit changes the function which deletes rules to surface the error. This change removes the misleading log messages and allows us to assert against this in tests. Signed-off-by: Chris Tarazi <chris@isovalent.com>

This commit structures the code in such a way that allows the previously duplicated parsing/validation logic to be consolidated into one place. It also reduces the interdependencies between packages and allows for a dummy implementation to be used for testing purposes. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi · 2020-05-06T18:40:39Z

Build went green. Pushed to rebase

Edit: travis went red on this push, with this flake #11373

christarazi · 2020-05-06T20:09:13Z

test-me-please

joestringer · 2020-05-06T21:38:59Z

Hit #11373 .

maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 28, 2020

joestringer added needs-backport/1.7 and removed backport/1.7 labels Apr 29, 2020

christarazi commented Apr 29, 2020

View reviewed changes

christarazi changed the title ~~Refactor enirouting package to utilize interfaces to abstract away source of routing info~~ Refactor enirouting package to utilize interfaces to reduce code interdependence Apr 29, 2020

christarazi changed the title ~~Refactor enirouting package to utilize interfaces to reduce code interdependence~~ Refactor enirouting package to reduce code interdependence Apr 29, 2020

christarazi force-pushed the pr/christarazi/refactor-enirouting-pkg branch from 32bd730 to 7f84987 Compare April 29, 2020 01:08

christarazi force-pushed the pr/christarazi/refactor-enirouting-pkg branch from 7f84987 to 0ba3950 Compare April 29, 2020 19:33

joestringer added this to Needs backport from master in 1.7.4 Apr 29, 2020

christarazi changed the title ~~Refactor enirouting package to reduce code interdependence~~ Refactor enirouting package to reduce code interdependence and add test coverage May 5, 2020

christarazi force-pushed the pr/christarazi/refactor-enirouting-pkg branch from 0ba3950 to a29480c Compare May 6, 2020 01:06

christarazi force-pushed the pr/christarazi/refactor-enirouting-pkg branch from a29480c to fe777ed Compare May 6, 2020 01:14

christarazi marked this pull request as ready for review May 6, 2020 02:05

christarazi requested review from a team May 6, 2020 02:05

christarazi requested review from a team and joestringer May 6, 2020 02:05

soumynathan mentioned this pull request May 6, 2020

Validate when Cilium is in ENI mode that IPv4 is enabled #11328

Merged

aanm approved these changes May 6, 2020

View reviewed changes

christarazi added 3 commits May 6, 2020 11:39

Add unit-tests for enirouting package

b93f4d0

Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi force-pushed the pr/christarazi/refactor-enirouting-pkg branch from fe777ed to b93f4d0 Compare May 6, 2020 18:40

joestringer approved these changes May 6, 2020

View reviewed changes

christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 7, 2020

joestringer merged commit a83d538 into cilium:master May 7, 2020

1.8.0 automation moved this from In progress to Merged May 7, 2020

christarazi deleted the pr/christarazi/refactor-enirouting-pkg branch May 7, 2020 01:29

christarazi mentioned this pull request May 8, 2020

v1.7 backports 2020-05-08 #11441

Merged

christarazi added backport-pending/1.7 and removed needs-backport/1.7 labels May 8, 2020

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.4 May 8, 2020

aanm added backport-done/1.7 and removed backport-pending/1.7 labels May 13, 2020

maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.4 May 13, 2020

christarazi mentioned this pull request May 20, 2020

Rework netns handling in LinuxRoutingSuite privileged tests #11620

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor enirouting package to reduce code interdependence and add test coverage #11208

Refactor enirouting package to reduce code interdependence and add test coverage #11208

christarazi commented Apr 28, 2020 •

edited

christarazi commented Apr 28, 2020 •

edited

coveralls commented Apr 28, 2020 •

edited

christarazi Apr 29, 2020

joestringer Apr 30, 2020

joestringer Apr 30, 2020

christarazi May 1, 2020

christarazi May 1, 2020

joestringer May 1, 2020

christarazi commented Apr 29, 2020

christarazi commented Apr 29, 2020

joestringer commented May 5, 2020

christarazi commented May 5, 2020 •

edited

christarazi commented May 6, 2020

christarazi commented May 6, 2020 •

edited

christarazi commented May 6, 2020

christarazi commented May 6, 2020 •

edited

christarazi commented May 6, 2020

joestringer commented May 6, 2020

Refactor enirouting package to reduce code interdependence and add test coverage #11208

Refactor enirouting package to reduce code interdependence and add test coverage #11208

Conversation

christarazi commented Apr 28, 2020 • edited

christarazi commented Apr 28, 2020 • edited

coveralls commented Apr 28, 2020 • edited

christarazi Apr 29, 2020

Choose a reason for hiding this comment

joestringer Apr 30, 2020

Choose a reason for hiding this comment

joestringer Apr 30, 2020

Choose a reason for hiding this comment

christarazi May 1, 2020

Choose a reason for hiding this comment

christarazi May 1, 2020

Choose a reason for hiding this comment

joestringer May 1, 2020

Choose a reason for hiding this comment

christarazi commented Apr 29, 2020

christarazi commented Apr 29, 2020

joestringer commented May 5, 2020

christarazi commented May 5, 2020 • edited

christarazi commented May 6, 2020

christarazi commented May 6, 2020 • edited

christarazi commented May 6, 2020

christarazi commented May 6, 2020 • edited

christarazi commented May 6, 2020

joestringer commented May 6, 2020

christarazi commented Apr 28, 2020 •

edited

christarazi commented Apr 28, 2020 •

edited

coveralls commented Apr 28, 2020 •

edited

christarazi commented May 5, 2020 •

edited

christarazi commented May 6, 2020 •

edited

christarazi commented May 6, 2020 •

edited