Code review comments

2025-08-06 01:26:59 +02:00 · 2018-07-06 11:44:33 +01:00 · 2018-07-06 11:44:33 +01:00 · 3c9a944fec
commit 3c9a944fec
parent 88da61e742
6 changed files with 101 additions and 15 deletions
--- a/README.md
+++ b/README.md
@ -58,6 +58,7 @@ The following tutorials are provided:
 * Google Container Engine
 	* [Using Google's Default Ingress Controller](docs/tutorials/gke.md)
 	* [Using the Nginx Ingress Controller](docs/tutorials/nginx-ingress.md)
+* [Oracle Cloud Infrastructure (OCI) DNS](docs/tutorials/oracle.md)

 ## Running Locally

--- a/docs/tutorials/oracle.md
+++ b/docs/tutorials/oracle.md
@ -58,6 +58,9 @@ rules:
 - apiGroups: ["extensions"]
  resources: ["ingresses"]
  verbs: ["get","watch","list"]
+- apiGroups: [""]
+  resources: ["nodes"]
+  verbs: ["list"]
 ---
 apiVersion: rbac.authorization.k8s.io/v1beta1
 kind: ClusterRoleBinding
@ -93,7 +96,6 @@ spec:
        - --source=ingress
        - --provider=oci
        - --policy=upsert-only # prevent ExternalDNSfrom deleting any records, omit to enable full synchronization
-        - --registry=txt
        - --txt-owner-id=my-identifier
        volumeMounts:
          - name: config
--- a/main.go
+++ b/main.go
@ -171,7 +171,11 @@ func main() {
 			},
 		)
 	case "oci":
-		p, err = provider.NewOCIProvider(cfg.OCIConfigFile, domainFilter, zoneIDFilter, cfg.DryRun)
+		var config *provider.OCIConfig
+		config, err = provider.LoadOCIConfig(cfg.OCIConfigFile)
+		if err == nil {
+			p, err = provider.NewOCIProvider(*config, domainFilter, zoneIDFilter, cfg.DryRun)
+		}
 	default:
 		log.Fatalf("unknown dns provider: %s", cfg.Provider)
 	}
