Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

ignore image_pull.go SA1019 tlsConfig.BuildNameToCertificate is deprecated#1516

Closed
mikebrow wants to merge 1 commit intocontainerd:masterfrom
mikebrow:remove-deprecation-static-check
Closed

ignore image_pull.go SA1019 tlsConfig.BuildNameToCertificate is deprecated#1516
mikebrow wants to merge 1 commit intocontainerd:masterfrom
mikebrow:remove-deprecation-static-check

Conversation

@mikebrow
Copy link
Copy Markdown
Member

Resolves:

Got an error that seems unrelated to my changes:

I0624 07:55:20.688] pkg/server/image_pull.go:287:3: SA1019: tlsConfig.BuildNameToCertificate is deprecated: NameToCertificate only allows associating a single certificate with a given name. Leave that field nil to let the library select the first compatible chain from Certificates.  (staticcheck)
I0624 07:55:20.688] 		tlsConfig.BuildNameToCertificate()
I0624 07:55:20.688] 		^

let's see... BuildNameToCertificate seems to have been deprecated in go 1.14.. and must've been cherry picked in the linter we are using so..

Signed-off-by: Mike Brown brownwm@us.ibm.com

…cated

Signed-off-by: Mike Brown <brownwm@us.ibm.com>
@mikebrow
Copy link
Copy Markdown
Member Author

/test pull-cri-containerd-node-e2e

@thaJeztah
Copy link
Copy Markdown
Member

Curious; Is there an alternative that would work without ignoring?

@k8s-ci-robot
Copy link
Copy Markdown

@mikebrow: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cri-containerd-node-e2e a128d10 link /test pull-cri-containerd-node-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

@mikebrow
Copy link
Copy Markdown
Member Author

mikebrow commented Jun 24, 2020

Curious; Is there an alternative that would work without ignoring?

Nothing that I found. Field NameToCertificate set by BuildNameToCertificate() was deprecated without replacement.

// NameToCertificate maps from a certificate name to an element of
// Certificates. Note that a certificate name can be of the form
// '*.example.com' and so doesn't have to be a domain name as such.
//
// Deprecated: NameToCertificate only allows associating a single
// certificate with a given name. Leave this field nil to let the library
// select the first compatible chain from Certificates.
NameToCertificate map[string]*Certificate

IOW it only remembers one name to certificate in the NameToCertificate map.. if you have two.. you get the last one..

seems to be saying... don't call Build.. leave the map nil... and as long as you don't use the map.. the library will do what is needed to pick certificate from certificates...

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 24, 2020 via email

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@mikebrow
Copy link
Copy Markdown
Member Author

ugh. I hate deprecated without clear replacement. not at my keyboard, but will have a look if the commit history provides more context

yeah have looked through the code.. now. If you don't set NameToCertificate... the library will just loop through all certificates looking for a match, vs doing the match based on the one you set in BuildNameToCertificate()

Not sure that is better or worse and would be a behavior change.

@mikebrow
Copy link
Copy Markdown
Member Author

Not sure what broke the k8s e2e bucket.. sigh :-)

@mikebrow
Copy link
Copy Markdown
Member Author

ok fix for node e2e bucket here:
#1517

@mikebrow
Copy link
Copy Markdown
Member Author

mikebrow commented Jun 24, 2020

will just merge this over to the e2e bucket fix yeah I know could've done that here... long day

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants