Consolidate some probe code #373
Conversation
…in different packages, upgrade mutual MTLS test to use 4.0.4 instead of 3.11.7
| @@ -214,7 +214,7 @@ endif | |||
|
|
|||
| .PHONY: cert-manager | |||
| cert-manager: ## Install cert-manager to the cluster | |||
| kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.8.0/cert-manager.yaml | |||
| kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.8.2/cert-manager.yaml | |||
There was a problem hiding this comment.
It would be good to keep common deps in sync between cass-operator and k8ssandra-operator. Can you upgrade cert-manager in k8ssandra-operator as well?
pkg/httphelper/security.go
Outdated
| @@ -171,7 +170,8 @@ func (provider *InsecureManagementApiSecurityProvider) BuildMgmtApiWgetAction(en | |||
| "wget", | |||
| "--output-document", "/dev/null", | |||
| "--no-check-certificate", | |||
| fmt.Sprintf("http://%s", endpoint), | |||
| "--timeout 20", | |||
There was a problem hiding this comment.
Should the timeout be configurable?
pkg/httphelper/security.go
Outdated
| @@ -185,7 +185,8 @@ func (provider *ManualManagementApiSecurityProvider) BuildMgmtApiWgetAction(endp | |||
| "--certificate", tlsCrt, | |||
| "--private-key", tlsKey, | |||
| "--ca-certificate", caCertPath, | |||
| fmt.Sprintf("https://%s", endpoint), | |||
| "--timeout 20", | |||
There was a problem hiding this comment.
Should the timeout be configurable?
There was a problem hiding this comment.
Not really, we don't allow configuring other mgmt-api timeouts in the code either.
There was a problem hiding this comment.
A hard coded value seems less than ideal. If someone, even for dev/testings needs to adjust it is going require building a new image. What about making it configurable through the operator config map?
There was a problem hiding this comment.
That would make it inconsistent with the httpGet calls we have as well as all the client.go (management-api communications), they're not configurable either.
We can make a ticket to configure all those, but that will require quite a lot of work and break APIs for all users of management-api client.
There was a problem hiding this comment.
I added a commit that allows it. It will break backwards compatibility, so I wouldn't backport it.
There was a problem hiding this comment.
Before you pushed the commit I was about reply saying I am ok punting if you think it is unnecessary :)
pkg/httphelper/security.go
Outdated
| @@ -196,8 +197,9 @@ func (provider *InsecureManagementApiSecurityProvider) BuildMgmtApiWgetPostActio | |||
| "wget", | |||
| "--output-document", "/dev/null", | |||
| "--no-check-certificate", | |||
| "--timeout 20", | |||
There was a problem hiding this comment.
Should the timeout be configurable?
pkg/httphelper/security.go
Outdated
| @@ -211,8 +213,9 @@ func (provider *ManualManagementApiSecurityProvider) BuildMgmtApiWgetPostAction( | |||
| "--certificate", tlsCrt, | |||
| "--private-key", tlsKey, | |||
| "--ca-certificate", caCertPath, | |||
| "--timeout 20", | |||
There was a problem hiding this comment.
Should the timeout be configurable?
|
@burmanm Can you also create an issue for this with some details on why the wget calls are made and why it's a good idea to add the timeouts? |
|
Changes look good other than the failing tests. |
* Add timeout to all wget calls * Make probe timeouts configurable when using mutual auth * Remove deprecated BuildNameToCertificate call
What this PR does:
Adds back the terminate test for drain test and consolidates from probe and presto hook URL paths.
Which issue(s) this PR fixes:
Fixes #377
Checklist