test: Use Pebble as the ACME server in integration tests #339

marc1404 · 2024-11-07T16:12:41Z

How to categorize this PR?

/kind test
/kind enhancement

What this PR does / why we need it:

The integration test skeleton added with #315 currently uses the Let's Encrypt ACME staging service.
While it can be safely assumed that this service should be relatively stable it means that the integration tests are making calls to the internet and effectively depend on an external service. Furthermore, we cannot skip the DNS-01 challenge in integration tests as the staging service would enforce passed challenges.

Pebble is a small ACME server intended to be run for testing purposes. It's already used in the functional E2E tests. This PR invokes the code to start the Pebble ACME server locally during the integration test suite startup.

Which issue(s) this PR fixes:

n.a.

Special notes for your reviewer:

/cc @MartinWeindel

Thanks to @MartinWeindel for the great idea! 💡
Additional thanks to @LucaBernstein for debugging and fixing the TLS setup. 🔐

Release note:

Use Pebble as an ACME server in the integration tests.

…aging service

…e and test

marc1404 · 2024-11-07T16:14:05Z

test/integration/controller/issuer/issuer_suite_test.go

+	restConfig           *rest.Config
+	testEnv              *envtest.Environment
+	testClient           client.Client
+	acmeDirectoryAddress string


acmeDirectoryAddress has been added. This variable holds the directory address of the local Pebble ACME server that will be used to create Issuer resources in tests.
The test asserts that this variable has been set before the test starts.

marc1404 · 2024-11-07T16:14:30Z

test/integration/controller/issuer/issuer_suite_test.go

+	By("Start Pebble ACME server")
+	certificatePath, directoryAddress, err := testutils.RunPebble(log.WithName("pebble"))
+	Expect(err).NotTo(HaveOccurred())
+	acmeDirectoryAddress = directoryAddress


Sets the package scoped variable that tests will use to create Issuer resources.

marc1404 · 2024-11-07T16:15:39Z

test/integration/controller/issuer/issuer_suite_test.go

