Add tls pkg#2
Conversation
joelanford
left a comment
There was a problem hiding this comment.
No need for this repo to have a vendor directory. That's only needed in repos that build images for OCP.
| // Shutdown is a function that will be called to trigger a graceful shutdown | ||
| // when the TLS profile changes. | ||
| Shutdown context.CancelFunc |
There was a problem hiding this comment.
WDYT about changing this to a general-purpose callback function that includes context about the old and new TLS profile?
There was a problem hiding this comment.
Yeah that's a good idea, so the consumers can tweak their own logging if needed.
I've pushed a change for this. PTAL
| Eventually(shutdownCnt.Load).Should(Equal(int32(1)), "shutdown count should be 1 (shutdown triggered)") | ||
| }) | ||
|
|
||
| It("should trigger a shutdown when switching to custom profile", func() { |
There was a problem hiding this comment.
What if the custom profile definition is identical to the pre-defined profile they were switching from? Seems like we may want to do a comparison of the actual TLS settings?
There was a problem hiding this comment.
Seems like we may want to do a comparison of the actual TLS settings?
That's what we do already.
I've now added a test to cover this use case and demonstrate correct behaviour.
| Eventually(shutdownCnt.Load).Should(Equal(int32(1)), "shutdown count should be 1 (shutdown triggered)") | ||
| }) | ||
|
|
||
| It("should trigger a shutdown when switching from custom to predefined profile", func() { |
There was a problem hiding this comment.
Same question as above, just in the reverse.
There was a problem hiding this comment.
That's what we do already.
I've now added a test to cover this use case and demonstrate correct behaviour.
There was a problem hiding this comment.
Seems unrelated to controller-runtime and TLS?
There was a problem hiding this comment.
It is needed to achieve the CRD loading for envtest based tests given we want to drop the vendor folder.
I had a chat with Joel about this, and we settled on looking those CRDs up from the gomod cache's folder, hence the package to do that. I can always do it inline but that's going to be copy-pasta in almost all the integration tests.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ack, makes sense. And that's a good justification for including here because envtest is so pervasive and needing to install CRDs from repos is a fairly common thing.
Maybe this turns into a nit-level suggestion then of making it obvious this is a test utility. Maybe like pkg/testutils/envtest? But totally a nit. Naming is hard ™️.
There was a problem hiding this comment.
Ok, cool, I've moved this to said pkg. TY
237ce42 to
03fa978
Compare
|
@joelanford I've addressed all comments, this is ready for a second pass review. |
|
/lgtm |
Stacked PR on top of #1
/hold
For #1 to merge first