sync restrictedCIDRs with install-config #74

sanchezl · 2018-10-16T18:31:49Z

Enables the cluster-kube-apiserver-operator to receive notifications of changes to the cluster-config-v1 ConfigMap in the kube-system namespace, from which some CIDRs are extracted, i.e:

data:
  install-config:
    networking:
    podCIDR: 10.2.0.0/16
    serviceCIDR: 10.3.0.0/16

Adds the discovered CIDRs to the restrictedCIDRs array to the cluster kube apiserver's config. e.g.:

admissionPluginConfig:
  openshift.io/RestrictedEndpointsAdmission:
    configuration:
      restrictedCIDRs:
        - 10.3.0.0/16 # ServiceCIDR
        - 10.2.0.0/16 # ClusterCIDR

Resolves #50

deads2k · 2018-10-16T18:37:39Z

Towards resolving #50

give me a better description

@sanchezl WIP pulls should have [WIP] in the description so they can't auto-merge. please add it

deads2k · 2018-10-16T18:38:33Z

pkg/operator/observe_config.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"gopkg.in/yaml.v2"


imports go

core library

not core, not kube, not openshift

kube

openshift.

I've notice use of gopkg.in/yaml.v2 and github.com/ghodss/yaml in the openshift/* repositories. Any guidance on when to pick one over another?

deads2k · 2018-10-16T18:38:50Z

pkg/operator/observe_config.go

@@ -25,6 +26,7 @@ import (

 	operatorconfigclientv1alpha1 "github.com/openshift/cluster-kube-apiserver-operator/pkg/generated/clientset/versioned/typed/kubeapiserver/v1alpha1"
 	operatorconfiginformerv1alpha1 "github.com/openshift/cluster-kube-apiserver-operator/pkg/generated/informers/externalversions/kubeapiserver/v1alpha1"
+	installer "github.com/openshift/installer/pkg/types"


no. You may not import this package into the project.

pkg/operator/observe_config.go

deads2k · 2018-10-16T18:40:53Z

pkg/operator/observe_config.go

+		return fmt.Errorf("error retieving cluster config: %s", err)
+	}
+
+	// get install-config from cluster config


seriously? Delete useless comment.

deads2k · 2018-10-16T18:41:23Z

pkg/operator/observe_config.go

@@ -86,6 +88,45 @@ func (c ConfigObserver) sync() error {
 		unstructured.SetNestedStringSlice(observedConfig, etcdURLs, "storageConfig", "urls")
 	}

+	// observe configuration from cluster-config-v1


make this comment useful. You're collecting some CIDR from the clusterconfig.

deads2k · 2018-10-16T18:44:13Z

pkg/operator/observe_config.go

+	}
+
+	// set observed values
+	//  admissionPluginConfig:


wow. One the one hand I'm glad I now know. On the other hand, that's going to be a bitch to maintain.

@squeed @knobunc does anyone use this admission plugin to control ranges other than these two CIDRs? Should they be able to?

Not that I know of... but who knows what people have configured.

deads2k · 2018-10-16T18:45:33Z

notice how NewConfigObserver is called. Go to its callsite and wire through an informer that watches the kube-system namespace. Then update NewConfigObserver to add an event handler for updates to configmaps from that namespace.

deads2k · 2018-10-16T18:45:52Z

/retest

deads2k · 2018-10-16T21:36:21Z

pkg/operator/observe_config.go

