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

chore: fix master and v3 branches #54

Merged
merged 9 commits into from
Feb 8, 2022
Merged

chore: fix master and v3 branches #54

merged 9 commits into from
Feb 8, 2022

Conversation

dkoshkin
Copy link

@dkoshkin dkoshkin commented Feb 3, 2022

Fixes https://jira.d2iq.com/browse/COPS-7145

https://github.com/mesosphere/traefik-forward-auth/releases/tag/v3.0.0 was released with #41 feature. But the v3 branch was cut before that work went in. Then there was a big refactor that was done in 7e8ab37 and merged to v3 branch but the PR to master was never merged in. Then there were some more work that went into v3 and most of it did not make to master.

This PR is to get master back to expected state that includes both Jared's clusterstorage feature and the changes from the v3 branch. After this is merged we can create a new release/v3 branch from this tip that can then be used for v3 development.


Jason and I cherry-picked each commit from v3. The first 4 had the most merge conflicts.
When reviewing pay special attention to 39539aa as that one made some additional changes from the cherry-pick commit. The original commit added a sessionCookie which is no longer needed with userinfo introduced in the clusterstorage PR.

image

Mario Hros and others added 9 commits February 2, 2022 15:40
…types added

Original wildcard prefix match now allows multiple * characters.

For increased safety, a single '*' character now matches
within one path segment only. To match any number of path segments,
two consecutive charaters must be specified '**'.

In addition to original "prefix match", full URL match is possible:

1. Wildcard Prefix Match
   `/admin/*` matches /admin/overview, /admin/users, *not* /admin/users/1
   `/admin/**` matches what '/admin/*' matched + also '/admin/users/1'

2. Full URL Wildcard Match
   `*://a.com/admin` matches http://a.com/admin and https://a.com/admin
   `*://a.com/**` matches everything under http://a.com/ and https://a.com/
   `https://b.com/admin` matches https://b.com/admin only

3. Full URL Regular Expression Match (prefixed by ~ character!)
   `~^https?://[cd].com/.*` matches everything under
     http://c.com/, http://d.com/ and their https versions

Tests were extended for new functionality and updated for the fact that
single '*' now matches within the one path component only.

cherry-pick 36c3eee
`makeSessionCookie` turns its err value into a nil, losing its info.

Also make fatal problems use Error and non-fatal problems use Warn.

cherry-pick 3e4e6f9
(cherry picked from commit 8334159)
@dkoshkin
Copy link
Author

dkoshkin commented Feb 3, 2022

Shoutout to @jkoelker for the work here 👏 !

@dkoshkin dkoshkin requested review from jkoelker and joejulian and removed request for jkoelker and joejulian February 3, 2022 17:41
@jkoelker
Copy link

jkoelker commented Feb 4, 2022

LGTM, I'll refrain from approving, since I helped with the code side, but I went through the remaining commits and it looks good to me.

@dkoshkin
Copy link
Author

dkoshkin commented Feb 7, 2022

Using https://github.com/mesosphere/kubernetes-base-addons/tree/dkoshkin-tfa-clusterstorage

Tested manually by setting custom values on the addon:

        - name: traefik-forward-auth
          enabled: true
          values: |
            clusterStorage:
              enabled: true
              namespace: kubeaddons

Verified the login still worked and saw this:

➜  dkkonvoy-tfa-cops k view-secret -n kubeaddons tfa-claims-nlmrm
Choosing key: userInfo
{"username":"hungry_banach","email":"hungry_banach","groups":[]}

image

@dkoshkin
Copy link
Author

dkoshkin commented Feb 7, 2022

Also ran Konvoy e2e tests (specifically should login with ...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants