Reorganise web test code for use outside repo#17457
Conversation
Extract the `webSuite` type from apiserver_test.go and bring it into the lib/web package so that it can be used from outside the lib/web package. This will allow the movement of the enterprise SSO code to the enterprise repository, retaining the existing tests. `webSuite` has been renamed to `TestWebSuite` to highlight that it exists for testing purposes and is not part of the lib/web package for non-testing purposes. Ideally this would be moved to a separate fixtures package but circular dependencies prevent that for now - larger changes would be needed. Some ancillary types and functions have also been extracted as they were needed by `TestWebSuite`.
Export some names in the `lib/auth` package so that SSO auth plugins can be implemented from outside this package: * struct `ssoRequestParams` (including fields) * struct `ssoCallbackResponse` (including fields) * fields of struct `TestWebSuite` * func `parseSSORequestParams` * func `ssoSetWebSessionAndRedirectURL` * func `redirectURLWithError` * var `ssoLoginConsoleErr` * func `addCSRFCookieToReq` (for TestSAML) * method `TestWebSuite.clientNoRedirects` (for TestSAML) * type `CachedSessionLingeringThreshold` (for TestSAML)
Add the ability to configure TestWebSuite so it can be used outside of
the lib/web package. This currently includes:
* Specifying the assets directory to serve, instead of the hard-coded
default of `../../webassets/teleport`, which only works for tests at
a particular level of the hierarchy.
* Specifying a plugin registry for the web.Handler so that external
plugins can set up the test suite. This is needed to move the
enterprise SSO connectors to the enterprise repo.
| @@ -0,0 +1,750 @@ | |||
| package web | |||
There was a problem hiding this comment.
Unsure what others will think - but I'm quite uncomfortable with test helpers making their way into the packages themselves. The lines quickly become blurred as to whats for the test and whats actually making this code tick in production (e.g something like AddCSRFCookieToReq). Could we do something like lib/web/test instead ?
There was a problem hiding this comment.
I am not all that comfortable with it myself. I did try to split it into a separate package, but circular dependencies on creating the web.Handler (from the web.Config) made that quite difficult (package web would need to use this new webfixture package, and it would need to use web to stand up the fixture). It might be possible to abstract that through an interface, but the Config struct is pretty unwieldy in its breadth.
I only went this route when I noted that lib/auth/helper.go contained TestAuthServer and similar.
There was a problem hiding this comment.
And sorry, I meant to note that in the PR description.
There was a problem hiding this comment.
One (imperfect) solution is to hide the testing utilities behind a build tag, which would only be used for testing. We may also have luck with the internal directory (https://go.dev/doc/go1.4#internalpackages), but I'm not sure we can use it in our case.
I also wonder if we may simply copy the code, either manually or with go:generate magic.
|
The |
r0mant
left a comment
r0mant
left a comment
There was a problem hiding this comment.
@camscale What parts of the web suite do we need to test SAML/OIDC after they move to teleport.e? Do we need to expose the full web suite?
Looking at this test here it just needs a basic setup like auth + web handler: https://github.com/gravitational/teleport.e/pull/562/files#diff-c21568611c884c7dc907ee38c89cd62d92d99b2ee44182a73f12e1984a3cdbdf.
Can enterprise web package just have a more lightweight version of the web suite that would allow us to test what's needed for SAML/OIDC handlers? Then we wouldn't need to expose it here.
|
@strideynet @Tener @zmb3 @r0mant Thanks for your comments. Rolled-up response here:
We likely don't need the whole web suite - that was my first approach, trying to stand up a fake piecemeal but I very quickly got lost as a newbie to the code base and didn't get anything to work. Having been around this multiple times now, I may have better success trying that approach again.
That was approach # 2, which Roman preferred not be done (and I agree - i felt dirty cutting and pasting that code)
That was part of attempt # 1 but I'm too new to the code base to understand why things were done the way they were and what the new approach would look like. I think this is the right idea, but I think it needs a bit more experience with the code base to do it well.
That would work, but I think it might be too annoying - it would require you run I will have another crack at simplifying the requirements for |
FWIW I think that just cut/paste our entire OSS web suite is indeed what we don't want to do, however I don't see anything wrong with just building a smaller web suite in enterprise that borrows the parts it needs from the OSS one. We do it in many places in Teleport, a lot of packages have their own test suites that set up services that they need, and most of the time it's similar since they all need auth, web, etc.
This approach sounds good to me. |
|
Clsoed in favour of #17530 |
5 participants
Reorganise the test code in lib/web to make parts available to tests running elsewhere, in particular the enterprise repository for components that are only available in the enterprise edition.
Three changes are:
webSuiteasTestWebSuiteand move into thelib/webpackage proper, rather than only being in the_test.gofiles. This allows other packages to use this test setup code.lib/webpackage so code and tests can be moved outside of this package. This is just enough to get the OIDC/SAML connectors moved, not a comprehensive look at what would need to be exported in general.NewTestWebSuiteso parts of the test suite can be configured appropriately for external use.This is the first of a series of PRs to move the enterprise connectors to the enterprise repository.
Issues: https://github.com/gravitational/teleport.e/issues/525
Note: The commits are broken up into reviewable pieces (as much as this renaming and code movement are reviewable). Reviewing the individual commits may be easier than reviewing the whole lot at once.