+	//       podCIDR: 10.2.0.0/16
+	//       serviceCIDR: 10.3.0.0/16
+	restrictedCIDRs := []string{}
+	if networking, ok := installConfig["networking"].(map[string]string); ok {


I would expect this to always panic. Please add a test.

I would expect this to always panic. Please add a test.

err, always fail. Is it really particular?

deads2k · 2018-10-16T21:37:13Z

pkg/operator/observe_config.go

+	//       serviceCIDR: 10.3.0.0/16
+	restrictedCIDRs := []string{}
+	if networking, ok := installConfig["networking"].(map[string]string); ok {
+		if cidr, ok := networking["podCIDR"]; ok && cidr != "" {


ok is unnecessary to check here, right?

deads2k · 2018-10-16T21:37:35Z

pkg/operator/observe_config.go

+	//	    - 10.3.0.0/16 # ServiceCIDR
+	//	    - 10.2.0.0/16 # ClusterCIDR
+	// TODO should I just skip this if no CIDRs are found?
+	unstructured.SetNestedStringSlice(observedConfig, restrictedCIDRs,


only do this if you have more than zero restricted CIDRs

deads2k · 2018-10-16T21:38:56Z

Think of a way to factor this that makes sense for to extend assuming we observe about 10 values. Don't spend more than an hour.....

And let me know if you're stuck on the wiring bit.

squeed · 2018-10-16T22:08:12Z

FYI, I will need to refactor the configuration object to support multiple ClusterCIDRs.

deads2k · 2018-10-17T13:09:06Z

FYI, I will need to refactor the configuration object to support multiple ClusterCIDRs.

@squeed when that happens, please give us a heads up.

deads2k · 2018-10-17T15:53:46Z

pkg/operator/observe_config.go

+	if observedConfig, err = c.observeEtcdEndpoints(observedConfig); err != nil {
+		return err
+	}
+


deads2k · 2018-10-17T15:54:34Z

pkg/operator/observe_config.go

+	observedConfig := map[string]interface{}{}
+	var err error
+
+	if observedConfig, err = c.observeEtcdEndpoints(observedConfig); err != nil {


we don't do this. If you are assigning to a value that escapes the block, you don't do it in the block.

deads2k · 2018-10-18T14:50:35Z

pkg/operator/observe_config_test.go

+			t.Fatal("expected 2 entries in restrictedCIDRs")
+		}
+		for _, cidr := range []string{podCIDR, serviceCIDR} {
+			if func() int {


deads2k · 2018-10-18T14:52:14Z

pkg/operator/starter.go

 		operatorConfigClient.KubeapiserverV1alpha1(),
 		kubeClient.AppsV1(),
 		kubeClient.CoreV1(),
 		kubeClient.RbacV1(),
 	)

-	kubeInformersEtcdNamespaced := informers.NewFilteredSharedInformerFactory(kubeClient, 10*time.Minute, etcdNamespaceName, nil)
+	kubeInformersForKubeSystemNamespace := informers.NewSharedInformerFactoryWithOptions(kubeClient, 10*time.Minute, informers.WithNamespace("kube-system"))


construct all in the same spot (line 35ish) please

deads2k · 2018-10-18T17:20:28Z

pkg/operator/observe_config_test.go

+		t.Fatal(err)
+	}
+	if restrictedCIDRs[0] != podCIDR {
+		t.Fail()


print me out hte actual value in the last two conditions.

deads2k · 2018-10-18T17:43:11Z

pkg/operator/observe_config_test.go

+			Namespace: "kube-system",
+		},
+		Data: map[string]string{
+			"install-config": fmt.Sprintf("networking:\n  podCIDR: %s\n  serviceCIDR: %s\n", podCIDR, serviceCIDR),


just saw this was missed. Just use plus signs

deads2k · 2018-10-18T17:43:31Z

pkg/operator/observe_config_test.go

+)
+
+func TestObserveClusterConfig(t *testing.T) {
+


and while you're updating, you need to kill your whitespace bunny that keeps putting these here.

deads2k · 2018-10-18T18:01:29Z

/lgtm

sttts · 2018-10-19T07:50:21Z

/lgtm
/approve

Prow had issues.

sttts · 2018-10-19T08:24:53Z

@sanchezl we need the same during the bootstrapping phase via the render command. Can you find out whether we have access to the cluster config already at that stage? This code is executed at that point: https://github.com/openshift/installer/blob/master/pkg/asset/ignition/bootstrap/content/bootkube.go#L31

It's a text/template. So maybe we have the cluster config in one form or another around to be plugged in, or even as file on the bootstrap node.

deads2k · 2018-10-19T13:49:44Z

rebase requried

deads2k · 2018-10-23T15:32:59Z

/lgtm

openshift-ci-robot · 2018-10-23T15:33:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sanchezl, sttts

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~~ [deads2k,sttts]

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

openshift-ci-robot requested review from deads2k and sttts October 16, 2018 18:31

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 16, 2018

deads2k reviewed Oct 16, 2018

View reviewed changes

pkg/operator/observe_config.go Show resolved Hide resolved

deads2k reviewed Oct 16, 2018

View reviewed changes

pkg/operator/observe_config.go Show resolved Hide resolved

deads2k reviewed Oct 16, 2018

View reviewed changes

sanchezl changed the title ~~sync restrictedCIDRs with install-config~~ [WIP] sync restrictedCIDRs with install-config Oct 16, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2018

sanchezl force-pushed the sync_restricted_cidrs branch 3 times, most recently from 1c51f55 to df19478 Compare October 16, 2018 20:33

deads2k reviewed Oct 16, 2018

View reviewed changes

sanchezl force-pushed the sync_restricted_cidrs branch 2 times, most recently from 36db284 to 90ffa2a Compare October 17, 2018 15:46

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 17, 2018

deads2k reviewed Oct 17, 2018

View reviewed changes

deads2k reviewed Oct 18, 2018

View reviewed changes

sanchezl force-pushed the sync_restricted_cidrs branch 4 times, most recently from 430b6e9 to acc7d7d Compare October 18, 2018 15:39

deads2k reviewed Oct 18, 2018

View reviewed changes

sanchezl force-pushed the sync_restricted_cidrs branch from acc7d7d to 210fa55 Compare October 18, 2018 17:32

deads2k reviewed Oct 18, 2018

View reviewed changes

sanchezl force-pushed the sync_restricted_cidrs branch 2 times, most recently from 3fcdcfc to 7df2a0c Compare October 18, 2018 17:59

openshift-ci-robot assigned sttts Oct 19, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 19, 2018

sync restrictedCIDRs with install-config

4fc7a1b

sanchezl force-pushed the sync_restricted_cidrs branch from 7df2a0c to 4fc7a1b Compare October 22, 2018 13:37

sanchezl mentioned this pull request Oct 22, 2018

initialize restrictedCIDRs during the bootstrapping phase #83

Closed

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2018

openshift-ci-robot assigned deads2k Oct 23, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2018

openshift-merge-robot merged commit 7cd79ea into openshift:master Oct 23, 2018

sttts mentioned this pull request Nov 13, 2018

Network CIDRs not observed from cluster config openshift/cluster-kube-controller-manager-operator#80

Closed

sanchezl deleted the sync_restricted_cidrs branch November 16, 2018 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync restrictedCIDRs with install-config #74

sync restrictedCIDRs with install-config #74

sanchezl commented Oct 16, 2018 •

edited

deads2k commented Oct 16, 2018

deads2k Oct 16, 2018

sanchezl Oct 16, 2018

deads2k Oct 16, 2018

deads2k Oct 16, 2018

deads2k Oct 16, 2018

deads2k Oct 16, 2018

squeed Oct 16, 2018

deads2k commented Oct 16, 2018

deads2k commented Oct 16, 2018

deads2k Oct 16, 2018

deads2k Oct 16, 2018

deads2k Oct 16, 2018

deads2k Oct 16, 2018

deads2k commented Oct 16, 2018

squeed commented Oct 16, 2018

deads2k commented Oct 17, 2018

deads2k Oct 17, 2018

deads2k Oct 17, 2018

deads2k Oct 18, 2018

deads2k Oct 18, 2018

deads2k Oct 18, 2018

deads2k Oct 18, 2018 •

edited

deads2k Oct 18, 2018

deads2k commented Oct 18, 2018

sttts commented Oct 19, 2018

sttts commented Oct 19, 2018

deads2k commented Oct 19, 2018

deads2k commented Oct 23, 2018

openshift-ci-robot commented Oct 23, 2018

sync restrictedCIDRs with install-config #74

sync restrictedCIDRs with install-config #74

Conversation

sanchezl commented Oct 16, 2018 • edited

deads2k commented Oct 16, 2018

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

deads2k commented Oct 16, 2018

deads2k commented Oct 16, 2018

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

deads2k commented Oct 16, 2018

squeed commented Oct 16, 2018

deads2k commented Oct 17, 2018

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

deads2k Oct 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 18, 2018

sttts commented Oct 19, 2018

sttts commented Oct 19, 2018

deads2k commented Oct 19, 2018

deads2k commented Oct 23, 2018

openshift-ci-robot commented Oct 23, 2018

sanchezl commented Oct 16, 2018 •

edited

deads2k Oct 18, 2018 •

edited