Add keycloak versions to circleci test job #294

hawknewton · 2020-05-22T18:40:39Z

This pull request makes use of the marix jobs feature to add additional Keycloak versions to the CircleCI acceptance test run. Additionally, I tacked on the circleci test reporter to make it easier to see failures.

As you can see there are test failures in all versions except 8.0.1, 8.0.0, 7.0.1, and 7.0.0.

~~Unfortunately there's no pre-built docker image for 4.8.3 so I pulled it.~~ I've re-added 4.8.3 but it's also failing, unfortunately.

tomrutsaert · 2020-05-22T19:58:03Z

Nice job

I guess we have to go through the failing tests and see why they are failing, if needed ignore them for that version.
For versions 9 en 10, I see issues concerning federated link on a user and google idp, I will take a look at those next week.

.circleci/config.yml

tomrutsaert · 2020-05-27T14:51:26Z

@mrparkers Could you take a look at TestAccKeycloakProvider_passwordGrant
It started failing starting from keycloak version 9, And I have no idea why, I also do not really understand the test and what it is doing. For example Why is the skipTestIfEnv part there?

In the test you get a 403 forbidden after a GET for an newly created Realm.
Starting from keycloak 9 the next call you get a 401, while before it seem to recover on it is own

2020/05/27 16:47:43 [DEBUG] Response: 403 Forbidden.  Attempting refresh
2020/05/27 16:47:43 [DEBUG] Refresh request: client_id=terraform&grant_type=password&password=password&username=keycloak
2020/05/27 16:47:43 [DEBUG] Refresh response: {"error":"unauthorized_client","error_description":"Client secret not provided in request"}
2020/05/27 16:47:43 [DEBUG] Response: 401 Unauthorized
2020/05/27 16:47:43 [DEBUG] Response body: {"error":"HTTP 401 Unauthorized"}
2020/05/27 16:47:43 [DEBUG] keycloak_realm.realm: apply errored, but we're indicating that via the Error pointer rather than returning it: error sending GET request to /auth/admin/realms/terraform-rrvkdmttdx: 401 Unauthorized
2020/05/27 16:47:43 [ERROR] <root>: eval: *terraform.EvalApplyPost, err: error sending GET request to /auth/admin/realms/terraform-rrvkdmttdx: 401 Unauthorized
2020/05/27 16:47:43 [ERROR] <root>: eval: *terraform.EvalSequence, err: error sending GET request to /auth/admin/realms/terraform-rrvkdmttdx: 401 Unauthorized
2020/05/27 16:47:43 [DEBUG] New state was assigned lineage "e995df85-d30e-5743-48c4-1f032d511eab"
    TestAccKeycloakProvider_passwordGrant: testing.go:640: Step 0 error: errors during apply:
        
        Error: error sending GET request to /auth/admin/realms/terraform-rrvkdmttdx: 401 Unauthorized
        
          on /tmp/tf-test367076349/main.tf line 2:
          (source code not available)

mrparkers · 2020-05-27T15:04:43Z

That test makes sure that the provider is correctly able to handle the password grant when authenticating to keycloak. I think the reason it depends on that environment variable was because we originally ran the entire suite of tests using the password grant in CI, but I ended up removing that and never removed the need for that environment variable. This test can probably run every time now.

I'm not sure why it's failing for the latest Keycloak versions. I'll run it locally and take a look.

mrparkers · 2020-05-27T15:35:20Z

