Skip to content
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

Merged
merged 9 commits into from
May 13, 2022
Merged

Conversation

lucacome
Copy link
Member

@lucacome lucacome commented May 5, 2022

Syncs changes from https://github.com/nginxinc/nginx-openid-connect fixing a couple of bugs.

@lucacome lucacome self-assigned this May 5, 2022
@github-actions github-actions bot added the bug An issue reporting a potential bug label May 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 5, 2022

Codecov Report

Merging #2654 (a081f09) into main (85e3c6e) will increase coverage by 0.06%.
The diff coverage is 45.51%.

@@            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              
Impacted Files Coverage Δ
cmd/nginx-ingress/main.go 6.81% <0.00%> (-0.05%) ⬇️
internal/configs/annotations.go 21.56% <0.00%> (ø)
internal/configs/config_params.go 76.74% <ø> (ø)
internal/configs/version2/http.go 0.00% <ø> (ø)
internal/k8s/spiffe.go 0.00% <0.00%> (ø)
internal/k8s/utils.go 11.76% <ø> (ø)
internal/metrics/collectors/latency.go 49.66% <ø> (ø)
internal/metrics/collectors/processes.go 0.00% <0.00%> (ø)
internal/nginx/fake_manager.go 0.00% <ø> (ø)
internal/configs/configurator.go 37.70% <10.00%> (+0.25%) ⬆️
... and 11 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@lucacome lucacome force-pushed the fix/oidc-changes branch 2 times, most recently from 65b96e2 to 96ac0d1 Compare May 12, 2022 06:55
@lucacome lucacome marked this pull request as ready for review May 12, 2022 06:55
@lucacome lucacome requested review from a team, haywoodsh, jjngx and shaun-nx May 12, 2022 06:55
@lucacome lucacome changed the title Sync changes from OIDC repo Sync changes from OIDC repo, add field in policy May 12, 2022
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label May 12, 2022
@lucacome lucacome requested review from pleshakov and a team May 12, 2022 18:42
docs/content/configuration/policy-resource.md Outdated Show resolved Hide resolved
|``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 |
Copy link
Contributor

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

Copy link
Member Author

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 |
Copy link
Contributor

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.

@lucacome lucacome merged commit d8d4d3e into main May 13, 2022
@lucacome lucacome deleted the fix/oidc-changes branch May 13, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug documentation Pull requests/issues for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants