Hollow out our ConfigMaps. #3047

mattmoor · 2019-01-31T05:33:08Z

This change hollows out the ConfigMaps that we use with Knative
in order to walk a very fine line that enables us to deploy
knative/serving via ko or kubectl apply while still preserving
edits made to the serving ConfigMaps. This fine line is dancing
around the way the kubectl apply annotation
kubectl.kubernetes.io/last-applied-configuration is used to update
resources.

Essentially, this annotation is attached with the body of the current
apply, and subsequent applications take it upon themselves to remove
keys that were present in the prior apply, but not in the current apply.

By reserving all of our ConfigMap keys to be specified by the user
via kubectl edit cm -nknative-serving config-foo, which does not
update the annotation, we ensure that subsequent applies won't remove
the keys because those keys were never a part of the previous apply.

One of the key scenarios this unlocks is the ability to preserve
users' domain settings in config-domain.yaml across applies.

Fixes: #2668

This is WIP until the supporting change knative/pkg#258 is merged and this is updated to pull from knative/pkg.

knative-prow-robot · 2019-01-31T05:33:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor

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~~ [mattmoor]

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

pkg/reconciler/v1alpha1/revision/revision_test.go

mattmoor · 2019-01-31T05:36:30Z

pkg/reconciler/v1alpha1/revision/config/network.go

