CMP-3818: Add test to test tokenrules pass when configuring oauthclients#1058
CMP-3818: Add test to test tokenrules pass when configuring oauthclients#1058Anna-Koudelkova wants to merge 1 commit intoComplianceAsCode:masterfrom
Conversation
|
@Anna-Koudelkova: This pull request references CMP-3818 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
🤖 To deploy this PR, run the following command: |
|
Currently this does not work correctly even after multiple rescans and I am not sure if the problem is the test case itself. Posting some logs just so I do not loose it during the treasure hunt ( meaning: while chasing the fix): ok, but after I patch the value to the one expected by the variable and rescan, it is passing - is this expected? |
Anna-Koudelkova
force-pushed
the
CMP-3818
branch
2 times, most recently
from
b29f107 to
17972bc
Compare
|
🤖 To deploy this PR, run the following command: |
|
🤖 To deploy this PR, run the following command: |
Anna-Koudelkova
force-pushed
the
CMP-3818
branch
from
17972bc to
e4a63ec
Compare
|
🤖 To deploy this PR, run the following command: |
rhmdnd
left a comment
rhmdnd
left a comment
There was a problem hiding this comment.
A couple suggestions on reducing calls and a potential approach to cleaning up after the test is done, since we're dealing with a cluster-wide setting.
tests/e2e/framework/common.go
Outdated
| } | ||
|
|
||
| // Patch accessTokenMaxAgeSeconds | ||
| patch1 := []byte(fmt.Sprintf(`{"accessTokenMaxAgeSeconds":%d}`, maxAgeSeconds)) |
There was a problem hiding this comment.
We could put the "accessTokenInactivityTimeoutSeconds":%d in here, too and save on a second patch and verify call, which would cut the function length by about half.
tests/e2e/serial/main_test.go
Outdated
| } | ||
|
|
||
| // Patch all oauthclients to make them compliant | ||
| if err := f.PatchAllOAuthClients(28800, 600); err != nil { |
There was a problem hiding this comment.
Since this is patch a cluster-wide configuration option, we could do something like:
originalConfigs := getOAuthClientConfigs()
if err := f.PatchAllOAuthClients(28800, 600); err != nil {
t.Fatalf("failed to patch oauthclients: %s", err)
}
defer restoreOAuthClientConfigs(originalConfigs)The defer would make sure the original values are set back to what they were, in case those updates fail other tests for some reason. The defer function might need to poll to verify the configs are updated, but we're doing that already in the PatchAllOAuthClients function.
There was a problem hiding this comment.
Oh! Thanks a lot, defer step was definitely missing. I ended up combining the getOauthclientConfigs() with PatchAllOauthClients() into one, not really sure if that is acceptable or not, please let me know if that is not the right way to do it - I really appreciate your feedback.
|
/retest-required |
Anna-Koudelkova
force-pushed
the
CMP-3818
branch
from
e4a63ec to
0ed69d9
Compare
|
Passing with the new changes however I am unsure about smashing |
|
🤖 To deploy this PR, run the following command: |
Anna-Koudelkova
force-pushed
the
CMP-3818
branch
from
0ed69d9 to
d151d26
Compare
|
🤖 To deploy this PR, run the following command: |
rhmdnd
left a comment
rhmdnd
left a comment
There was a problem hiding this comment.
Looks great! Approving pending testing feedback.
/lgtm
/approve
tests/e2e/framework/common.go
Outdated
| if !ok { | ||
| continue | ||
| } | ||
| maxAge := int64(0) |
There was a problem hiding this comment.
If an OAuth client doesn't have maxAge or inactivity set, the config passed back to the restore function will set them to 0 instead of leaving them unset.
For example:
# before test
OAuth Config:
name: foo
# during test
OAuth Config:
name: foo
maxAge: 28800
inactivity: 600
# after restore
OAuth Config:
name: foo
maxAge: 0
inactivity: 0So the restore would set the values to 0 rather than removing them. The question is whether the OAuth server treats an unset value the same as 0.
Either way, this is probably fine given we don't test OAuth outside of this case, but wanted to mention it as a potential edge case in the restoration flow.
tests/e2e/framework/common.go
Outdated
| // GetAndPatchAllOAuthClientConfigs lists all OAuth clients once; for each client it saves the current config | ||
| // into the returned slice and then patches the client with the given values. Return the slice to | ||
| // RestoreOAuthClientConfigs when done. | ||
| func (f *Framework) GetAndPatchAllOAuthClientConfigs(maxAgeSeconds, inactivityTimeoutSeconds int64) ([]OAuthClientConfig, error) { |
There was a problem hiding this comment.
To your question about combining these. The primary reason behind my previous comment was to advocate for a single patch call for two values instead of two separate patch calls per config (which you've implemented).
I'm not opposed to what you've done here, since it saves off the original configs and patches each client into the same loop.
Alternatively, we could have a getOAuthClientConfigs and an updateOAuthClientConfigs separately, but in that case we'd be iterating the list of configs three times (getting the original values, updating the clients for the test, restoring the original values) as opposed to just twice (getting the original values and setting the test-specific values, then restoring the original values).
I could go either way - it's up to you how you'd like to structure it and whatever make sense to you organizationally.
There was a problem hiding this comment.
Ok, if the decision is up to me and there is no stylistic preference (and the cost of iterating can be disregarded), I have split it again to two functions (getOAuthClientConfigs and updateOAuthClientConfigs - that is what looks "clearer" to my brain. 🙂
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Anna-Koudelkova, rhmdnd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Clean e2e test runs across parallel and serial suites 🎉 - just needs a rebase. |
Anna-Koudelkova
force-pushed
the
CMP-3818
branch
from
d151d26 to
9d1136c
Compare
|
New changes are detected. LGTM label has been removed. |
|
🤖 To deploy this PR, run the following command: |
Anna-Koudelkova
force-pushed
the
CMP-3818
branch
from
9d1136c to
4bc8e45
Compare
|
🤖 To deploy this PR, run the following command: |
|
/retest |
|
@Anna-Koudelkova: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
In the upstream content testing we apply remediation for rules
ocp4-oauth-or-oauthclient-token-maxageandocp4-oauth-or-oauthclient-inactivity-timeoutby patching it on cluster level. Both rules should be passing if client do not set the value on a cluster level, but set the values for all oauthclients individually.This PR should add test to see if both rules are passing when the values are set per each client.
Note: I have run into some issues while trying to make this testcase work - mainly because I understood the description of the rule
ocp4-oauth-or-oauthclient-token-maxagethat for it to pass it is enough to set the values for all oauth clients - and I can set whatever value I want. Which is not true - the values set need to correspond with the value in the corresponding variableocp4-var-oauth-token-maxage. If the values from the variable and from the oauth clients mismatch, the rule would FAIL. Is this the expected behavior?