Add support for pulling config from a directory #39

liggitt · 2017-01-24T16:28:56Z

now that optional configmaps are possible, kube-dns should support pulling config from a mounted directory

this PR refactors the existing API-based configmap source and adds a directory-based source

c.f.
kubernetes/kubernetes#36775 (comment)
kubernetes/kubernetes#38816
kubernetes/kubernetes#39981

liggitt · 2017-01-24T16:39:45Z

cc @thockin @MrHohn

GheRivero

small nit

GheRivero · 2017-01-24T17:29:08Z

cmd/kube-dns/app/server.go

+		glog.V(0).Infof("Using configuration read from directory: %v", config.ConfigDir, config.ConfigPeriod)
+		syncSource := dnsconfig.NewFileSyncSource(config.ConfigDir, config.ConfigPeriod)
+		configSync = dnsconfig.NewSync(syncSource)
+	default:
 		glog.V(0).Infof("ConfigMap not configured, using values from command line flags")


Needs to be updated to inform that ConfigDir is neither configured

liggitt · 2017-01-24T19:16:34Z

added tests for the file-based config

bowei

thanks for taking care of this

bowei · 2017-01-24T19:54:52Z

cmd/kube-dns/app/server.go

-			config.ConfigMapNs, config.ConfigMap)
-		configSync = dnsconfig.NewSync(
-			kubeClient, config.ConfigMapNs, config.ConfigMap)
+	switch {


add

if config.ConfigMap != "" && config.ConfigDir != "" { glog.V(0).Warnf("Both ConfigMap and ConfigDir set...") }

added check for both below

bowei · 2017-01-24T19:55:06Z

cmd/kube-dns/app/server.go

-		configSync = dnsconfig.NewSync(
-			kubeClient, config.ConfigMapNs, config.ConfigMap)
+	switch {
+	case config.ConfigMap != "":


Do we want to prefer ConfigDir in this case?

actually, I'd error... they need to pick one

bowei · 2017-01-24T19:55:54Z

cmd/kube-dns/app/options/options.go

@@ -166,4 +172,9 @@ func (s *KubeDNSConfig) AddFlags(fs *pflag.FlagSet) {
 			"dynamically adjustable configuration.")
 	fs.DurationVar(&s.InitialSyncTimeout, "initial-sync-timeout", s.InitialSyncTimeout,
 		"Timeout for initial resource sync.")
+
+	fs.StringVar(&s.ConfigDir, "config-dir", s.ConfigDir,
+		"directory to read config values from.")


add something like ". Only one of config-map or config-dir should be set"

bowei · 2017-01-24T19:57:00Z

pkg/dns/config/sync.go

+	Once() (SyncResult, error)
+	Periodic() <-chan SyncResult
+}
+
 // NewSync for ConfigMap from namespace `ns` and `name`.


this comment needs to be updated

bowei · 2017-01-24T19:58:12Z

pkg/dns/config/sync.go

+	Data    map[string]string
+}
+
+type SyncSource interface {


Seems like the same as Sync interface above? Why have another layer here?

source just gets the data (from the API or from the disk)

that's logically distinct from deduping on version, parsing and validating that data into a config

Can we make SyncSource and SyncResult package private in that case

no, they get returned and used by the app package

I think SyncResult for sure does not need to be exposed. I just tried it with your patch and there isn't an issue.

It would be preferable to just have just expose NewConfigMapSync and NewFileSync constructors for SYnc and hide the SyncSource and SyncResult from outside altogether.

bowei · 2017-01-24T19:58:58Z

pkg/dns/config/sync_api.go

@@ -0,0 +1,120 @@
+/*


can we name this sync_configmap.go? sync_api would refer to "the sync API" in my mind

liggitt · 2017-01-24T20:26:51Z

comments addressed

liggitt · 2017-01-24T23:22:55Z

It would be preferable to just have just expose NewConfigMapSync and NewFileSync constructors for Sync and hide the SyncSource and SyncResult from outside altogether.

sounds good, updated.

MrHohn · 2017-01-24T23:33:16Z

pkg/dns/config/sync_dir_test.go

+
+	// should return error if non-utf8 data is encountered
+	// https://en.wikipedia.org/wiki/UTF-8#Codepage_layout
+	if err := ioutil.WriteFile(filepath.Join(testDir, "binary"), []byte{192}, os.FileMode(0755)); err != nil {


Curious, does data in ConfigMap only contain UTF-8? I used to think that is only true for key, which got reflected as path here.

it's map[string]string, not map[string][]byte, so it should only be utf-8 data

MrHohn · 2017-01-24T23:37:36Z

pkg/dns/config/sync_dir_test.go

+		t.Fatalf("expected no error, got: %v", err)
+	}
+	expectedResult := syncResult{
+		Version: "d56592d659b731bdf7979b45644cba0ce80cc672ec7d30899757b4ad892c7360",


Is it possible to get this hash programmably?

MrHohn · 2017-01-25T18:12:22Z

Thanks for the helps! LGTM @bowei

liggitt · 2017-01-25T18:51:56Z

thanks! what's the timeframe or process for getting a version of the image made with this?

bowei · 2017-01-25T18:55:13Z

uploading right now...

bowei · 2017-01-25T18:56:46Z

I have plans to make the releases automated, but haven't gotten around to it. There are some issues with automation google_containers auth tokens which I have to resolve.

bowei · 2017-01-25T19:20:46Z

gcr.io/google_containers/k8s-dns-dnsmasq-amd64:1.12.0
gcr.io/google_containers/k8s-dns-dnsmasq-arm:1.12.0
gcr.io/google_containers/k8s-dns-dnsmasq-arm64:1.12.0
gcr.io/google_containers/k8s-dns-dnsmasq-ppc64le:1.12.0
gcr.io/google_containers/k8s-dns-kube-dns-amd64:1.12.0
gcr.io/google_containers/k8s-dns-kube-dns-arm:1.12.0
gcr.io/google_containers/k8s-dns-kube-dns-arm64:1.12.0
gcr.io/google_containers/k8s-dns-kube-dns-ppc64le:1.12.0
gcr.io/google_containers/k8s-dns-sidecar-amd64:1.12.0
gcr.io/google_containers/k8s-dns-sidecar-arm:1.12.0
gcr.io/google_containers/k8s-dns-sidecar-arm64:1.12.0
gcr.io/google_containers/k8s-dns-sidecar-e2e-amd64:1.12.0
gcr.io/google_containers/k8s-dns-sidecar-e2e-arm:1.12.0
gcr.io/google_containers/k8s-dns-sidecar-e2e-arm64:1.12.0
gcr.io/google_containers/k8s-dns-sidecar-e2e-ppc64le:1.12.0
gcr.io/google_containers/k8s-dns-sidecar-ppc64le:1.12.0

Automatic merge from submit-queue (batch tested with PRs 40382, 41060) Make kube-dns mount optional configmap Switches add-on templates to use an optional mounted configmap for dns Uses options added in kubernetes/dns#39 Blocks #38816

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 24, 2017

GheRivero reviewed Jan 24, 2017

View reviewed changes

liggitt force-pushed the configmap branch 2 times, most recently from 3e122b9 to ba472f4 Compare January 24, 2017 18:41

bowei self-assigned this Jan 24, 2017

bowei reviewed Jan 24, 2017

View reviewed changes

liggitt mentioned this pull request Jan 24, 2017

Make kube-dns mount optional configmap kubernetes/kubernetes#40382

Merged

liggitt force-pushed the configmap branch from ba472f4 to a774261 Compare January 24, 2017 20:26

Refactor configmap API source

55b3583

liggitt force-pushed the configmap branch from a774261 to c59f4dd Compare January 24, 2017 22:56

MrHohn reviewed Jan 24, 2017

View reviewed changes

Add directory-based config source

44f5d30

liggitt force-pushed the configmap branch from c59f4dd to 44f5d30 Compare January 25, 2017 05:40

bowei approved these changes Jan 25, 2017

View reviewed changes

bowei merged commit 8a21972 into kubernetes:master Jan 25, 2017

liggitt deleted the configmap branch January 26, 2017 21:44

cmluciano mentioned this pull request Feb 13, 2017

Custom DNS entries for kube-dns kubernetes/kubernetes#41328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for pulling config from a directory #39

Add support for pulling config from a directory #39

liggitt commented Jan 24, 2017 •

edited

Loading

liggitt commented Jan 24, 2017

GheRivero left a comment

GheRivero Jan 24, 2017

liggitt Jan 24, 2017

liggitt commented Jan 24, 2017

bowei left a comment

bowei Jan 24, 2017

liggitt Jan 24, 2017

bowei Jan 24, 2017

liggitt Jan 24, 2017

bowei Jan 24, 2017

bowei Jan 24, 2017

bowei Jan 24, 2017

liggitt Jan 24, 2017

bowei Jan 24, 2017

liggitt Jan 24, 2017

bowei Jan 24, 2017

bowei Jan 24, 2017

liggitt commented Jan 24, 2017

liggitt commented Jan 24, 2017

MrHohn Jan 24, 2017

liggitt Jan 24, 2017

MrHohn Jan 24, 2017

liggitt Jan 25, 2017

MrHohn commented Jan 25, 2017

liggitt commented Jan 25, 2017

bowei commented Jan 25, 2017

bowei commented Jan 25, 2017

bowei commented Jan 25, 2017

Add support for pulling config from a directory #39

Add support for pulling config from a directory #39

Conversation

liggitt commented Jan 24, 2017 • edited Loading

liggitt commented Jan 24, 2017

GheRivero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jan 24, 2017

bowei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jan 24, 2017

liggitt commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn commented Jan 25, 2017

liggitt commented Jan 25, 2017

bowei commented Jan 25, 2017

bowei commented Jan 25, 2017

bowei commented Jan 25, 2017

liggitt commented Jan 24, 2017 •

edited

Loading