--- a/pkg/apis/externaldns/types_test.go
+++ b/pkg/apis/externaldns/types_test.go
@ -94,7 +94,7 @@ var (
 		InfobloxWapiPassword:    "infoblox",
 		InfobloxWapiVersion:     "2.6.1",
 		InfobloxSSLVerify:       false,
-		OCIConfigFile:           "/etc/kubernetes/oci.yaml",
+		OCIConfigFile:           "oci.yaml",
 		InMemoryZones:           []string{"example.org", "company.com"},
 		PDNSServer:              "http://ns.example.com:8081",
 		PDNSAPIKey:              "some-secret-key",
@ -159,6 +159,7 @@ func TestParseFlags(t *testing.T) {
 				"--pdns-server=http://ns.example.com:8081",
 				"--pdns-api-key=some-secret-key",
 				"--pdns-tls-enabled",
+				"--oci-config-file=oci.yaml",
 				"--tls-ca=/path/to/ca.crt",
 				"--tls-client-cert=/path/to/cert.pem",
 				"--tls-client-cert-key=/path/to/key.pem",
@ -208,6 +209,7 @@ func TestParseFlags(t *testing.T) {
 				"EXTERNAL_DNS_INFOBLOX_WAPI_PASSWORD":     "infoblox",
 				"EXTERNAL_DNS_INFOBLOX_WAPI_VERSION":      "2.6.1",
 				"EXTERNAL_DNS_INFOBLOX_SSL_VERIFY":        "0",
+				"EXTERNAL_DNS_OCI_CONFIG_FILE":            "oci.yaml",
 				"EXTERNAL_DNS_INMEMORY_ZONE":              "example.org\ncompany.com",
 				"EXTERNAL_DNS_DOMAIN_FILTER":              "example.org\ncompany.com",
 				"EXTERNAL_DNS_PDNS_SERVER":                "http://ns.example.com:8081",
--- a/provider/oci.go
+++ b/provider/oci.go
@ -67,21 +67,25 @@ type ociDNSClient interface {
 	PatchZoneRecords(ctx context.Context, request dns.PatchZoneRecordsRequest) (response dns.PatchZoneRecordsResponse, err error)
 }

-// NewOCIProvider initialises a new OCI DNS based Provider.
-func NewOCIProvider(configFile string, domainFilter DomainFilter, zoneIDFilter ZoneIDFilter, dryRun bool) (*OCIProvider, error) {
-	contents, err := ioutil.ReadFile(configFile)
+// LoadOCIConfig reads and parses the OCI ExternalDNS config file at the given
+// path.
+func LoadOCIConfig(path string) (*OCIConfig, error) {
+	contents, err := ioutil.ReadFile(path)
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to read OCI config file %q", configFile)
+		return nil, errors.Wrapf(err, "reading OCI config file %q", path)
 	}
-	cfg := OCIConfig{}
-	err = yaml.Unmarshal(contents, &cfg)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to read OCI config file %q", configFile)
-	}
-	// TODO(apryde): validate config.

+	cfg := OCIConfig{}
+	if err := yaml.Unmarshal(contents, &cfg); err != nil {
+		return nil, errors.Wrapf(err, "parsing OCI config file %q", path)
+	}
+	return &cfg, nil
+}
+
+// NewOCIProvider initialises a new OCI DNS based Provider.
+func NewOCIProvider(cfg OCIConfig, domainFilter DomainFilter, zoneIDFilter ZoneIDFilter, dryRun bool) (*OCIProvider, error) {
 	var client ociDNSClient
-	client, err = dns.NewDnsClientWithConfigurationProvider(common.NewRawConfigurationProvider(
+	client, err := dns.NewDnsClientWithConfigurationProvider(common.NewRawConfigurationProvider(
 		cfg.Auth.TenancyID,
 		cfg.Auth.UserID,
 		cfg.Auth.Region,
@ -247,7 +251,6 @@ func (p *OCIProvider) ApplyChanges(changes *plan.Changes) error {

 // newRecordOperation returns a RecordOperation based on a given endpoint.
 func newRecordOperation(ep *endpoint.Endpoint, opType dns.RecordOperationOperationEnum) dns.RecordOperation {
-	// NOTE(apryde): works around appending a trailing dot to TXT records.
 	targets := make([]string, len(ep.Targets))
 	copy(targets, []string(ep.Targets))
 	if ep.RecordType == endpoint.RecordTypeCNAME {
--- a/provider/oci_test.go
+++ b/provider/oci_test.go
@ -122,6 +122,80 @@ func validateOCIZones(t *testing.T, actual, expected map[string]*dns.ZoneSummary
 	}
 }

+func TestNewOCIProvider(t *testing.T) {
+	testCases := map[string]struct {
+		config OCIConfig
+		err    error
+	}{
+		"valid": {
+			config: OCIConfig{
+				Auth: OCIAuthConfig{
+					TenancyID:   "ocid1.tenancy.oc1..aaaaaaaaxf3fuazosc6xng7l75rj6uist5jb6ken64t3qltimxnkymddqbma",
+					UserID:      "ocid1.user.oc1..aaaaaaaahx2vpvm4of5nqq3t274ike7ygyk2aexvokk3gyv4eyumzqajcrvq",
+					Region:      "us-ashburn-1",
+					Fingerprint: "48:ba:d4:21:63:53:db:10:65:20:d4:09:ce:01:f5:97",
+					PrivateKey: `-----BEGIN RSA PRIVATE KEY-----
+MIIEowIBAAKCAQEAv2JspZyO14kqcO/X4iz3ZdcyAf1GQJqYsBb6wyrlU0PB9Fee
+H23/HLtMSqeqo+2KQHmdV1OHFQ/S6tx7zcBaby/+2b+z3/gJO4PGxohe2812AJ/J
+W8Fp/4EnwbaRqDhoLN7ms0/e566zE3z40kCSW0NAIzv/F+0nNaka1xrypBqzvaNm
+N49dAGvqWRpzFFUb8CbvKmgE6c/H4a2zVNW3G7/K6Og4HQGeEP3NKSVvi0BiQlvd
+tVJTg7084kKcrngsS2N3qI3pzsr5wgpzPPefuPHWRKokZ20kpu8tXdFt+mAC2NHh
+eWbtY3jsR6JFaXCyZLMXInwDvRgdP0T5+uh8WwIDAQABAoIBAG0rr94omDLKw7L4
+naUfEWC+iIAqAdEIXuDTuudpqLb+h7zh3gj/re6tyK8tRWGNNrfgp6gQtZWGGUJv
+0w9jEjMqpa2AdRLlYh7Y5KKLV9D6Or3QaAQ3KEffXNZbVmsnAgXWgLL4dKakOPJ8
+71LAEryMeCGhL7puRVeOxwi9Dnwc4pcloimdggw/uwVHMK9eY5ylyt5ziiiWfhAo
+cnNJNPHRSTqSiCoEhk/8BLZT5gxf1YX0hVSEdQh2WNyxmPmVSC9uuzKOqcEBfHf5
+hmLnsUET1REM9IxCLqC9ebW263lIO/KdGiCu+YgIdwIi3wrLhaKXAZQmp4oMvWlE
+n5eYlcECgYEA5AhctPWCQBCJhcD39pSWgnSq1O9bt8yQi2P2stqlxKV9ZBepCK49
+OT42OYPUgWn7/y//6/LLzsPY58VTDHF3xZN1qu+fU0IM22D3Jqc19pnfVEb6TXSc
+0jJIiaYCWTdqRQ4p2DuDcI+EzRB+V1Z7tFWxshZWXwNvtMXNoYPOYaUCgYEA1ttn
+R3pCuGYJ5XbBwPzD5J+hvdZ6TQf8oTDraUBPxjtFOr7ea42T6KeYRFvnK2AQDnKL
+Mw3I55lNO4I2W9gahUFG28dhxEuxeyvXGqXEJvPCUYePstab/BkUrm7/jkS3CLcJ
+dlRXjqOfGwi5+NPUZMoOkZ54ZR4ZpdhIAeEpBf8CgYEAyMyMRlVCowNs9jkcoSfq
+Wme3O8BhvI9/mDCZnCfNHC94Bvtn1U/WF7uBOuPf35Ch05PQAiHa8WOBVn/bZ+l
+ZngZT7K+S+SHyc6zFHh9zm9k96Og2f/r8DSTJ5Ll0oY3sCNuuZh+f+oBeUoi1umy
+PPVDAsbd4NhJIBiOO4GGHkCgYA1p4i9Es0Cm4ixItzzwqtwtmR/scXM4se1wS+o
+kwTY7gg1yWBl328mVGPz/jdWX6Di2rvkPfcDzwa4a6YDfY3x5QE69Sl3CagCqEoJ
+P4giahEGpyG9eVZuuBywCswKzSIgLQVR5XIQDtA2whEfEFcj7EmDF93c8o1ZGw+w
+WHgUJQKBgEXr0HgxGG+v8bsXdrJ87Avx/nuA2rrFfECDPa4zuPkEK+cSFibdAq/H
+u6OIV+z59AD2s84gxR+KLzEDfQAqBt7cVA5ZH6hrO+bkCtK9ycLL+koOuB+1EV+Y
+hKRtDhmSdWBo3tJK12RrAe4t7CUe8gMgTvU7ExlcA3xQkseFPx9K
+-----END RSA PRIVATE KEY-----
+`,
+				},
+			},
+		},
+		"invalid": {
+			config: OCIConfig{
+				Auth: OCIAuthConfig{
+					TenancyID:   "ocid1.tenancy.oc1..aaaaaaaaxf3fuazosc6xng7l75rj6uist5jb6ken64t3qltimxnkymddqbma",
+					UserID:      "ocid1.user.oc1..aaaaaaaahx2vpvm4of5nqq3t274ike7ygyk2aexvokk3gyv4eyumzqajcrvq",
+					Region:      "us-ashburn-1",
+					Fingerprint: "48:ba:d4:21:63:53:db:10:65:20:d4:09:ce:01:f5:97",
+					PrivateKey: `-----BEGIN RSA PRIVATE KEY-----
+`,
+				},
+			},
+			err: errors.New("initialising OCI DNS API client: can not create client, bad configuration: PEM data was not found in buffer"),
+		},
+	}
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			_, err := NewOCIProvider(
+				tc.config,
+				NewDomainFilter([]string{"com"}),
+				NewZoneIDFilter([]string{""}),
+				false,
+			)
+			if err == nil {
+				require.NoError(t, err)
+			} else {
+				require.Equal(t, tc.err.Error(), err.Error())
+			}
+		})
+	}
+}
+
 func TestOCIZones(t *testing.T) {
 	testCases := []struct {
 		name         string