@@ -71,6 +71,7 @@ func NewNetworkFromConfigMap(configMap *corev1.ConfigMap) (*Network, error) {
 	nc := &Network{}
 	if ipr, ok := configMap.Data[IstioOutboundIPRangesKey]; !ok {
 		// It is OK for this to be absent, we will elide the annotation.
+		nc.IstioOutboundIPRanges = "*"


@mdemirhan Does this make sense?

cc @tcnghia

Looks correct to me.

and empty have different meanings. From the config map:

# If omitted or set to "", value of global.proxy.includeIPRanges # provided at Istio deployment time is used. In default Knative serving # deployment, global.proxy.includeIPRanges value is set to "*".

will intercept everything. Empty will default to Istio's installation defaults, which is vendor specific.

I think it is ok to change this behavior, given that we will soon remove this entire thing and replace is with proper Egress rules from Istio (hopefully the change will be in soon to enable us that). But for clarity, we should change the documentation in the config map and mention that both *, omitted or "" behaves the same way.

mattmoor · 2019-01-31T05:42:00Z

@mdemirhan and @tcnghia I'd like to get this blessed by both of you because there may be subtle changes to the logging/metrics or networking/istio configurations, which I'd like to have you sanity check for your respective areas.

Also, if you have any light to shed on my "Note to self" I'd appreciate it. I will take a deeper look in the morning with fresh eyes that can hopefully see through the sea of IPv4 :)

pkg/reconciler/testing/configmap.go

knative-metrics-robot · 2019-01-31T15:06:53Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/autoscaler/config.go	90.0%	95.8%	5.8
pkg/gc/config.go	100.0%	84.6%	-15.4
pkg/reconciler/v1alpha1/clusteringress/config/istio.go	95.8%	96.2%	0.3
pkg/reconciler/v1alpha1/revision/config/controller.go	92.9%	100.0%	7.1
pkg/reconciler/v1alpha1/revision/revision.go	91.7%	90.8%	-0.9

mattmoor · 2019-01-31T15:12:10Z

Removing WIP, but I want multiple sets of eyeballs on this so adding a
/hold

mattmoor · 2019-01-31T15:18:38Z

Fixed the coverage dip in pkg/gc/config.go.

mattmoor · 2019-01-31T16:12:46Z

e2e failure looks legit, will investigate and push a fix.

vagababov

/lgtm

pkg/logging/config_test.go

This change hollows out the ConfigMaps that we use with Knative in order to walk a very fine line that enables us to deploy knative/serving via `ko` or `kubectl apply` while still preserving edits made to the serving ConfigMaps. This fine line is dancing around the way the `kubectl apply` annotation `kubectl.kubernetes.io/last-applied-configuration` is used to update resources. Essentially, this annotation is attached with the body of the current apply, and subsequent applications take it upon themselves to remove keys that were present in the prior apply, but not in the current apply. By reserving all of our ConfigMap keys to be specified by the user via `kubectl edit cm -nknative-serving config-foo`, which does not update the annotation, we ensure that subsequent applies won't remove the keys because those keys were never a part of the previous apply. One of the key scenarios this unlocks is the ability to preserve users' domain settings in `config-domain.yaml` across applies. Fixes: knative#2668

vagababov · 2019-02-01T06:50:04Z

/lgtm

tcnghia · 2019-02-01T17:30:06Z

config/999-cache.yaml

@@ -23,14 +23,3 @@ spec:
  # This is the Go import path for the binary that is containerized
  # and substituted here.
  image: github.com/knative/serving/cmd/queue
---
-apiVersion: caching.internal.knative.dev/v1alpha1


can you please also explain this change in the description? this isn't a config map change.

I removed this because it was removed from the observability ConfigMap.

tcnghia · 2019-02-01T17:34:04Z

pkg/reconciler/v1alpha1/clusteringress/config/istio.go

@@ -38,6 +37,17 @@ const (
 	LocalGatewayKeyPrefix = "local-gateway."
 )

+var (
+	defaultGateway = Gateway{


Can we add instructions somewhere describing the new way of adding new config entries? Look like from now on we will need to make changes in the code to do this?

I wonder if instead we should provide a set of "default" configmap which we can automatically patch post installation? That would make it easier to add new config key.

After I wrote this I don't know if that's even possible given what we want (default config won't be able to be migrated).

However you may know a way so I'll leave the comment here for you to decide.

We need Operator docs on how to configure things in general. I believe some of this is documented (e.g. custom domains), but the ConfigMaps are just one class of knobs here.

tcnghia · 2019-02-01T17:40:23Z

config/config-network.yaml

@@ -53,4 +58,4 @@ data:
  # For more information, visit
  # https://istio.io/docs/tasks/traffic-management/egress/
  #
-  istio.sidecar.includeOutboundIPRanges: "*"
+  # istio.sidecar.includeOutboundIPRanges: "*"


Does commenting these out result in them not being parsed in unit tests? I worry that these get stale pretty soon.

Another (usability?) problem here is that when people does kubectl edit they probably won't see these commented out values?

I was wondering if we can prefix these with something "special" (hack?) like underscore so they still show when user does kubectl edit cm. If we do that we can also remove the defaulting logic that scatter through the code. What do you think?

I opened #3066 to track the idea we discussed offline.

mdemirhan · 2019-02-01T22:26:14Z

config/config-logging.yaml

@@ -19,36 +19,41 @@ metadata:
  namespace: knative-serving
  labels:
    serving.knative.dev/release: devel
-data:
+


This configuration file is mainly for vendors and I don't think customers need to make any changes here or if we care to preserve their changes. Should we keep the original form of this?

Discussed on chat. Vendors can still modify these files in releases, if they want to specify values that users shouldn't override (and each apply will clobber any user changes). If they want to let users configure them then they can omit them as we do here.

mdemirhan · 2019-02-01T22:28:43Z

config/config-observability.yaml


  # The revision log url template, where ${REVISION_UID} will be replaced by the actual
  # revision uid. For the url to work you'll need to set up monitoring and have access to
  # the kibana dashboard using `kubectl proxy`.
-  logging.revision-url-template: |
-    http://localhost:8001/api/v1/namespaces/knative-monitoring/services/kibana-logging/proxy/app/kibana#/discover?_a=(query:(match:(kubernetes.labels.knative-dev%2FrevisionUID:(query:'${REVISION_UID}',type:phrase))))
+  # logging.revision-url-template: |


This is also something vendor specific and each vendor will probably want to put their own values here rather than letting customer override it.

If vendors don't want this specified, then they can set it and kubectl apply will start to overwrite it.

mdemirhan · 2019-02-01T22:54:16Z

pkg/reconciler/v1alpha1/revision/config/network.go

@@ -71,6 +71,7 @@ func NewNetworkFromConfigMap(configMap *corev1.ConfigMap) (*Network, error) {
 	nc := &Network{}
 	if ipr, ok := configMap.Data[IstioOutboundIPRangesKey]; !ok {
 		// It is OK for this to be absent, we will elide the annotation.
+		nc.IstioOutboundIPRanges = "*"


and empty have different meanings. From the config map:

# If omitted or set to "", value of global.proxy.includeIPRanges # provided at Istio deployment time is used. In default Knative serving # deployment, global.proxy.includeIPRanges value is set to "*".

will intercept everything. Empty will default to Istio's installation defaults, which is vendor specific.

I think it is ok to change this behavior, given that we will soon remove this entire thing and replace is with proper Egress rules from Istio (hopefully the change will be in soon to enable us that). But for clarity, we should change the documentation in the config map and mention that both *, omitted or "" behaves the same way.

mattmoor · 2019-02-02T15:00:27Z

/hold cancel

This change hollows out the ConfigMaps that we use with Knative in order to walk a very fine line that enables us to deploy knative/serving via `ko` or `kubectl apply` while still preserving edits made to the serving ConfigMaps. This fine line is dancing around the way the `kubectl apply` annotation `kubectl.kubernetes.io/last-applied-configuration` is used to update resources. Essentially, this annotation is attached with the body of the current apply, and subsequent applications take it upon themselves to remove keys that were present in the prior apply, but not in the current apply. By reserving all of our ConfigMap keys to be specified by the user via `kubectl edit cm -nknative-serving config-foo`, which does not update the annotation, we ensure that subsequent applies won't remove the keys because those keys were never a part of the previous apply. One of the key scenarios this unlocks is the ability to preserve users' domain settings in `config-domain.yaml` across applies. Fixes: knative#2668

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2019

knative-prow-robot requested review from josephburnett, lichuqiang, markusthoemmes, mdemirhan and vagababov January 31, 2019 05:33

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 31, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2019

mattmoor commented Jan 31, 2019

View reviewed changes

pkg/reconciler/v1alpha1/revision/revision_test.go Outdated Show resolved Hide resolved

mattmoor commented Jan 31, 2019

View reviewed changes

mattmoor assigned tcnghia and mdemirhan Jan 31, 2019

vagababov reviewed Jan 31, 2019

View reviewed changes

pkg/reconciler/testing/configmap.go Outdated Show resolved Hide resolved

mattmoor force-pushed the hollow-configmaps branch from 9ecf609 to 4bbf071 Compare January 31, 2019 15:04

mattmoor force-pushed the hollow-configmaps branch from 4bbf071 to a7233d2 Compare January 31, 2019 15:11

mattmoor changed the title ~~[WIP] Hollow out our ConfigMaps.~~ Hollow out our ConfigMaps. Jan 31, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2019

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2019

mattmoor force-pushed the hollow-configmaps branch from a7233d2 to eeb6ce0 Compare January 31, 2019 15:18

mattmoor force-pushed the hollow-configmaps branch 2 times, most recently from f726ef0 to 48606fa Compare February 1, 2019 06:32

knative-prow-robot assigned vagababov Feb 1, 2019

vagababov reviewed Feb 1, 2019

View reviewed changes

pkg/logging/config_test.go Outdated Show resolved Hide resolved

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2019

mattmoor force-pushed the hollow-configmaps branch from 48606fa to f89c5e4 Compare February 1, 2019 06:49

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2019

tcnghia reviewed Feb 1, 2019

View reviewed changes

mattmoor mentioned this pull request Feb 1, 2019

Persist documentation in ConfigMaps #3066

Closed

mdemirhan reviewed Feb 1, 2019

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2019

knative-prow-robot merged commit 9faddd8 into knative:master Feb 2, 2019

alculquicondor mentioned this pull request Feb 6, 2019

Fix network config patching #3114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hollow out our ConfigMaps. #3047

Hollow out our ConfigMaps. #3047

mattmoor commented Jan 31, 2019

knative-prow-robot commented Jan 31, 2019

mattmoor Jan 31, 2019

mattmoor Jan 31, 2019

tcnghia Feb 1, 2019

mdemirhan Feb 1, 2019

mattmoor commented Jan 31, 2019

knative-metrics-robot commented Jan 31, 2019

mattmoor commented Jan 31, 2019

mattmoor commented Jan 31, 2019

mattmoor commented Jan 31, 2019

vagababov left a comment

vagababov commented Feb 1, 2019

tcnghia Feb 1, 2019

mattmoor Feb 1, 2019

tcnghia Feb 1, 2019

tcnghia Feb 1, 2019

mattmoor Feb 1, 2019

tcnghia Feb 1, 2019

tcnghia Feb 1, 2019

mattmoor Feb 1, 2019

mdemirhan Feb 1, 2019

mattmoor Feb 2, 2019

mdemirhan Feb 1, 2019

mattmoor Feb 2, 2019

mdemirhan Feb 1, 2019

mattmoor commented Feb 2, 2019

Hollow out our ConfigMaps. #3047

Hollow out our ConfigMaps. #3047

Conversation

mattmoor commented Jan 31, 2019

knative-prow-robot commented Jan 31, 2019

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

mattmoor commented Jan 31, 2019

knative-metrics-robot commented Jan 31, 2019

mattmoor commented Jan 31, 2019

mattmoor commented Jan 31, 2019

mattmoor commented Jan 31, 2019

vagababov left a comment

Choose a reason for hiding this comment

vagababov commented Feb 1, 2019

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

mattmoor commented Feb 2, 2019