Add logic for pilot to read ConfigMap to override MeshConfig by andraxylia · Pull Request #3916 · istio/istio

andraxylia · 2018-03-02T21:49:03Z

ayj · 2018-03-02T22:00:24Z

pilot/pkg/bootstrap/server.go

 		// Config file either wasn't specified or failed to load - use a default mesh.
-		defaultMesh := model.DefaultMeshConfig()
-		mesh = &defaultMesh
+		if _, mesh, err = GetMeshConfig(s.kubeClient, kube.IstioNamespace, kube.IstioConfigMap); err != nil {


When is the config file not specified (see below for current pilot deployment)? And when would that file read fail?

istio/install/kubernetes/templates/istio-pilot.yaml.tmpl

Lines 156 to 158 in 720be7a

volumeMounts:

- name: config-volume

mountPath: /etc/istio/config

istio/install/kubernetes/templates/istio-pilot.yaml.tmpl

Lines 179 to 181 in 720be7a

volumes:

- name: config-volume

configMap:

I think the configFile is used for VMs. In a K8s setup, it always fails, I tested it.

Pilot discovery is not run for VMs, only proxy.

You mean it can fail if the config map is not mounted ?

I really don't see how this works - if the config map is not yet ready, but the mount and the load would fail.

I agree we shouldn't use the defaults silently - but fail if config map is missing, but I'm not sure this (reading using the API) solves much.

args.Mesh.ConfigFile should default to /etc/istio/config/mesh per

istio/pilot/cmd/pilot-discovery/main.go

Line 87 in 720be7a

discoveryCmd.PersistentFlags().StringVar(&serverArgs.Mesh.ConfigFile, "meshConfig", "/etc/istio/config/mesh",

. Is that path not available in pilot-discovery container?

In fact the file was not available when running pilot from my mac. When running in a pod, the configMap gets mounted under /etc/istio/config, so there is no need to re-read the map.

The problem with the connectTimeout was in fact there are two of them in the config, one at the top level, and one in defaultConfig. Only the one at the top level is taken into account for CDS.

I can close this PR, unless we want to keep the ability of falling back on reading from a cluster config map when running pilot as a process locally. It seems to work fine.

rshriram · 2018-03-02T22:15:56Z

Are you sure you need an entire config map reading function ? We already read the global mesh config from a config map, and we are trying to clean up that bit (cc @loewenstein / @xiaolanz ).

If the issue is only about connect timeout not being respected, then this should already be covered as part of the new v1alpha2 destination rules (https://github.com/istio/api/blob/master/networking/v1alpha3/destination_rule.proto#L276) and the connect timeout option must be removed from the global config map.

I think that is what is happening (we stopped reading the connect timeout from the config map and use the one in DestRule - it has a default value). [ cc @ijsnellf / @GregHanson ]

andraxylia · 2018-03-02T22:29:53Z

This is the global config map not being read by pilot. It may have got lost in the refactoring, or maybe it was never there?

It is not just about the timeout, it is also about anything in pilot that needs to be overritten. For instance, I will use this to enable or disable validate_clusters.

Until or if we get rid of the configmap, this PR is fixing a bug.

andraxylia · 2018-03-02T22:30:51Z

For the moment, connectTimeout is also used for v1, and as I said in my previous comment this is needed in general.

For v2, we still need a value in the config map, that the user may overwrite on a per service basis, but if I need to change something globally, I do not want to create 1000 destination rules just for this. Nor do I want to change the code. BTW, inability to tune this easily stopped me from easily debugging the 503s issues.

* WIP - codecov package support * Enabling package check * Update codecov range * Update codecov requirement

* Use schema value for group and version for test. * Remove obsolete comment.

* A few enhancements * Make --output to be a directory Add --clear_cache_stats option * add port-forward support so that it's even easier to use this command outside the k8s cluster * use port-forward to download envoy config

costinm

/lgtm

This is the first step to also watch it - and it may help for pilot on VMs, where it can pull the config map from apiserver instead of having to copy it to each VM.

istio-merge-robot · 2018-03-05T20:23:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~pilot/OWNERS~~ [costinm]
~~security/OWNERS~~ [costinm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

codecov · 2018-03-05T21:32:05Z

Codecov Report

Merging #3916 into master will increase coverage by 1%.
The diff coverage is 77%.

@@           Coverage Diff           @@
##           master   #3916    +/-   ##
=======================================
+ Coverage      76%     76%    +1%     
=======================================
  Files         297     295     -2     
  Lines       27075   26961   -114     
=======================================
+ Hits        20407   20468    +61     
+ Misses       5358    5184   -174     
+ Partials     1310    1309     -1

Impacted Files	Coverage Δ
pilot/pkg/serviceregistry/kube/controller.go	`69% <ø> (+5%)`	⬆️
security/pkg/workload/server.go	`74% <ø> (ø)`	⬆️
mixer/pkg/config/crd/admit.go	`59% <ø> (ø)`	⬆️
pilot/pkg/serviceregistry/consul/conversion.go	`91% <ø> (ø)`	⬆️
pilot/pkg/kube/inject/inject.go	`80% <ø> (ø)`	⬆️
pilot/pkg/serviceregistry/kube/client.go	`30% <ø> (ø)`	⬆️
pilot/pkg/model/config.go	`61% <ø> (ø)`	⬆️
pilot/pkg/config/kube/crd/types.go	`62% <ø> (+5%)`	⬆️
pilot/pkg/serviceregistry/kube/queue.go	`86% <ø> (ø)`	⬆️
mixer/pkg/runtime/init.go	`28% <100%> (ø)`	⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 720be7a...6ac5d99. Read the comment docs.

istio-merge-robot · 2018-03-05T21:32:41Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @andraxylia @costinm

… andra-configmap

- Various changes to the e2e tests - Re-introducing old mixer docs (with many changes) for running a local docker registry in k8s.

… andra-configmap

…ded path (#3981)

…string attribute when the mixer filter was TCP previously. (#3989)

… andra-configmap

istio-testing · 2018-03-06T00:48:58Z

@andraxylia: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-unit-tests.sh	`6ac5d99`	link	`/test istio-unit-tests`

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ldemailly · 2018-03-06T06:49:55Z

can you link the replacement PR here or in #3826, tia ? (with just the change you want to make, this one seemed like it had unrelated stale commits...)

andraxylia · 2018-03-06T21:48:27Z

@ldemailly Closed this because the set of files went into a bad state after the rebase. Opened instead #4024 from exactly the same branch, but the set of files changed is correct.

Allow pilot to read ConfigMap to override connectTimeout

56984b1

andraxylia requested a review from a team March 2, 2018 21:49

googlebot added the cla: yes label Mar 2, 2018

ayj reviewed Mar 2, 2018

View reviewed changes

Fix format

6bfed3f

andraxylia requested a review from a team March 2, 2018 22:07

xiaolanz and others added 10 commits March 2, 2018 15:39

Change pilot CRD client to handle multiple apiGroupVersions (#3830)

aeff05e

Improve CertUtil. (#3918)

c509392

Revise start_driver.sh (#3923)

afd5911

Change the Infof to Debugf in KindMap() (#3925)

f434fc2

Adding requirement for codecov package (#3884)

4a27208

* WIP - codecov package support * Enabling package check * Update codecov range * Update codecov requirement

Switch CI badge to match style of other badges. [skip ci]. (#3932)

4081af6

Update TestIstioConfig (#3924)

da2219c

* Use schema value for group and version for test. * Remove obsolete comment.

init integration framework doc (#3914)

18a20f9

A few enhancements to istio-proxy-cfg.py (#3235)

be57dc6

* A few enhancements * Make --output to be a directory Add --clear_cache_stats option * add port-forward support so that it's even easier to use this command outside the k8s cluster * use port-forward to download envoy config

CA retries three times when requesting secret creation. (#3909)

46d8682

costinm approved these changes Mar 5, 2018

View reviewed changes

istio-testing assigned costinm Mar 5, 2018

istio-testing added the lgtm label Mar 5, 2018

istio-merge-robot added the approved label Mar 5, 2018

Fix goimports

1b26b7b

istio-merge-robot removed the lgtm label Mar 5, 2018

andraxylia added 2 commits March 5, 2018 13:47

Allow pilot to read ConfigMap to override connectTimeout

f0e54ad

Fix goimports

1ec916f

andraxylia added 4 commits March 5, 2018 13:53

Fix goimports

5afee26

Allow pilot to read ConfigMap to override connectTimeout

1185249

Fix goimports

3854414

Merge branch 'andra-configmap' of https://github.com/istio/istio into…

96877f1

… andra-configmap

andraxylia added the do-not-merge/hold Block automatic merging of a PR. label Mar 5, 2018

chxchx and others added 12 commits March 5, 2018 14:27

Conform to Testgrid Regex on Junit Reports (#3974)

ce0625e

Fix code coverage reporting (#3979)

a5eb925

Pilot e2e test support for local docker registry (#3920)

7ba4568

- Various changes to the e2e tests - Re-introducing old mixer docs (with many changes) for running a local docker registry in k8s.

Allow pilot to read ConfigMap to override connectTimeout

db7d2d3

Fix goimports

e44aaed

Allow pilot to read ConfigMap to override connectTimeout

4381f59

Fix goimports

4e57a96

Allow pilot to read ConfigMap to override connectTimeout

568ccf0

Fix goimports

51f9812

Allow pilot to read ConfigMap to override connectTimeout

635433e

Fix goimports

64602bb

Merge branch 'andra-configmap' of https://github.com/istio/istio into…

e831efc

… andra-configmap

andraxylia requested a review from a team March 5, 2018 23:34

smawson and others added 7 commits March 5, 2018 15:42

Fix typo in codecov.sh plus switch to use variable instead of hard-co…

d72123c

…ded path (#3981)

Always send destination IP address as bytes; we were sending it as a …

7fa9247

…string attribute when the mixer filter was TCP previously. (#3989)

Allow pilot to read ConfigMap to override connectTimeout

cf5da80

Fix goimports

2e8be09

Allow pilot to read ConfigMap to override connectTimeout

4160ec2

Fix goimports

fb1da51

Merge branch 'andra-configmap' of https://github.com/istio/istio into…

6ac5d99

… andra-configmap

andraxylia closed this Mar 6, 2018

andraxylia mentioned this pull request Mar 6, 2018

Add logic for pilot to read the istio ConfigMap #4024

Merged

	volumeMounts:
	- name: config-volume
	mountPath: /etc/istio/config

Conversation

andraxylia commented Mar 2, 2018

Uh oh!

ayj Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

andraxylia Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

costinm Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

costinm Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

ayj Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

andraxylia Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram commented Mar 2, 2018

Uh oh!

andraxylia commented Mar 2, 2018

Uh oh!

andraxylia commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Mar 5, 2018

Uh oh!

codecov bot commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

istio-merge-robot commented Mar 5, 2018

Uh oh!

istio-testing commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly commented Mar 6, 2018

Uh oh!

andraxylia commented Mar 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

19 participants

andraxylia Mar 3, 2018 •

edited

Loading

andraxylia commented Mar 2, 2018 •

edited

Loading

codecov bot commented Mar 5, 2018 •

edited

Loading

istio-testing commented Mar 6, 2018 •

edited

Loading