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

Prevent deprecation of AppServerOrSAMLIdPServiceProvider from being a breaking change #41047

Merged
merged 2 commits into from
May 8, 2024

Conversation

gzdunek
Copy link
Contributor

@gzdunek gzdunek commented Apr 30, 2024

Closes #39031

In #38709 the types KindAppOrSAMLIdPServiceProvider and AppServerOrSAMLIdPServiceProvider were deprecated.
Unfortunately, this was also a breaking change for v15 clients because SAML providers started to be sent as types.SAMLIdPServiceProvider instead of types.AppServerOrSAMLIdPServiceProvider.

AFAIK the only thing we need to fix is the paginated resources response.
This can be done by checking the client version and sending a compatible response.

@gzdunek gzdunek added the no-changelog Indicates that a PR does not require a changelog entry label Apr 30, 2024
@gzdunek gzdunek requested review from avatus and rosstimothy April 30, 2024 13:41
@gzdunek
Copy link
Contributor Author

gzdunek commented Apr 30, 2024

I just learned from @espadolini that we should avoid checking the client version, I will try to send the same resource twice from the server, the clients should support only the new or old resource.

@gzdunek gzdunek marked this pull request as draft April 30, 2024 14:17
@gzdunek
Copy link
Contributor Author

gzdunek commented May 8, 2024

Okay, I realized that returning the same resource twice won't work. The new types.KindSAMLIdPServiceProvider resource will be sent starting from v16 and there are places in the code that return an error when they receive an unknown resource type (for example, web/apiserver).
So, I have to stick to checking the client version, otherwise auth v16 may not work with proxy v15.

@gzdunek gzdunek marked this pull request as ready for review May 8, 2024 11:08
@github-actions github-actions bot requested review from camscale and strideynet May 8, 2024 11:09
@gzdunek gzdunek removed request for camscale and strideynet May 8, 2024 11:09
@gzdunek gzdunek added this pull request to the merge queue May 8, 2024
Merged via the queue into master with commit 117fa0f May 8, 2024
42 checks passed
@gzdunek gzdunek deleted the gzdunek/fix-saml-idp-compatiblity branch May 8, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AppServerOrSAMLIdPServiceProvider type deprecation and backport plan for Connect
3 participants