New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1777137: add observation of idp config and validation of its cm/secrets #222
Conversation
0313a34
to
3eb0494
Compare
/retest |
1 similar comment
/retest |
6e43291
to
dbe3514
Compare
did some very wild rebasing to make this reviewable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too serious a review as befits my lack of experience with the operator and the WIP nature of the PR. I'm surprised so many jobs passed, I guess that's an indication of a lack of test coverage?
Given the amount of repetition between the observers, maybe there's room for generic helpers that can simplify reading and writing unstructured data? I'm thinking most observers could be driven by data rather than code:
// Accepts lister and returns a mapping of canonical field names and their current values
func currentConfigFunc(configobservation.Listers) (map[string]interface{}, error)
// Example result
currentConfig := map[string]interface{}{
// . separated field names
"my.value": 0,
}
Given a function that can marshal data from a static type from the API to unstructured, maybe it's possible for generic helpers to be used to read/write from unstructured?
"github.com/openshift/cluster-authentication-operator/pkg/operator2/configobservation" | ||
) | ||
|
||
const defaultAccessTokenMaxAgeSeconds = float64(86400) // a day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the golant struct fields are int32 and the value in seconds is fixed point, is it desirable to use a floating point type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glad you asked, this was such a PITA to get right.
The config is really a json that gets deserialized into golang nested interfaces.
When we make this int32 and store as such, the information about it being int is obviously stored.
Eventually, the configObserver
, which lives in library-go, compares the config it got originally with the one that it got as observed config.
This is where all the fun happens. because the json parser actually likes to think of numbers as of floats prior to them being integers. That means that if we make this integer, same jsons will actually be considered different in their go interpretation even if our comparison here tells you the numbers are equal. That results in hotloop trying to update the observedConfig
field on same values.
I tried many things, played with the json parser flags, use json.Number
as the default representation for numbers found in the jsons we have here but nothing except this really worked.
I did a terrible job with code comments about this, I see.
pkg/operator2/configobservation/routersecret/observe_router_secret.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First bunch of answers, for some reason github defaults to comments using their review system. Meh.
21e87ef
to
7e12e9a
Compare
addressed the comments, during which I noticed default authz token max age, which is not actually being dynamically set, was missing, fixed that |
/retest |
expected: map[string]interface{}{ | ||
"oauthConfig": map[string]interface{}{ | ||
"tokenConfig": map[string]interface{}{ | ||
"accessTokenMaxAgeSeconds": float64(86400), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(No action required) Maybe this is just my ignorance of accepted practice for cluster operators, but I find it strange to see magic numbers rather than constants.
errors: []error{}, | ||
}, | ||
{ | ||
name: "inactivity 0 means disabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused - how is this different from "max age 0 still means default max age"
?
@stlaz Thank you for cleanup! Other than the seemingly duplicated test, lgtm. Please squash for merge. |
@stlaz: This pull request references Bugzilla bug 1777137, which is valid. 3 validation(s) were run on this bug
In 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 kubernetes/test-infra repository. |
1 similar comment
@stlaz: This pull request references Bugzilla bug 1777137, which is valid. 3 validation(s) were run on this bug
In 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 kubernetes/test-infra repository. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marun, stlaz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
1 similar comment
/retest |
/retest |
@stlaz: All pull requests linked via external trackers have merged: openshift/cluster-authentication-operator#222. Bugzilla bug 1777137 has been moved to the MODIFIED state. In 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 kubernetes/test-infra repository. |
currently probably quite fragile, many FIXMEs and TODOs
The previous resource sync for the resources from the oauth config was replaces by syncing it from the configobserver and it seems to work, just as well as does the slightly reworked logic about how mounts are placed in the pods