feat: make HUB_CERTIFICATE_AUTHORITY optional to allow agent use OS's…

… root CA (#149)

* make HUB_CERTIFICATE_AUTHORITY optional to allow agent use OS's root certificate authority

* add fake content in fake token file

* fix wrong unit test, and a bug

* remove wantErr from unit test

* add expected result in unit test's name

api/v1alpha1/common_types.go

@@ -79,10 +79,10 @@ type ClusterNamespace string
 // ServiceInUseBy describes the member clusters that have requested to import a Service from the hub cluster.
 // This object is not provided directly as a part of fleet networking API, but provided as a contract for
 // marshaling/unmarshaling ServiceImport annotations, specifically for
-// * the InternalServiceImport controller to annotate on a ServiceImport which member clusters have requested to
-//   import the Service; and
-// * the EndpointSliceExport controller to find out from annotations on a ServiceImport which member clusters
-//   have requested to import the Service.
+//   - the InternalServiceImport controller to annotate on a ServiceImport which member clusters have requested to
+//     import the Service; and
+//   - the EndpointSliceExport controller to find out from annotations on a ServiceImport which member clusters
+//     have requested to import the Service.
 type ServiceInUseBy struct {
 	MemberClusters map[ClusterNamespace]ClusterID
 }

pkg/common/hubconfig/hubconfig.go

@@ -65,22 +65,21 @@ func PrepareHubConfig(tlsClientInsecure bool) (*rest.Config, error) {
 			},
 		}
 	} else {
+		var caData []byte
 		hubCA, err := env.Lookup(hubCAEnvKey)
-		if err != nil {
-			klog.ErrorS(err, "Hub certificate authority cannot be empty")
-			return nil, err
-		}
-		decodedClusterCaCertificate, err := base64.StdEncoding.DecodeString(hubCA)
-		if err != nil {
-			klog.ErrorS(err, "Cannot decode hub cluster certificate authority data")
-			return nil, err
+		if err == nil {
+			caData, err = base64.StdEncoding.DecodeString(hubCA)
+			if err != nil {
+				klog.ErrorS(err, "Cannot decode hub cluster certificate authority data")
+				return nil, err
+			}
 		}
 		hubConfig = &rest.Config{
 			BearerTokenFile: tokenFilePath,
 			Host:            hubURL,
 			TLSClientConfig: rest.TLSClientConfig{
 				Insecure: tlsClientInsecure,
-				CAData:   decodedClusterCaCertificate,
+				CAData:   caData,
 			},
 		}
 	}

pkg/common/hubconfig/hubconfig_test.go

@@ -18,111 +18,108 @@ import (
 func TestPrepareHubConfig(t *testing.T) {
 	var (
 		fakeHubhubServerURLEnvVal       = "fake-hub-server-url"
-		fakeConfigtokenConfigPathEnvVal = "fake-config-path" //nolint:gosec
-		fakeCerhubCAEnvVal              = base64.StdEncoding.EncodeToString([]byte("fake-certificate-authority"))
+		fakeConfigtokenConfigPathEnvVal = "testdata/fake-config-path" //nolint:gosec
+		fakeCerhubCA                    = []byte("fake-certificate-authority")
+		fakeCerhubCAEnvVal              = base64.StdEncoding.EncodeToString(fakeCerhubCA)
 	)
 
-	cleanupFunc := func() {
-		os.Unsetenv(hubServerURLEnvKey)
-		os.Unsetenv(tokenConfigPathEnvKey)
-		os.Unsetenv(hubCAEnvKey)
-		os.Remove(fakeConfigtokenConfigPathEnvVal)
-	}
-
-	defer cleanupFunc()
-
 	testCases := []struct {
 		name                 string
 		environmentVariables map[string]string
 		tlsClientInsecure    bool
-		wantErr              bool
+		validate             func(t *testing.T, config *rest.Config, err error)
 	}{
 		{
-			name:                 "environment variable `HUB_SERVER_URL` is not present",
+			name:                 "environment variable `HUB_SERVER_URL` is not present - error",
 			environmentVariables: map[string]string{tokenConfigPathEnvKey: fakeConfigtokenConfigPathEnvVal, hubCAEnvKey: fakeCerhubCAEnvVal},
 			tlsClientInsecure:    false,
-			wantErr:              true,
+			validate: func(t *testing.T, config *rest.Config, err error) {
+				if err == nil {
+					t.Errorf("expect return error if HUB_SERVER_URL not present")
+				}
+			},
 		},
 		{
-			name:                 "environment variable `CONFIG_PATH` is not present",
+			name:                 "environment variable `CONFIG_PATH` is not present - error",
 			environmentVariables: map[string]string{hubServerURLEnvKey: fakeHubhubServerURLEnvVal, hubCAEnvKey: fakeCerhubCAEnvVal},
 			tlsClientInsecure:    false,
-			wantErr:              true,
+			validate: func(t *testing.T, config *rest.Config, err error) {
+				if err == nil {
+					t.Errorf("expect return error if CONFIG_PATH not present")
+				}
+			},
 		},
 		{
-			name:                 "environment variable `HUB_CERTIFICATE_AUTHORITY` is not present when tlsClientInsecure is false",
+			name:                 "environment variable `HUB_CERTIFICATE_AUTHORITY` is not present when tlsClientInsecure is false - use OS's CA bundle",
 			environmentVariables: map[string]string{hubServerURLEnvKey: fakeHubhubServerURLEnvVal, tokenConfigPathEnvKey: fakeConfigtokenConfigPathEnvVal},
 			tlsClientInsecure:    false,
-			wantErr:              true,
+			validate: func(t *testing.T, config *rest.Config, err error) {
+				if err != nil {
+					t.Errorf("not expect error but actually get error %s", err)
+				}
+				wantConfig := &rest.Config{
+					BearerTokenFile: fakeConfigtokenConfigPathEnvVal,
+					Host:            fakeHubhubServerURLEnvVal,
+					TLSClientConfig: rest.TLSClientConfig{
+						Insecure: false,
+					},
+				}
+				if !cmp.Equal(config, wantConfig) {
+					t.Errorf("got hub config %+v, want %+v", config, wantConfig)
+				}
+			},
 		},
 		{
-			name:                 "environment variable `HUB_CERTIFICATE_AUTHORITY` is not present when tlsClientInsecure is true",
+			name:                 "environment variable `HUB_CERTIFICATE_AUTHORITY` is not present when tlsClientInsecure is true - use insecure TLS",
 			environmentVariables: map[string]string{hubServerURLEnvKey: fakeHubhubServerURLEnvVal, tokenConfigPathEnvKey: fakeConfigtokenConfigPathEnvVal},
 			tlsClientInsecure:    true,
-			wantErr:              false,
-		},
-		{
-			name:                 "hub configuration preparation is done when all requirements meet",
-			environmentVariables: map[string]string{hubServerURLEnvKey: fakeHubhubServerURLEnvVal, tokenConfigPathEnvKey: fakeConfigtokenConfigPathEnvVal, hubCAEnvKey: fakeCerhubCAEnvVal},
-			tlsClientInsecure:    false,
-			wantErr:              false,
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			// remove all environment variables related to this test
-			cleanupFunc()
-
-			for envKey, envVal := range tc.environmentVariables {
-				os.Setenv(envKey, envVal)
-				if envKey == tokenConfigPathEnvKey {
-					if _, err := os.Create(fakeConfigtokenConfigPathEnvVal); err != nil {
-						t.Errorf("failed to create file %s, err: %s", fakeConfigtokenConfigPathEnvVal, err.Error())
-					}
+			validate: func(t *testing.T, config *rest.Config, err error) {
+				if err != nil {
+					t.Errorf("not expect error but actually get error %s", err)
 				}
-			}
-
-			hubConfig, err := PrepareHubConfig(tc.tlsClientInsecure)
-			if (err != nil) != tc.wantErr {
-				t.Fatalf("PrepareHubConfig() error = %v, wantErr %t", err, tc.wantErr)
-			}
-
-			if tc.wantErr {
-				return
-			}
-
-			if tc.tlsClientInsecure {
-				expectedHubConfig := &rest.Config{
+				wantConfig := &rest.Config{
 					BearerTokenFile: fakeConfigtokenConfigPathEnvVal,
 					Host:            fakeHubhubServerURLEnvVal,
 					TLSClientConfig: rest.TLSClientConfig{
-						Insecure: tc.tlsClientInsecure,
+						Insecure: true,
 					},
 				}
-				if !cmp.Equal(*hubConfig, *expectedHubConfig) {
-					t.Errorf("PrepareHubConfig() got hub config: %v, want: %v", expectedHubConfig, hubConfig)
+				if !cmp.Equal(config, wantConfig) {
+					t.Errorf("got hub config %+v, want %+v", config, wantConfig)
 				}
-			}
-
-			if !tc.tlsClientInsecure {
-				decodedClusterCaCertificate, err := base64.StdEncoding.DecodeString(fakeCerhubCAEnvVal)
+			},
+		},
+		{
+			name:                 "hub configuration preparation is done when all requirements meet - use CAData",
+			environmentVariables: map[string]string{hubServerURLEnvKey: fakeHubhubServerURLEnvVal, tokenConfigPathEnvKey: fakeConfigtokenConfigPathEnvVal, hubCAEnvKey: fakeCerhubCAEnvVal},
+			tlsClientInsecure:    false,
+			validate: func(t *testing.T, config *rest.Config, err error) {
 				if err != nil {
-					t.Fatalf("failed to base-encode hub CA, error: %s", err.Error())
+					t.Errorf("not expect error but actually get error %s", err)
 				}
-				expectedHubConfig := &rest.Config{
+				wantConfig := &rest.Config{
 					BearerTokenFile: fakeConfigtokenConfigPathEnvVal,
 					Host:            fakeHubhubServerURLEnvVal,
 					TLSClientConfig: rest.TLSClientConfig{
-						Insecure: tc.tlsClientInsecure,
-						CAData:   decodedClusterCaCertificate,
+						Insecure: false,
+						CAData:   fakeCerhubCA,
 					},
 				}
-
-				if !cmp.Equal(hubConfig, expectedHubConfig) {
-					t.Errorf("PrepareHubConfig() got hub config: %v, want: %v", expectedHubConfig, hubConfig)
+				if !cmp.Equal(config, wantConfig) {
+					t.Errorf("got hub config %+v, want %+v", config, wantConfig)
 				}
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			for envKey, envVal := range tc.environmentVariables {
+				t.Setenv(envKey, envVal)
 			}
+
+			hubConfig, err := PrepareHubConfig(tc.tlsClientInsecure)
+			tc.validate(t, hubConfig, err)
 		})
 	}
 }

pkg/common/hubconfig/testdata/fake-config-path

@@ -0,0 +1 @@
+this is token

pkg/common/uniquename/uniquename.go

@@ -53,9 +53,9 @@ func removeDots(s string) string {
 // is [NAMESPACE]-[NAME]-[SUFFIX], e.g. an object `app` from the namespace `work` will be assigned a unique
 // name like `work-app-1x2yz`. The function may truncate name components as it sees fit.
 // Note: this function assumes that
-// * the input object namespace is a valid RFC 1123 DNS label; and
-// * the input object name follows one of the three formats used in Kubernetes (RFC 1123 DNS subdomain,
-// 	 RFC 1123 DNS label, RFC 1035 DNS label).
+//   - the input object namespace is a valid RFC 1123 DNS label; and
+//   - the input object name follows one of the three formats used in Kubernetes (RFC 1123 DNS subdomain,
+//     RFC 1123 DNS label, RFC 1035 DNS label).
 func ClusterScopedUniqueName(format Format, namespace, name string) (string, error) {
 	reservedSlots := 2 + uuidLength // 2 dashes + 5 character UUID string
 
@@ -124,10 +124,10 @@ func ClusterScopedUniqueName(format Format, namespace, name string) (string, err
 // `work` in cluster `bravelion` will be assigned a unique name like `bravelion-work-app-1x2yz`. The function may
 // truncate name components as it sees fit.
 // Note: this function assumes that
-// * the input cluster ID is a valid RFC 1123 DNS subdomain; and
-// * the input object namespace is a valid RFC 1123 DNS label; and
-// * the input object name follows one of the three formats used in Kubernetes (RFC 1123 DNS subdomain,
-// 	 RFC 1123 DNS label, RFC 1035 DNS label).
+//   - the input cluster ID is a valid RFC 1123 DNS subdomain; and
+//   - the input object namespace is a valid RFC 1123 DNS label; and
+//   - the input object name follows one of the three formats used in Kubernetes (RFC 1123 DNS subdomain,
+//     RFC 1123 DNS label, RFC 1035 DNS label).
 func FleetScopedUniqueName(format Format, clusterID, namespace, name string) (string, error) {
 	reservedSlots := 3 + uuidLength // 3 dashes + 5 character UUID string
 

pkg/controllers/member/internalmembercluster/controller.go

@@ -133,9 +133,9 @@ func findAgentStatus(status []fleetv1alpha1.AgentStatus, agentType fleetv1alpha1
 
 // setAgentStatus sets the corresponding condition in conditions based on the new agent status.
 // status must be non-nil.
-// 1. if the condition of the specified type already exists (all fields of the existing condition are updated to
-//    newCondition, LastTransitionTime is set to now if the new status differs from the old status)
-// 2. if a condition of the specified type does not exist (LastTransitionTime is set to now() if unset, and newCondition is appended)
+//  1. if the condition of the specified type already exists (all fields of the existing condition are updated to
+//     newCondition, LastTransitionTime is set to now if the new status differs from the old status)
+//  2. if a condition of the specified type does not exist (LastTransitionTime is set to now() if unset, and newCondition is appended)
 func setAgentStatus(status *[]fleetv1alpha1.AgentStatus, newStatus fleetv1alpha1.AgentStatus) {
 	existingStatus := findAgentStatus(*status, newStatus.Type)
 	if existingStatus == nil {

test/e2e/export_service_test.go

@@ -8,7 +8,7 @@ package e2e
 import (
 	"context"
 	"fmt"
-	"io/ioutil"
+	"io"
 	"net/http"
 	"strings"
 	"time"
@@ -991,7 +991,7 @@ func fetchHTTPRequestBody(requestURL string) (string, error) {
 		return "", err
 	}
 	defer res.Body.Close()
-	respBody, err := ioutil.ReadAll(res.Body)
+	respBody, err := io.ReadAll(res.Body)
 	if err != nil {
 		return "", err
 	}