-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Sync changes from OIDC repo, add field in policy #2654
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2654 +/- ##
==========================================
+ Coverage 53.43% 53.49% +0.06%
==========================================
Files 52 52
Lines 14696 14744 +48
==========================================
+ Hits 7853 7888 +35
- Misses 6581 6594 +13
Partials 262 262
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
65b96e2
to
96ac0d1
Compare
96ac0d1
to
e4d4519
Compare
|``redirectURI`` | Allows overriding the default redirect URI. The default is ``/_codexch``. | ``string`` | No | | ||
|``zoneSyncLeeway`` | Specifies the maximum timeout for synchronizing ID tokens between multiple Ingress Controllers. The default is ``0s``. | ``string`` | No | |
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.
There is a mismatch here. The documentation says the type is string
, but the golang type is ZoneSyncLeeway *int
Additionally, the doc says 0s
is the default, but this is an illegal value for an int.
I suggest using string
as a golang type, as most of the other time-related fields use it (please see timeout-related fields in https://docs.nginx.com/nginx-ingress-controller/configuration/virtualserver-and-virtualserverroute-resources/ ). This way the new field will be consistent with the existing fields. For example:
zoneSyncLeeway: 500ms
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 made a mistake in the doc here. The value is not time in ms
like in the nginx config, it's an integer that is passed to the javascript function to calculate the time.
|``redirectURI`` | Allows overriding the default redirect URI. The default is ``/_codexch``. | ``string`` | No | | ||
|``zoneSyncLeeway`` | Specifies the maximum timeout for synchronizing ID tokens between multiple Ingress Controllers. The default is ``0s``. | ``string`` | No | |
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.
From the documentation it is not clear why this field is needed, what problem it solves? Could we add that? For example, the doc could answer the following questions:
- when it is necessary to enable this field (for example, when what errors/conditions clients see or admins see an error in the logs)
- what values to use, how to pick the value
- what the downsides of enabling that field
Additionally, based on the description here nginxinc/nginx-openid-connect@bd4e9cb#diff-512d27cf176962c2b1cffd370bbd1adba80368558ad4aaa2a6255eea9ff453bdR47-R53 , would it make sense to make the default non-zero? Because with the Ingress Controller, it is usually at least 2 replicas of it running in a cluster with a load balancer in front them.
Syncs changes from https://github.com/nginxinc/nginx-openid-connect fixing a couple of bugs.