-
Notifications
You must be signed in to change notification settings - Fork 49
Parameterise ClusterIssuer for Dex, Gangway, HTTPBin #482
Conversation
So the tests failed with weird error:
Are we hitting: golang/go#24652 ? |
No, this is expected. We need to modify the test to ignore self-signed certs, or just test HTTP. http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true} |
I can modify the test to accept self signed certs, but does that mean this PR is irrelevant? |
No, why irrelevant? We definitely want to use the staging environment assuming we're already randomizing the hostname and are hitting the maximum certs per email LE limit. I've tried to catch as much problems as possible with |
a24d6c7
to
09e9f50
Compare
This commit adds a variable `certmanager_cluster_issuer` which allows components to specify which cluster issuer to be used with Certmanager. This variable is added for components Dex, Gangway, HTTPBin. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
This will avoid the throttling done by LetsEncrypt when running in CI. Hence it will avoid the AWS e2e test failure. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
This commit adds docs to Dex, Gangway, Httpbin configuration reference guide. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
09e9f50
to
2427c70
Compare
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.
LGTM
This commit fixes the test to use LetsEncrypt Staging Root PEM in the http client. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
2427c70
to
8b5d518
Compare
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.
lgtm
rootCAs := x509.NewCertPool() | ||
if ok := rootCAs.AppendCertsFromPEM([]byte(letsEncryptStagingRootPEM)); !ok { | ||
// This should fail in the developer testing itself. | ||
panic("Failed to parse root certificate") |
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.
Using t.Fatal()
would be better here, as panic
will abort all the tests.
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.
I think panic is ok here. This function decodes a PEM-encoded certificate which is statically defined in code so it should not fail during regular testing.
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.
Yeah, it's very nit-picking from my side 😂
Note: See changes per commit.
certmanager: Parameterise components to provide ClusterIssuer
Add a variable
certmanager_cluster_issuer
which allow components to specify which cluster issuer to be used with Certmanager. This variable is added for components Dex, Gangway, HTTPBin.ci: Use certmanager clusterissuer
letsencrypt-staging
This will avoid the throttling done by LetsEncrypt when running in CI. Hence it will avoid the AWS e2e test failure.
docs: Add variable
certmanager_cluster_issuer
Add docs to Dex, Gangway, Httpbin configuration reference guide.
Fixes #437