Repository: metal3-io/baremetal-operator
Pull request #619 (2020-09-01 21:54:12)

allow mock ironic/inspector service to run without specifying a port
Rewrite the mock server for the ironic provisioner ready checks so it
does not need a specific port. This will
eventually make it easier to expand the server to support other tests.

metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/619#" title="Author self-approved">dhellmann</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [dhellmann]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dhellmann: /test-integration

metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/619#" title="Author self-approved">dhellmann</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/619#pullrequestreview-481257793" title="Approved">zaneb</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [dhellmann,zaneb]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dhellmann: > I was a bit confused at the headline of the PR and commit messages at first; of course the server is still listening on a port and communicating over a socket, but now it uses a system-chosen port instead of requiring us to reserve ports 6385 and 5050 for this.

I thought based on reading the description of the test server that it was using some sort of in-memory communication instead of a socket, but of course you're right because then we would need a special client, too.

Patch:
diff --git a/docs/dev-setup.md b/docs/dev-setup.md
index e3a10306..3767476a 100644
--- a/docs/dev-setup.md
+++ b/docs/dev-setup.md
@@ -2,7 +2,7 @@
 
 ## Install the operator-sdk
 
-Follow the instructions on the [Operator SDK website](https://sdk.operatorframework.io/docs/installation/install-operator-sdk/)
+Follow the instructions on the [Operator SDK website](https://sdk.operatorframework.io/docs/install-operator-sdk/)
 to check out and install the operator-sdk tools.
 
 ## Install openapi-gen
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 86cfe31f..1d8b6872 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -122,13 +122,13 @@ func LogStartup() {
 
 // A private function to construct an ironicProvisioner (rather than a
 // Provisioner interface) in a consistent way for tests.
-func newProvisioner(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (*ironicProvisioner, error) {
-	clientIronic, err := clients.IronicClient(ironicEndpoint, ironicAuth)
+func newProvisionerWithSettings(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher, ironicURL string, ironicAuthSettings clients.AuthConfig, inspectorURL string, inspectorAuthSettings clients.AuthConfig) (*ironicProvisioner, error) {
+	clientIronic, err := clients.IronicClient(ironicURL, ironicAuthSettings)
 	if err != nil {
 		return nil, err
 	}
 
-	clientInspector, err := clients.InspectorClient(inspectorEndpoint, inspectorAuth)
+	clientInspector, err := clients.InspectorClient(inspectorURL, inspectorAuthSettings)
 	if err != nil {
 		return nil, err
 	}
@@ -155,6 +155,14 @@ func newProvisioner(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials
 	return p, nil
 }
 
+// A private function to construct an ironicProvisioner (rather than a
+// Provisioner interface) in a consistent way for tests.
+func newProvisioner(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (*ironicProvisioner, error) {
+	return newProvisionerWithSettings(host, bmcCreds, publisher,
+		ironicEndpoint, ironicAuth, inspectorEndpoint, inspectorAuth,
+	)
+}
+
 // New returns a new Ironic Provisioner
 func New(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) {
 	return newProvisioner(host, bmcCreds, publisher)


Test patch:
diff --git a/pkg/provisioner/ironic/ironic_test.go b/pkg/provisioner/ironic/ironic_test.go
index 1607ae57..5b25573c 100644
--- a/pkg/provisioner/ironic/ironic_test.go
+++ b/pkg/provisioner/ironic/ironic_test.go
@@ -2,11 +2,8 @@ package ironic
 
 import (
 	"fmt"
-	"net"
 	"net/http"
 	"net/http/httptest"
-	"strconv"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -18,6 +15,7 @@ import (
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/pkg/apis/metal3/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
 	"k8s.io/utils/pointer"
 )
 
@@ -39,61 +37,61 @@ func TestProvisionerIsReady(t *testing.T) {
 	}{
 		{
 			name:      "IsReady",
-			ironic:    newMockServer(6385).addDrivers(),
-			inspector: newMockServer(5050),
+			ironic:    newMockServer(t, "ironic").addDrivers(),
+			inspector: newMockServer(t, "inspector"),
 
-			expectedIronicCalls:    "localhost:6385/v1;localhost:6385/v1/drivers;",
-			expectedInspectorCalls: "localhost:5050/v1;",
+			expectedIronicCalls:    "/v1;/v1/drivers;",
+			expectedInspectorCalls: "/v1;",
 			expectedIsReady:        true,
 		},
 		{
 			name:      "NoDriversLoaded",
-			ironic:    newMockServer(6385),
-			inspector: newMockServer(5050),
+			ironic:    newMockServer(t, "ironic"),
+			inspector: newMockServer(t, "inspector"),
 
-			expectedIronicCalls: "localhost:6385/v1;localhost:6385/v1/drivers;",
+			expectedIronicCalls: "/v1;/v1/drivers;",
 		},
 		{
 			name:      "IronicDown",
-			inspector: newMockServer(5050),
+			inspector: newMockServer(t, "inspector"),
 
 			expectedIsReady: false,
 		},
 		{
 			name:   "InspectorDown",
-			ironic: newMockServer(6385).addDrivers(),
+			ironic: newMockServer(t, "ironic").addDrivers(),
 
-			expectedIronicCalls: "localhost:6385/v1;localhost:6385/v1/drivers;",
+			expectedIronicCalls: "/v1;/v1/drivers;",
 
 			expectedIsReady: false,
 		},
 		{
 			name:      "IronicNotOk",
-			ironic:    newMockServer(6385).setErrorCode(http.StatusInternalServerError),
-			inspector: newMockServer(5050),
+			ironic:    newMockServer(t, "ironic").setErrorCode(http.StatusInternalServerError),
+			inspector: newMockServer(t, "inspector"),
 
 			expectedIsReady: false,
 
-			expectedIronicCalls: "localhost:6385/v1;",
+			expectedIronicCalls: "/v1;",
 		},
 		{
 			name:      "IronicNotOkAndNotExpected",
-			ironic:    newMockServer(6385).setErrorCode(http.StatusBadGateway),
-			inspector: newMockServer(5050),
+			ironic:    newMockServer(t, "ironic").setErrorCode(http.StatusBadGateway),
+			inspector: newMockServer(t, "inspector"),
 
 			expectedIsReady: false,
 
-			expectedIronicCalls: "localhost:6385/v1;",
+			expectedIronicCalls: "/v1;",
 		},
 		{
 			name:      "InspectorNotOk",
-			ironic:    newMockServer(6385).addDrivers(),
-			inspector: newMockServer(5050).setErrorCode(http.StatusInternalServerError),
+			ironic:    newMockServer(t, "ironic").addDrivers(),
+			inspector: newMockServer(t, "inspector").setErrorCode(http.StatusInternalServerError),
 
 			expectedIsReady: false,
 
-			expectedIronicCalls:    "localhost:6385/v1;localhost:6385/v1/drivers;",
-			expectedInspectorCalls: "localhost:5050/v1;",
+			expectedIronicCalls:    "/v1;/v1/drivers;",
+			expectedInspectorCalls: "/v1;",
 		},
 	}
 
@@ -109,7 +107,11 @@ func TestProvisionerIsReady(t *testing.T) {
 				defer tc.inspector.stop()
 			}
 
-			prov, err := newProvisioner(makeHost(), bmc.Credentials{}, nil)
+			auth := clients.AuthConfig{Type: clients.NoAuth}
+
+			prov, err := newProvisionerWithSettings(makeHost(), bmc.Credentials{}, nil,
+				tc.ironic.Endpoint(), auth, tc.inspector.Endpoint(), auth,
+			)
 			ready, err := prov.IsReady()
 
 			if tc.ironic != nil {
@@ -132,8 +134,11 @@ func TestProvisionerIsReady(t *testing.T) {
 func TestGetUpdateOptsForNodeWithRootHints(t *testing.T) {
 
 	eventPublisher := func(reason, message string) {}
+	auth := clients.AuthConfig{Type: clients.NoAuth}
 
-	prov, err := newProvisioner(makeHost(), bmc.Credentials{}, eventPublisher)
+	prov, err := newProvisionerWithSettings(makeHost(), bmc.Credentials{}, eventPublisher,
+		"https://ironic.test", auth, "https://ironic.test", auth,
+	)
 	ironicNode := &nodes.Node{}
 
 	patches, err := prov.getUpdateOptsForNode(ironicNode)
@@ -215,8 +220,11 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 	}
 
 	eventPublisher := func(reason, message string) {}
+	auth := clients.AuthConfig{Type: clients.NoAuth}
 
-	prov, err := newProvisioner(host, bmc.Credentials{}, eventPublisher)
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, eventPublisher,
+		"https://ironic.test", auth, "https://ironic.test", auth,
+	)
 	ironicNode := &nodes.Node{}
 
 	patches, err := prov.getUpdateOptsForNode(ironicNode)
@@ -310,8 +318,11 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 	}
 
 	eventPublisher := func(reason, message string) {}
+	auth := clients.AuthConfig{Type: clients.NoAuth}
 
-	prov, err := newProvisioner(host, bmc.Credentials{}, eventPublisher)
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, eventPublisher,
+		"https://ironic.test", auth, "https://ironic.test", auth,
+	)
 	ironicNode := &nodes.Node{}
 
 	patches, err := prov.getUpdateOptsForNode(ironicNode)
@@ -427,14 +438,16 @@ func makeHost() *metal3v1alpha1.BareMetalHost {
 	}
 }
 
-func newMockServer(port int) *mockServer {
+func newMockServer(t *testing.T, name string) *mockServer {
 	return &mockServer{
-		port: strconv.Itoa(port),
+		t:    t,
+		name: name,
 	}
 }
 
 type mockServer struct {
-	port      string
+	t         *testing.T
+	name      string
 	requests  string
 	server    *httptest.Server
 	drivers   string
@@ -471,29 +484,48 @@ func (m *mockServer) addDrivers() *mockServer {
 	return m
 }
 
-func (m *mockServer) start() *mockServer {
-	listener, err := net.Listen("tcp", "127.0.0.1:"+m.port)
-	if err != nil {
-		panic(err)
+func (m *mockServer) Endpoint() string {
+	if m == nil || m.server == nil {
+		// The consumer of this method expects something valid, but
+		// won't use it if m is nil.
+		return "https://ironic.test/v1/"
+	}
+	response := m.server.URL + "/v1/"
+	m.t.Logf("%s: endpoint: %s/", m.name, response)
+	return response
+}
+
+func (m *mockServer) logRequest(r *http.Request, msg string) {
+	m.t.Logf("%s: %s %s", m.name, msg, r.URL)
+	m.requests += r.RequestURI + ";"
+}
+
+func (m *mockServer) handleNoResponse(w http.ResponseWriter, r *http.Request) {
+	m.logRequest(r, "no response")
+	if m.errorCode != 0 {
+		http.Error(w, "An error", m.errorCode)
+		return
 	}
+}
 
-	m.server = httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		m.requests += r.Host + r.RequestURI + ";"
+func (m *mockServer) handleDrivers(w http.ResponseWriter, r *http.Request) {
+	m.logRequest(r, "drivers")
+	if m.errorCode != 0 {
+		http.Error(w, "An error", m.errorCode)
+		return
+	}
+	w.Header().Set("Content-Type", "application/json")
+	fmt.Fprintf(w, m.drivers)
+}
 
-		if m.errorCode != 0 {
-			http.Error(w, "An error", m.errorCode)
-			return
-		}
+func (m *mockServer) start() *mockServer {
 
-		if strings.Contains(r.RequestURI, "/v1/drivers") {
-			w.Header().Set("Content-Type", "application/json")
-			fmt.Fprintf(w, m.drivers)
-		}
-	}))
+	mux := http.NewServeMux()
+	mux.HandleFunc("/", m.handleNoResponse)
+	mux.HandleFunc("/v1", m.handleNoResponse)
+	mux.HandleFunc("/v1/drivers", m.handleDrivers)
 
-	m.server.Listener.Close()
-	m.server.Listener = listener
-	m.server.Start()
+	m.server = httptest.NewServer(mux)
 
 	return m
 }