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

[advancedTLS] Removed deprecated APIs in advancedTLS #7303

Merged
merged 11 commits into from
Jun 6, 2024

Conversation

gtcooke94
Copy link
Contributor

@gtcooke94 gtcooke94 commented Jun 5, 2024

This PR removes the following deprecated APIs in preparation for a 1.0 release of advancedTLS:
The first list were simply renamed

RootCertificateOptions.RootCACerts
GetRootCAsParams
GetRootCAsResults
ClientOptions
ServerOptions
Options.MinVersion
Options.MaxVersion
RevocationConfig
Options.RevocationConfig
AllowUndetermined
CustomVerificationFunc
Options.VType
Options.VerifyPeer

The second list are fully deprecated features in the CRL stack - use CRL providers instead. For reading a directory, the FileWatcherCRLProvider should be a relatively simple replacement.

Cache
RevocationOptions.RootDir
RevocationOptions.Cache

RELEASE NOTES: None

@gtcooke94 gtcooke94 added Type: API Change Breaking API changes (experimental APIs only!) Type: Internal Cleanup Refactors, etc labels Jun 5, 2024
Copy link
Contributor

@matthewstevenson88 matthewstevenson88 left a comment

Choose a reason for hiding this comment

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

Please fix the failing tests, otherwise LGTM.

@@ -319,7 +183,7 @@ func fetchCRL(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revocat
}
return crl, nil
}
return fetchIssuerCRL(c.RawIssuer, crlVerifyCrt, cfg)
return nil, fmt.Errorf("trying to fetch CRL but CRLProvider is nil")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this check to become the fist thing in this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, changed

@matthewstevenson88 matthewstevenson88 added this to the 1.65 Release milestone Jun 5, 2024
@erm-g erm-g self-requested a review June 6, 2024 04:12
Copy link
Contributor

@erm-g erm-g left a comment

Choose a reason for hiding this comment

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

LGTM

@arvindbr8
Copy link
Member

Note: the rel notes in the PR description is only for grpc-Go releases. We would need to come up with rel notes for the advancedTLS release.

Also since the fields were deprecated before the 1.0 release, i would rather not mention anything in the rel notes for 1.0

@gtcooke94
Copy link
Contributor Author

That's a good point - I removed release notes

Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

LGTM.

As for 1.0 release, I think we should just audit everything once again. For eg: I see we have a separate module under security/advancedtls/examples/ which has a replace directive to the advancedTLS module. So after release, we should also update the go.mod to reference to the right release

@arvindbr8 arvindbr8 removed the Type: Internal Cleanup Refactors, etc label Jun 6, 2024
@arvindbr8 arvindbr8 assigned gtcooke94 and unassigned arvindbr8 and dfawley Jun 6, 2024
@gtcooke94 gtcooke94 merged commit dbd24a9 into grpc:master Jun 6, 2024
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: API Change Breaking API changes (experimental APIs only!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants