Introduce config metadata #1049

kyessenov · 2017-08-17T06:18:42Z

Step 2 of config update: define config metadata and config wrapper object.

Step 1 was switching to individual kinds for each config type.
Step 3 is to update config proto definitions to utilize the metadata.

codecov · 2017-08-17T06:30:22Z

Codecov Report

Merging #1049 into master will decrease coverage by 2%.
The diff coverage is 48.45%.

@@            Coverage Diff             @@
##           master    #1049      +/-   ##
==========================================
- Coverage   71.74%   69.73%   -2.01%     
==========================================
  Files          47       47              
  Lines        5422     5522     +100     
==========================================
- Hits         3890     3851      -39     
- Misses       1350     1474     +124     
- Partials      182      197      +15

Impacted Files	Coverage Δ
model/validation.go	`85.94% <ø> (-0.2%)`	⬇️
model/conversion.go	`32.43% <0%> (-19.75%)`	⬇️
test/mock/config.go	`0% <0%> (ø)`	⬆️
adapter/config/ingress/conversion.go	`78.57% <100%> (+1.87%)`	⬆️
adapter/config/crd/controller.go	`67.78% <42.42%> (-5.47%)`	⬇️
adapter/config/aggregate/config.go	`46.06% <56.52%> (-7.78%)`	⬇️
adapter/config/ingress/controller.go	`68.46% <62.96%> (-4.19%)`	⬇️
adapter/config/memory/config.go	`81.37% <79.68%> (-18.63%)`	⬇️
adapter/config/crd/client.go	`68.91% <88.23%> (-3.69%)`	⬇️
model/config.go	`92.66% <90.32%> (+0.13%)`	⬆️
... and 2 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 c4ac7cd...733b8f0. Read the comment docs.

istio-testing · 2017-08-17T20:46:12Z

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

Test name	Commit	Details	Rerun command
prow/pilot-e2e-smoketest.sh	`733b8f0`	link	`/test pilot-e2e-smoketest`

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.

* update configs * update SHA

kyessenov · 2017-08-17T20:49:50Z

validated with istio/istio#571

kyessenov · 2017-08-17T22:12:11Z

ping. PR in istio broke all PRs in pilot until this gets merged.

ayj · 2017-08-17T22:11:33Z

adapter/config/crd/controller.go

@@ -47,7 +46,8 @@ type cacheHandler struct {
 }

 // NewController creates a new Kubernetes controller for CRDs
-func NewController(client *Client, resyncPeriod time.Duration) model.ConfigStoreCache {
+// Use "" for namespace to listen for all namespace changes


nit: maybe add platform neutral equivalent to v1.NamespaceAll?

ayj · 2017-08-17T22:13:02Z

adapter/config/crd/controller.go

+			continue
+		}
+
+		if namespace != "" && namespace != item.GetObjectMeta().Namespace {


nit: s/""/v1.NamespaceAll

ayj · 2017-08-17T22:15:18Z

adapter/config/ingress/controller.go

-					Content:  rule,
-				})
-			}
+		if namespace != "" && namespace != ingress.Namespace {


nit: s/""/v1.NamespaceAll

ayj · 2017-08-17T22:18:01Z

adapter/config/ingress/conversion.go

-func encodeIngressRuleName(ingressName, ingressNamespace string, ruleNum, pathNum int) string {
-	return fmt.Sprintf("%s.%s.%d.%d", ingressNamespace, ingressName, ruleNum, pathNum)
+func encodeIngressRuleName(ingressName string, ruleNum, pathNum int) string {
+	return fmt.Sprintf("%s-%d-%d", ingressName, ruleNum, pathNum)


Why the switch from . to - as a separator?

k8s allows only - in names.

ayj · 2017-08-17T22:24:07Z

model/config.go

 	// Type is a short configuration name that matches the content message type
-	Type string
+	// (e.g. "route-rule")


Are type names normalized to non-k8s specific names? e.g. route-rule instead of route-rule.config.istio.io?

Yes, type is route-rule, API is route-rules.config.istio.io/v1alpha2.

can we move to v1beta1 ? claim beta release with alpha api version is odd.

CRDs may only support one version at a time and mixer already started using v1alpha2

I chatted with @geeknoid. Let's please change everywhere to v1beta1

Changed to v1beta1 in config design doc: https://docs.google.com/document/d/1fGZpgFWJZhNRlQoBlCW815aOFR-ewfxKhla0CcPRsBM/edit#

If we use v1beta1 then by k8s conventions, this API will provide future backwards compatibility, @geeknoid. As long as we're ok taking on the work to maintain it forever, i'm OK changing to v1beta1.

kyessenov · 2017-08-17T23:31:55Z

let me merge this to unblock the others. will address namespace.All in a subsequent PR.

rshriram · 2017-08-18T00:54:41Z

model/config.go

 			out = append(out, rule)
 		}
 	}
 	return out
 }

 func (i *istioConfigStore) DestinationPolicy(destination string, tags Tags) *proxyconfig.DestinationVersionPolicy {
-	value, exists, _ := i.Get(DestinationPolicy.Type, destination)
+	names := strings.Split(destination, ".")
+	name, namespace := "", ""


what if I have foo.lyft.com as my destination and its in istio global namespace?

we don't have a global namespace, CRDs are namespaces just like services themselves.

This is provisioning the "name" and "namespace" use from CRD like we discussed. It was already happening before under the hoods, this is just making it explicit.

rshriram

overall looks good. dont understand the need to extract namespace from dest policy

andraxylia · 2017-08-18T05:35:09Z

Is this needed for 0.2? This changes all the code, I will not be able to merge my PR anymore.

andraxylia · 2017-08-18T05:40:36Z

Why did you force merge this when e2e smoke test does not pass?

kyessenov · 2017-08-18T05:54:26Z

It passed on istio/istio against the SHA of this PR. This PR required coordinating changes across two repositories which is iffy each time.

* update configs * update SHA Former-commit-id: 96e9872

kyessenov added 6 commits August 16, 2017 21:27

revamping config metadata

451620d

fix meta config

d6cbe14

update istioctl

6d8e4a8

update configs

12b7f53

finish it

4e6298a

commit

338b1cb

kyessenov requested review from ayj, mandarjog, frankbu and rshriram August 17, 2017 06:18

fix delete command

733b8f0

kyessenov added a commit to istio/istio that referenced this pull request Aug 17, 2017

Update configs to match istio/old_pilot_repo#1049 (#571)

96e9872

* update configs * update SHA

ayj reviewed Aug 17, 2017

View reviewed changes

ayj approved these changes Aug 17, 2017

View reviewed changes

kyessenov merged commit e16f721 into istio:master Aug 17, 2017

rshriram reviewed Aug 18, 2017

View reviewed changes

kyessenov deleted the metadata branch August 18, 2017 06:53

rshriram pushed a commit to istio/istio that referenced this pull request Oct 30, 2017

Update configs to match istio/old_pilot_repo#1049 (#571)

a1ae1c5

* update configs * update SHA Former-commit-id: 96e9872

vbatts pushed a commit to vbatts/istio that referenced this pull request Oct 31, 2017

Update configs to match istio/old_pilot_repo#1049 (istio#571)

623b4d7

* update configs * update SHA Former-commit-id: 96e9872

mandarjog pushed a commit to istio/istio that referenced this pull request Nov 2, 2017

Update configs to match istio/old_pilot_repo#1049 (#571)

e1e107a

* update configs * update SHA Former-commit-id: 96e9872

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce config metadata #1049

Introduce config metadata #1049

kyessenov commented Aug 17, 2017

codecov bot commented Aug 17, 2017 •

edited

Loading

istio-testing commented Aug 17, 2017

kyessenov commented Aug 17, 2017

kyessenov commented Aug 17, 2017

ayj Aug 17, 2017

ayj Aug 17, 2017

ayj Aug 17, 2017

kyessenov Aug 17, 2017

ayj Aug 17, 2017

kyessenov Aug 17, 2017

ayj Aug 17, 2017

kyessenov Aug 17, 2017

rshriram Aug 18, 2017

kyessenov Aug 18, 2017

rshriram Aug 18, 2017

frankbu Aug 18, 2017

kyessenov Aug 18, 2017

kyessenov commented Aug 17, 2017

rshriram Aug 18, 2017

kyessenov Aug 18, 2017

kyessenov Aug 18, 2017

rshriram left a comment

andraxylia commented Aug 18, 2017

andraxylia commented Aug 18, 2017 •

edited

Loading

kyessenov commented Aug 18, 2017

Introduce config metadata #1049

Introduce config metadata #1049

Conversation

kyessenov commented Aug 17, 2017

codecov bot commented Aug 17, 2017 • edited Loading

Codecov Report

istio-testing commented Aug 17, 2017

kyessenov commented Aug 17, 2017

kyessenov commented Aug 17, 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

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

kyessenov commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram left a comment

Choose a reason for hiding this comment

andraxylia commented Aug 18, 2017

andraxylia commented Aug 18, 2017 • edited Loading

kyessenov commented Aug 18, 2017

codecov bot commented Aug 17, 2017 •

edited

Loading

andraxylia commented Aug 18, 2017 •

edited

Loading