@@ -86,6 +100,7 @@ var _ = BeforeSuite(func() {
 	DeferCleanup(func() {
 		By("Stop test environment")
 		Expect(testEnv.Stop()).To(Succeed())
+		_ = os.RemoveAll(filepath.Dir(certificatePath))


The code responsible for starting the Pebble server generates a certificate and private key on the fly in a temporary OS directory.
Instead of deleting the two files and their directory individually, we can clear them in one sweep.

marc1404 · 2024-11-07T16:16:43Z

test/integration/controller/issuer/issuer_test.go

@@ -28,6 +28,8 @@ var _ = Describe("Issuer controller tests", func() {
 	)

 	BeforeEach(func() {
+		Expect(acmeDirectoryAddress).NotTo(BeEmpty())


During development, I messed up initializing the package level variable in the test suite. Tests should assert early on that the variable has been set.

marc1404 · 2024-11-07T16:18:04Z

test/utils/logbridge.go

+package testutils
+
+import (
+	"github.com/go-logr/logr"
+	"log"
+	"strings"
+)
+
+type logBridge struct {
+	logr logr.Logger
+}
+
+func (logBridge *logBridge) Write(p []byte) (n int, err error) {
+	message := strings.TrimSpace(string(p))
+
+	logBridge.logr.Info(message)
+
+	return len(p), nil
+}
+
+// NewLogBridge creates a new log.Logger that forwards all log messages to the given logr.Logger.
+func NewLogBridge(logr logr.Logger) *log.Logger {
+	writer := &logBridge{logr}
+
+	return log.New(writer, "", 0)
+}


Pebble's code expects a logger from the log package. (ref)
This utility creates a bridge that returns a log.Logger but redirects messages to the logr.Logger used by the test suite.

marc1404 · 2024-11-07T16:19:25Z

test/utils/pebble.go

+	// We don't want to go through DNS-01 challenges in the integration tests as we would have to spin up a local, authoritative DNS server.
+	// Setting the environment variable PEBBLE_VA_ALWAYS_VALID to 1 makes the Pebble server always return a valid response for the validation authority.
+	// Testing the DNS-01 challenge is covered by the functional E2E tests.
+	// See the Pebble documentation: https://github.com/letsencrypt/pebble#skipping-validation
+	err = os.Setenv("PEBBLE_VA_ALWAYS_VALID", "1")
+	if err != nil {
+		return "", "", fmt.Errorf("failed to set environment variable: %v", err)
+	}


This could also be done in the test suite but as we never want to pass DNS-01 challenges in the integration tests I figured the responsibility could be kept in the Pebble utility code.

marc1404 · 2024-11-07T16:21:43Z

test/utils/pebble.go

+		err := http.ListenAndServeTLS(
+			listenAddress,
+			certificatePath,
+			privateKeyPath,
+			muxHandler)
+		cmd.FailOnError(err, "Calling ListenAndServeTLS()")


http.ListenAndServeTLS is a blocking function that only returns in case of an error (e.g. address already in use).

cmd.FailOnError is implemented by Pebble and effectively exits the process with the error.
It's useful as it will exit the integration test suite early with a descriptive error message in case the Pebble server can't be started.

marc1404 · 2024-11-07T16:25:54Z

test/utils/pebble.go

+	rootCAs, err := loadCertPool(certificatePath)
+	if err != nil {
+		return err
+	}
+
+	customTransport := http.DefaultTransport.(*http.Transport).Clone()
+	customTransport.TLSClientConfig = &tls.Config{RootCAs: rootCAs}
+	http.DefaultTransport = customTransport
+	client := &http.Client{Transport: customTransport}


The ACME protocol enforces TLS and the utility code generates a temporary certificate on the fly.
We need to trust the certificate when performing the availability check. Otherwise, we'd have to skip the insecure validation.

MartinWeindel

I took a quick look to the gosec issues (run make sast to list them) and provide easy solutions for them.

test/utils/pebble.go

MartinWeindel

/lgtm

I've fixed a minor issuer on the fly. The pebble HTTP server was not closed in the cleanup.

Great to have an ACME server in the integration tests. Thanks!

gardener-prow · 2024-11-08T10:55:43Z

LGTM label has been added.

Git tree hash: 790764bc527482a3867e684fea0cc63c6520a8d0

gardener-prow · 2024-11-08T10:55:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MartinWeindel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [MartinWeindel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

marc1404 · 2024-11-08T12:07:04Z

I've fixed a minor issuer on the fly. The pebble HTTP server was not closed in the cleanup.

@MartinWeindel Great catch! My bad, thank you! 🙇

marc1404 added 9 commits November 7, 2024 14:53

test: Install letsencrypt/pebble/v2

1ee52d4

test: Implement a bridge from log.Logger to logr.Logger

5261057

test: Implement utility to start Pebble on the fly

976f568

test: Start Pebble at the beginning of the test suite

c4bd12e

test: Use the local Pebble ACME address instead of Let's Encrypt's st…

9a4b83b

…aging service

style: Format using goimports

5247220

style: Format comments

1f74efd

refactor: Assert that the acmeDirectoryAddress is passed between suit…

302b707

…e and test

test: Make Pebble skip the validation of DNS-01 challenges

4b5c3c3

gardener-prow bot requested a review from MartinWeindel November 7, 2024 16:12

gardener-prow bot added kind/test Test kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2024

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 7, 2024

gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 7, 2024

marc1404 commented Nov 7, 2024

View reviewed changes

MartinWeindel requested changes Nov 8, 2024

View reviewed changes

test/utils/pebble.go Outdated Show resolved Hide resolved

test/utils/pebble.go Outdated Show resolved Hide resolved

test/utils/pebble.go Outdated Show resolved Hide resolved

gardener-prow bot assigned MartinWeindel Nov 8, 2024

MartinWeindel requested changes Nov 8, 2024

View reviewed changes

test/utils/pebble.go Outdated Show resolved Hide resolved

marc1404 added 2 commits November 8, 2024 09:10

fix: Fix unused variable restConfig

60c891f

style: Format logbridge.go with goimports

7d7e9f1

marc1404 added 5 commits November 8, 2024 09:24

fix: Set min version for TLS (gosec)

4e54849

fix: Clean certificate file path (gosec)

11f0c63

fix: Set timeouts for Pebble server (gosec)

b3fc789

chore: go mod tidy

c0c9f48

refactor: Inline code for generate_cert.go (gosec)

4ef2bd7

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2024

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2024

marc1404 requested a review from MartinWeindel November 8, 2024 09:31

gardener deleted a comment from gardener-prow bot Nov 8, 2024

close pebble HTTP server on test suite cleanup

ff75f19

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 8, 2024

MartinWeindel approved these changes Nov 8, 2024

View reviewed changes

gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024

gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2024

gardener-prow bot merged commit aa2ad79 into master Nov 8, 2024
11 checks passed

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2024

marc1404 deleted the test/use-pebble-acme-server-in-integration-test branch November 8, 2024 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Use Pebble as the ACME server in integration tests #339

test: Use Pebble as the ACME server in integration tests #339

marc1404 commented Nov 7, 2024

marc1404 Nov 7, 2024

marc1404 Nov 7, 2024

marc1404 Nov 7, 2024 •

edited

Loading

marc1404 Nov 7, 2024

marc1404 Nov 7, 2024 •

edited

Loading

marc1404 Nov 7, 2024

marc1404 Nov 7, 2024

marc1404 Nov 7, 2024

MartinWeindel left a comment

MartinWeindel left a comment

gardener-prow bot commented Nov 8, 2024

gardener-prow bot commented Nov 8, 2024

marc1404 commented Nov 8, 2024 •

edited

Loading

test: Use Pebble as the ACME server in integration tests #339

test: Use Pebble as the ACME server in integration tests #339

Conversation

marc1404 commented Nov 7, 2024

marc1404 Nov 7, 2024

Choose a reason for hiding this comment

marc1404 Nov 7, 2024

Choose a reason for hiding this comment

marc1404 Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

marc1404 Nov 7, 2024

Choose a reason for hiding this comment

marc1404 Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

marc1404 Nov 7, 2024

Choose a reason for hiding this comment

marc1404 Nov 7, 2024

Choose a reason for hiding this comment

marc1404 Nov 7, 2024

Choose a reason for hiding this comment

MartinWeindel left a comment

Choose a reason for hiding this comment

MartinWeindel left a comment

Choose a reason for hiding this comment

gardener-prow bot commented Nov 8, 2024

gardener-prow bot commented Nov 8, 2024

marc1404 commented Nov 8, 2024 • edited Loading

marc1404 Nov 7, 2024 •

edited

Loading

marc1404 Nov 7, 2024 •

edited

Loading

marc1404 commented Nov 8, 2024 •

edited

Loading