I pushed a fix that should fix the failing tests relating to the password grant for Keycloak >= 9. I suppose they made a change that requires that you specify the client secret during the password grant for confidential clients (which I think we should have been doing anyways, but it hasn't been caught until now).

Regarding the PR, I think this is an awesome change since we can never have too many tests, but this will probably make me run out of my weekly allocation of free CircleCI credits. Would you mind changing this to only test against the latest released version for every major release? So instead of testing 8.0.0, 8.0.1, and 8.0.2, let's change it to only test 8.0.2.

Right now I'm working on running a different CI server (Drone) on my own hardware, so we'll be able to test as many Keycloak versions as we want.

hawknewton · 2020-05-27T17:11:57Z

Totally makes sense to me.

What do you think about testing against all versions for the release job? You get the best of both worlds, we maintain good coverage during development and ensure no regression bugs sneak in during a release.

tomrutsaert · 2020-05-27T17:20:25Z

That seems like a good idea to me.

In the mean time we everything green from keycloak version 7 and up.

I took a look at only error on version 6, and i can not reproduce it locally, the number of expected events is 63 instead of 67 on CI ....

On version 4 and 5 the same 3 tests are failing. Perhaps they are new features not supported on those versions. I might check those later.

But the question that pops to my mind, do we want to support those older version?

hawknewton · 2020-05-27T17:36:42Z

@tomrutsaert Great question. Given that 4.8.3 came out in January of last year I don't think it's asking too much to support it. Once it becomes cumbersome (and perhaps it already has) we should think about dropping support, but do so with cause.

@mrparkers Just a thought, but if CircleCI credits are a problem maybe we just run circle with a few (maybe just the latest?) version(s) during development and save full-regression for the release process. The current pipeline appears to only run once the release tag has been pushed which puts us in the unfortunate position of needing to change the tag in the event a regression bug does sneak in.

We may want to consider changing the release strategy slightly, perhaps add a circle config that matches the pattern release-* (for example). You'd merge to a release-1.2.3 branch while preparing to cut a release and once everything is green push to master and create the tag.

That being said, there are really good reasons to stick with a very simple branching strategy and dancing around Circle credits may not be worth the added complexity.

mrparkers · 2020-05-27T17:38:57Z

What do you think about testing against all versions for the release job?

@hawknewton I don't necessarily have a problem with that and I don't think I'll run out of free credits by doing that. But I also don't think we have a lot to gain by testing against different patch releases. So you can add all versions back to the release job if you'd like.

I might change this up a bit once I move to the new CI server though. But since it'll be running on my own personal hardware, we can test against all versions if we wish.

But the question that pops to my mind, do we want to support those older version?

@tomrutsaert that's a good question. I think it's reasonable to officially support the latest three major versions, so in this case, 8.x, 9.x, and 10.x. That's not to say that we won't try to fix bugs if they exist on older versions, but I don't think we'll go out of our way to support them since that seems like a bit too much of a burden on an open source project.

Of course, PRs are always welcome, and I would never reject a PR that adds additional support for older versions unless it actively breaks other functionality.

mrparkers · 2020-05-27T17:42:45Z

@hawknewton yeah, the current release process wasn't designed to accommodate for this type of regression testing, which was intentional since I wanted to stay on the free tier.

Again, this shouldn't be a problem in the future as I switch CI solutions so I don't have to worry about going over a free tier. I'd probably let full regression testing happen on all pushes to master to keep the release pipeline simple though.

mrparkers · 2020-05-27T17:43:12Z

For now, why don't we just stick with the latest patch release all the way down to 4.8.3.Final as a compromise.

hawknewton · 2020-05-27T18:25:56Z

I took a look at only error on version 6, and i can not reproduce it locally, the number of expected events is 63 instead of 67 on CI ....

@tomrutsaert Looks like a legit incompatibility.

I'm not sure how you want to manage this. We could detect server version at runtime but it seems like a weird thing to do just for the tests.

mrparkers · 2020-05-27T18:58:15Z

Eventually our tests will have to make different assertions based on the running version. This won't be the only case - for example, the 4.8.3.Final tests are failing due to the absence of the builtin microprofile-jwt scope that exists from 5.x onward.

I don't think the provider itself should make the distinction between different versions (although who knows, that might eventually be needed), but at the very least the tests should make different assertions against different versions as needed.

hawknewton · 2020-06-01T19:20:04Z

resource_keycloak_openid_client_optional_scopes_test.go:18: /serverInfo endpoint retuned an unreadable version, server Keycloak version could not be determined: Malformed version: 4.8.3.Final

🤣 😭

tomrutsaert · 2020-06-01T19:29:17Z

:-/ well it is not wrong, it is a strange version number.....
Perhaps we should drop 4.8.3.Final
(and temp add 8.0.1 to be able to merge)
@mrparkers what do you think?

hawknewton · 2020-06-01T19:31:54Z

mrparkers · 2020-06-01T19:42:47Z

provider/test_utils.go

@@ -86,7 +87,11 @@ func keycloakVersionIsGreaterThanOrEqualTo(keycloakClient *keycloak.KeycloakClie
 		if err != nil {
 			return false, fmt.Errorf("/serverInfo endpoint retuned an error, server Keycloak version could not be determined: %s", err)
 		}
-		keycloakServerInfoVersion, err = version.NewVersion(serverInfo.SystemInfo.ServerVersion)
+
+		regex := regexp.MustCompile(`^(\d+\.\d+\.\d+)`)


my only suggestion here is that go-version does export their regular expressions that we can use instead: https://github.com/hashicorp/go-version/blob/master/version.go#L20-L31

my only suggestion here is that go-version does export their regular expressions that we can use instead: https://github.com/hashicorp/go-version/blob/master/version.go#L20-L31

regex := `v?([0-9]+(\.[0-9]+)*?)` + `(-([0-9]+[0-9A-Za-z\-~]*(\.[0-9A-Za-z\-~]+)*)|(-([A-Za-z\-~]+[0-9A-Za-z\-~]*(\.[0-9A-Za-z\-~]+)*)))?` + `(\+([0-9A-Za-z\-~]+(\.[0-9A-Za-z\-~]+)*))?` + `?` r := regexp.MustCompile(regex) fmt.Printf("%#v\n", r.FindStringSubmatch(`4.4.3.Final`))

[]string{"4", "4", "", "", "", "", "", "", "", "", "", ""}

No bueno, unfortunately.

ah, nevermind then. I think what you have is good enough, especially since this is only used for tests.

hawknewton · 2020-06-01T20:10:55Z

Looks like we're good, the required 8.0.1 result was superseded by 8.0.2.

mrparkers · 2020-06-01T20:18:48Z

.circleci/config.yml

+          matrix:
+            parameters:
+              keycloak-version:
+                # 4.8.3 doesn't have a docker image


last suggestion: can you remove this comment?

mrparkers

looks good! thanks for the PR!

hawknewton added 2 commits May 22, 2020 11:30

Add keycloak versions to acceptance testing

11cabbf

Remove 4.8.3 from acceptance testing

c297c93

tomrutsaert reviewed May 22, 2020

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

Add 4.8.3.Final to circle

8547787

This was referenced May 26, 2020

Fixes for tests that are failing on keycloak version >= 9 #300

Merged

starting from keycloak 8.0.2: rootUrl and BaseUrl should be real urls #302

Merged

URL encode role name. #213

Merged

Merge branch 'master' into add-keycloak-versions

16a5a95

fix missing client secret for password grant refresh

94ed8ec

Merge branch 'master' into add-keycloak-versions

6c8ce3b

Remove outdated versions from Circle run

139a271

tomrutsaert mentioned this pull request May 28, 2020

Introduction of testUtil function keycloakVersionIsHigherOrEqualThan + plus fix tests #306

Merged

hawknewton and others added 3 commits May 28, 2020 09:00

Merge branch 'master' into add-keycloak-versions

b5c3d8a

Merge branch 'master' into add-keycloak-versions

7215282

Merge branch 'master' into add-keycloak-versions

1925f08

Truncate non-semver server versions

52e3c9c

mrparkers reviewed Jun 1, 2020

View reviewed changes

Remove comment

b7c239f

mrparkers approved these changes Jun 2, 2020

View reviewed changes

mrparkers merged commit 0f9368e into keycloak:master Jun 2, 2020

hawknewton deleted the add-keycloak-versions branch June 12, 2020 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add keycloak versions to circleci test job #294

Add keycloak versions to circleci test job #294

hawknewton commented May 22, 2020 •

edited

Loading

tomrutsaert commented May 22, 2020

tomrutsaert commented May 27, 2020

mrparkers commented May 27, 2020

mrparkers commented May 27, 2020

hawknewton commented May 27, 2020

tomrutsaert commented May 27, 2020

hawknewton commented May 27, 2020

mrparkers commented May 27, 2020

mrparkers commented May 27, 2020

mrparkers commented May 27, 2020

hawknewton commented May 27, 2020 •

edited

Loading

mrparkers commented May 27, 2020

hawknewton commented Jun 1, 2020

tomrutsaert commented Jun 1, 2020

hawknewton commented Jun 1, 2020

mrparkers Jun 1, 2020

hawknewton Jun 1, 2020

mrparkers Jun 1, 2020

hawknewton commented Jun 1, 2020 •

edited

Loading

mrparkers Jun 1, 2020

mrparkers left a comment

Add keycloak versions to circleci test job #294

Add keycloak versions to circleci test job #294

Conversation

hawknewton commented May 22, 2020 • edited Loading

tomrutsaert commented May 22, 2020

tomrutsaert commented May 27, 2020

mrparkers commented May 27, 2020

mrparkers commented May 27, 2020

hawknewton commented May 27, 2020

tomrutsaert commented May 27, 2020

hawknewton commented May 27, 2020

mrparkers commented May 27, 2020

mrparkers commented May 27, 2020

mrparkers commented May 27, 2020

hawknewton commented May 27, 2020 • edited Loading

mrparkers commented May 27, 2020

hawknewton commented Jun 1, 2020

tomrutsaert commented Jun 1, 2020

hawknewton commented Jun 1, 2020

mrparkers Jun 1, 2020

Choose a reason for hiding this comment

hawknewton Jun 1, 2020

Choose a reason for hiding this comment

mrparkers Jun 1, 2020

Choose a reason for hiding this comment

hawknewton commented Jun 1, 2020 • edited Loading

mrparkers Jun 1, 2020

Choose a reason for hiding this comment

mrparkers left a comment

Choose a reason for hiding this comment

hawknewton commented May 22, 2020 •

edited

Loading

hawknewton commented May 27, 2020 •

edited

Loading

hawknewton commented Jun 1, 2020 •

edited

Loading