-
Notifications
You must be signed in to change notification settings - Fork 40k
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
[Prepare for go1.20] vclib: Update the unwrap check for UnknownAuthorityError #114763
[Prepare for go1.20] vclib: Update the unwrap check for UnknownAuthorityError #114763
Conversation
This commit accounts for the introduction of CertificateVerificationError in go1.20 and how govmomi handles it. This commit also gets rid of the check for the text of the x509.CertificateVerificationError error since we already check if the erorr is that type. Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
Skipping CI for Draft Pull Request. |
@MadhavJivrajani: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
if msg := x509Err.Error(); msg != "x509: certificate signed by unknown authority" { | ||
t.Fatalf("Expected 'signed by unknown authority' error, got: '%s'", msg) | ||
_, ok = verificationErr.Err.(x509.UnknownAuthorityError) | ||
if !ok { | ||
t.Fatalf("Expected to receive a wrapped x509.UnknownAuthorityError, got: '%s' (%#v)", verificationErr.Err.Error(), verificationErr.Err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit gets rid of the check for the text of the x509.CertificateVerificationError error since we already check if the erorr is that type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be written in a way that works with both go1.20 and go1.19... would something along these lines work with both?
var x509Err x509.UnknownAuthorityError
if !errors.As(urlErr.Err, &x509Err) {
fatal
}
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's a great idea
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MadhavJivrajani The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/close |
@MadhavJivrajani: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This commit accounts for the introduction of CertificateVerificationError in go1.20 and how govmomi handles it.
This commit also gets rid of the check for the text of the x509.CertificateVerificationError error since we already check if the erorr is that type.
Which issue(s) this PR fixes:
Special notes for your reviewer:
To be merged only after Go has been bumped to 1.20
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: