Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Parameterise ClusterIssuer for Dex, Gangway, HTTPBin #482

Merged
merged 4 commits into from
May 26, 2020

Conversation

surajssd
Copy link
Member

@surajssd surajssd commented May 26, 2020

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

@surajssd
Copy link
Member Author

So the tests failed with weird error:

    TestAWSIngress: aws_test.go:49: got an HTTP error: Get "https://httpbin.ci1590496091-tf.test.lokomotive-k8s.net/get": dial tcp 18.157.129.234:443: connect: connection refused
    TestAWSIngress: aws_test.go:49: got an HTTP error: Get "https://httpbin.ci1590496091-tf.test.lokomotive-k8s.net/get": x509: certificate signed by unknown authority

Are we hitting: golang/go#24652 ?

@johananl
Copy link
Member

johananl commented May 26, 2020

So the tests failed with weird error:

    TestAWSIngress: aws_test.go:49: got an HTTP error: Get "https://httpbin.ci1590496091-tf.test.lokomotive-k8s.net/get": dial tcp 18.157.129.234:443: connect: connection refused
    TestAWSIngress: aws_test.go:49: got an HTTP error: Get "https://httpbin.ci1590496091-tf.test.lokomotive-k8s.net/get": x509: certificate signed by unknown authority

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}

@surajssd
Copy link
Member Author

No, this is expected. We need to modify the test to ignore self-signed certs, or just test HTTP.

I can modify the test to accept self signed certs, but does that mean this PR is irrelevant?

@johananl
Copy link
Member

johananl commented May 26, 2020

No, this is expected. We need to modify the test to ignore self-signed certs, or just test HTTP.

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 TestAWSIngress. That's why I've used HTTPS. Perhaps this wasn't the best idea 🤷 I'll let you fix this as you see fit.

@surajssd surajssd force-pushed the surajssd/parameterise-cluster-issuers branch 2 times, most recently from a24d6c7 to 09e9f50 Compare May 26, 2020 14:16
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>
@surajssd surajssd force-pushed the surajssd/parameterise-cluster-issuers branch from 09e9f50 to 2427c70 Compare May 26, 2020 15:29
@surajssd surajssd requested review from ipochi and johananl May 26, 2020 15:30
johananl
johananl previously approved these changes May 26, 2020
Copy link
Member

@johananl johananl left a comment

Choose a reason for hiding this comment

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

LGTM

test/ingress/aws/aws_test.go Outdated Show resolved Hide resolved
This commit fixes the test to use LetsEncrypt Staging Root PEM in the
http client.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

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

lgtm

@surajssd surajssd merged commit fb2bfa6 into master May 26, 2020
@surajssd surajssd deleted the surajssd/parameterise-cluster-issuers branch May 26, 2020 23:14
rootCAs := x509.NewCertPool()
if ok := rootCAs.AppendCertsFromPEM([]byte(letsEncryptStagingRootPEM)); !ok {
// This should fail in the developer testing itself.
panic("Failed to parse root certificate")
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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 😂

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameterise the ClusterIssuers for components
5 participants