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

PeerAuthentication Graduation to v1 #3112

Merged
merged 9 commits into from
Mar 13, 2024

Conversation

whitneygriffith
Copy link
Contributor

As discussed in Release Channels RFC, v1beta1 security APIs can be promoted to v1.

@istio-testing
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 6, 2024
@whitneygriffith whitneygriffith changed the title Bump PeerAuthentication to v1 PeerAuthentication Graduation to v1 Mar 6, 2024
@whitneygriffith whitneygriffith marked this pull request as ready for review March 6, 2024 17:57
@whitneygriffith whitneygriffith requested a review from a team as a code owner March 6, 2024 17:57
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 6, 2024
@hzxuzhonghu
Copy link
Member

Just PA not requestAuthentication or authorizationPolicy?

@costinm
Copy link
Contributor

costinm commented Mar 7, 2024 via email

@linsun
Copy link
Member

linsun commented Mar 7, 2024

Just PA not requestAuthentication or authorizationPolicy?

I think some of the security APIs are already in v1.

I am good with this change.

I think the only positive thing about promoting this API to v1 is that it
sends the signal that the istio v1 API surface
is a dead end and not serious - and may motivate users to move to Gateway
API faster...

Not sure if this would signal users that. I don't think gateway API has Istio's security API replacement?

@linsun
Copy link
Member

linsun commented Mar 7, 2024

@whitneygriffith could you add a release note?

@costinm
Copy link
Contributor

costinm commented Mar 7, 2024 via email

@keithmattix
Copy link
Contributor

keithmattix commented Mar 8, 2024

It is rarely used, its original purpose ( to help migrate to 'strict' )
never actually worked, as proven by the vast number of users
who are still on 'permissive' ( which effectively means insecure ).

At least on the Azure side, we see a decent percentage of use. 10% of users who deploy a VirtualService also deploy a PeerAuthentication, and IMO that's a non-negligible segment/trend of adoption.

We have ample data that the API is bad - we know ambient can't really
support it and has a huge cost in
complexity and generated config.

Ambient already supports 100% of the PeerAuthentication API surface 😃

@whitneygriffith
Copy link
Contributor Author

/test release-notes

@whitneygriffith
Copy link
Contributor Author

whitneygriffith commented Mar 8, 2024

So far none of the Istio APIs changed from alpha to v1 - we seem to be the only project in the world that managed to get perfect APIs from start without any need to use the user feedback, and with minimal review and low bar in alpha.

I am not aware of the context around promoting PeerAuthentication to v1beta1, but since it is v1beta1 which users understand to be relatively stable, we do not want to revert its status to experimental. And as we discussed, there is no plans to improve this further especially as the future is still unclear. Keep in mind we can still deprecate v1 APIs with 1 year of advanced notice and a supported upgrade path to a replacement feature.

I'm very curious how the process will work for the new 'experimental' vs 'stable' track - if PeerAuthentication is promoted
to v1 as-is, what will be the bar there ? Why even bother with 2 tracks if
everything just gets promoted to v1 anyways ?

The release channels strategy is to get to a place where we have two API versions v1alpha1 and v1, and because we have existing v1beta1 APIs, we concluded we can graduate them to v1 as stated here which reflects a semantic change not an increase in stability/refined use case as the effort to do the latter is not worth it at the moment.

Moving forward the bar, APIs will only be graduated to v1 based on the existing requirements for Stable features as well as what's being proposed here.

@whitneygriffith
Copy link
Contributor Author

Just PA not requestAuthentication or authorizationPolicy?

Yes, all of the other Security APIs are v1.

@costinm
Copy link
Contributor

costinm commented Mar 9, 2024 via email

@louiscryan
Copy link
Contributor

Since it is v1beta1 Im fine with promoting.

I think we should update the API docs to indicate that the API is also frozen and likely to be replaced in ambient

@howardjohn
Copy link
Member

LGTM. As have Louis and Lin.

That leaves @nrjpoddar @ericvn @therealmitchconnors - PTAL

I don't know we need 6/6 signoff, but given the importance of this change would be good to get as much visibility as possible

Copy link

@ericvn ericvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this as well. One minor nit in the release notes.

releasenotes/notes/promote-peer-auth-v1.yaml Outdated Show resolved Hide resolved
@nrjpoddar
Copy link
Member

We see our customers frequently set mTLS STRICT in the PeerAuth at the mesh level to ensure all traffic is guaranteed mTLS. I don't see a reason why we can't promote this as it's been stable and used by customers.

@whitneygriffith
Copy link
Contributor Author

/retest

Signed-off-by: whitneygriffith <whitney.griffith16@gmail.com>
Signed-off-by: whitneygriffith <whitney.griffith16@gmail.com>
Signed-off-by: whitneygriffith <whitney.griffith16@gmail.com>
Signed-off-by: whitneygriffith <whitney.griffith16@gmail.com>
Signed-off-by: whitneygriffith <whitney.griffith16@gmail.com>
Signed-off-by: whitneygriffith <whitney.griffith16@gmail.com>
Signed-off-by: whitneygriffith <whitney.griffith16@gmail.com>
Signed-off-by: whitneygriffith <whitney.griffith16@gmail.com>
Signed-off-by: whitneygriffith <whitney.griffith16@gmail.com>
@istio-testing istio-testing merged commit 339eb52 into istio:master Mar 13, 2024
5 checks passed
@whitneygriffith whitneygriffith deleted the security-apis branch March 13, 2024 23:36
@whitneygriffith
Copy link
Contributor Author

Part of 173

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.