Add support for gtp with multiple traffic policies #162

aattuluri · 2021-01-04T16:53:28Z

No description provided.

Signed-off-by: aattuluri <anil_attuluri@intuit.com>

This reverts commit a9fa409.

…mmit sha if neither i) or ii)

josephpeacock · 2021-01-13T18:55:31Z

.circleci/config.yml

@@ -90,18 +90,18 @@ jobs:
            cd tests
            export IS_LOCAL=false
            ./run.sh "1.16.8" "1.5.7" "../out"
+#      - run:


Let's not keep commented out code

.circleci/config.yml

josephpeacock · 2021-01-13T18:58:06Z

admiral/pkg/clusters/handler.go

-	if gtpWrapper != nil {
+	processGtp := true
+	if len(locality) == 0 {
+		log.Errorf(LogErrFormat, "Process", "GlobalTrafficPolicy", host, "", "Skipping gtp processing, locality of the cluster nodes cannot be determined. Is this minikube?")


Should this be warning? Since it seems like the overall processing is able to continue

admiral/pkg/clusters/handler.go

admiral/pkg/clusters/serviceentry.go

josephpeacock · 2021-01-13T19:13:05Z

admiral/pkg/clusters/serviceentry.go

 func AddServiceEntriesWithDr(cache *AdmiralCache, sourceClusters map[string]string, rcs map[string]*RemoteController, serviceEntries map[string]*networking.ServiceEntry) {
 	syncNamespace := common.GetSyncNamespace()
 	for _, se := range serviceEntries {

-		//add service entry
-		serviceEntryName := getIstioResourceName(se.Hosts[0], "-se")
+		//Add a label


Is this comment accurate?

admiral/pkg/clusters/serviceentry.go

josephpeacock · 2021-01-13T19:22:51Z

admiral/pkg/clusters/serviceentry.go

+			var modifiedSe *networking.ServiceEntry
+			var host = se.Hosts[0]
+			var drName, seName = defaultDrName, defaultSeName
+			if gtpTrafficPolicy.Dns != env && gtpTrafficPolicy.Dns != common.Default {


Do you possible mean some other comparison here? Is it possible that the DNS = env?

josephpeacock · 2021-01-13T19:31:22Z

install/sample/gtp.yaml

  labels:
    identity: greeting
 spec:
  policy:
-    - dns: stage.greeting.global
+    - dns: default # default is a keyword, alternatively you can use `env` (ex: stage)


Connecting to my previous comment Do you possible mean some other comparison here? Is it possible that the DNS = env? - could we maybe document this behavior a little bit more clearly? I find it a little counterintuitive that the DNS field should be the relevant environment (and it's distinctly different from how the docs say things work: https://github.com/istio-ecosystem/admiral/blob/master/docs/Architecture.md)

josephpeacock · 2021-01-13T19:37:51Z

The change from the DNS field in the GTP to be the environment seems to be backwards incompatible to me. Are we concerned that may break any existing users?

aattuluri · 2021-01-13T21:34:46Z

The change from the DNS field in the GTP to be the environment seems to be backwards incompatible to me. Are we concerned that may break any existing users?

I changed the implementation to treat dns as a dns prefix rather than FQQN, the rationale is as follows:
Currently, we generate default service entries based on the env label and they look something like ..global. Now when a gtp is defined, repeating the fully qualified domain name for every dns is error prone and cannot guarantee uniqueness across envs and namespaces. The dns field will act as a dnsPrefix except for scenarios where dns = 'default' (a keyword) or dns = env in which case its assumed that someone is trying to define override for locality load balancing via gtp for the default name. This will actually preserve backward compatibility and make sure the generated entries are unique.

That being said, I can add an additional check to see if the dns in gtp matches the full default host for backward compatibility but that will retain hybrid behavior (dns prefix vs full dns), so I think it makes sense to introduce this backward incompatible change and then call it out in the release notes.

josephpeacock · 2021-01-13T22:54:38Z

I agree 100% that this is a good direction in which to go. I just want to make sure we're not breaking anyone else. Not sure how careeful we want to be, but the most foolproof option would probably be to introduce a new field in the GTP spec (something like DNSPrefix) and mark DNS as deprecated.

josephpeacock

Approving as all of your updates look good, but please update the GTP section here: https://github.com/istio-ecosystem/admiral/blob/master/docs/Architecture.md to use the DNS prefix and show how it's used.

Fixes #163 Fixes #161 Signed-off-by: Mengying <mengyinglimandy@gmail.com>

* Add support for gtp with multiple traffic policies (#162) Fixes #163 Fixes #161 Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Fix mesh port match against to look at k8s svc target port (#165) Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Add basic health check API (#171) * some basic folder * get the basic curl /health/ready working * Refactoring api server files * reverting a very tiny comment that I shouldn't have pushed Signed-off-by: Mengying <mengyinglimandy@gmail.com> * trying to fix the circle CI error Signed-off-by: Mengying <mengyinglimandy@gmail.com> * addressed the comment Signed-off-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Admiral apis (#175) * added the get all cluste api Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * fixed the unit test Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * updated the comment a little bit Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Adding api to get service entries based on given cluster or given identity Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Refactoring api code Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing failing tests for service.go Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments and ci failures Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * added some unit tests Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added the cluster id inside deployment controller (#176) * added the cluster id inside deployment controller Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed the helm setup failure Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id in other remote controllers Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added unit test for api function GetServiceEntriesByCluster Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed a small typing in test Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed some indentation Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added test for get se by identity Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id to be print in log Co-authored-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: Mengying-Li <43981707+Mengying-Li@users.noreply.github.com> * added more context to the API call Co-authored-by: aattuluri <44482891+aattuluri@users.noreply.github.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: vrushalijoshi <vrushalijoshi.cummins@gmail.com>

* Add support for gtp with multiple traffic policies (istio-ecosystem#162) Fixes istio-ecosystem#163 Fixes istio-ecosystem#161 Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Fix mesh port match against to look at k8s svc target port (istio-ecosystem#165) Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Add basic health check API (istio-ecosystem#171) * some basic folder * get the basic curl /health/ready working * Refactoring api server files * reverting a very tiny comment that I shouldn't have pushed Signed-off-by: Mengying <mengyinglimandy@gmail.com> * trying to fix the circle CI error Signed-off-by: Mengying <mengyinglimandy@gmail.com> * addressed the comment Signed-off-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Admiral apis (istio-ecosystem#175) * added the get all cluste api Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * fixed the unit test Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * updated the comment a little bit Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Adding api to get service entries based on given cluster or given identity Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Refactoring api code Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing failing tests for service.go Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments and ci failures Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * added some unit tests Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added the cluster id inside deployment controller (istio-ecosystem#176) * added the cluster id inside deployment controller Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed the helm setup failure Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id in other remote controllers Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added unit test for api function GetServiceEntriesByCluster Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed a small typing in test Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed some indentation Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added test for get se by identity Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id to be print in log Co-authored-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: Mengying-Li <43981707+Mengying-Li@users.noreply.github.com> * added more context to the API call Co-authored-by: aattuluri <44482891+aattuluri@users.noreply.github.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: vrushalijoshi <vrushalijoshi.cummins@gmail.com> Signed-off-by: psikka1 <pankaj_sikka@intuit.com>

aattuluri added 30 commits September 18, 2019 21:36

Bootstrapping admiral

d457913

Add circle ci config

7b9a2ff

Fix the working directory for builds

c9e80a5

Bootstrapping admiral

76b9561

Signed-off-by: aattuluri <anil_attuluri@intuit.com>

Add circle ci config

85aeeaa

Signed-off-by: aattuluri <anil_attuluri@intuit.com>

Fix the working directory for builds

fe313cc

Signed-off-by: aattuluri <anil_attuluri@intuit.com>

Merge branch 'master' of https://github.com/aattuluri/admiral

306ad93

Add build status badge.

a9fa409

Signed-off-by: aattuluri <anil_attuluri@intuit.com>

Revert "Add build status badge."

4ae717d

This reverts commit a9fa409.

Merge remote-tracking branch 'upstream/master'

ff09cfb

Merge remote-tracking branch 'upstream/master'

eb51849

Merge remote-tracking branch 'upstream/master'

928c0d2

Merge remote-tracking branch 'upstream/master'

2fb3364

Merge remote-tracking branch 'upstream/master'

ea3ed61

Merge remote-tracking branch 'upstream/master'

15e7845

Merge remote-tracking branch 'upstream/master'

3b34036

Merge remote-tracking branch 'upstream/master'

656bf71

Merge remote-tracking branch 'upstream/master'

0245314

Merge remote-tracking branch 'upstream/master'

111c3de

Merge remote-tracking branch 'upstream/master'

e39651c

Merge remote-tracking branch 'upstream/master'

b280c0b

Merge remote-tracking branch 'upstream/master'

4b74679

Merge remote-tracking branch 'upstream/master'

09b8a5b

Merge remote-tracking branch 'upstream/master'

6546170

Merge remote-tracking branch 'upstream/master'

1b0dfeb

Merge remote-tracking branch 'upstream/master'

9b0010b

Merge remote-tracking branch 'upstream/master'

7b68379

Merge remote-tracking branch 'upstream/master'

5ae2f4c

Publish images: i) latest from master, ii) TAG if its set and iii) co…

6e297db

…mmit sha if neither i) or ii)

Merge remote-tracking branch 'upstream/master'

d6674b9

aattuluri added 5 commits January 12, 2021 15:22

Run on larger circle ci instance, remove nodes

8b1198f

Label nodes with locality for tests.

8a7dad9

Switch back to original state

689b546

Remove nodes tagging for remote

41aa613

Temporariliy disable tests for 1.6

2ec6e1c

josephpeacock suggested changes Jan 13, 2021

View reviewed changes

aattuluri added 3 commits January 13, 2021 13:44

Code review feedback

09b3bc4

Fix installation

a2bbe74

Adjust time

5cd2555

aattuluri added 10 commits January 14, 2021 14:41

Adding a dnsPrefix field to gtp and deprecating dns field.

b05bbb1

Fix tests and docs

6ef572c

Wait a bit longer

14992a4

Change image pull policy. Tune tests.

3b296e8

Adjust grpc pod resources.

fa7aad3

Retry tests to account for instability

455b816

Adding admiral logs to debug better

5132b1f

Remove admiral logging

5e33f2e

More buffer

fd39127

Add back istio 1.5

8c999eb

josephpeacock approved these changes Jan 19, 2021

View reviewed changes

Add to example for dnsPrefix usage.

b4fe020

aattuluri merged commit c604d11 into istio-ecosystem:master Jan 19, 2021

josephpeacock mentioned this pull request Feb 11, 2021

[BUG] Multi cluster case's sample load balanced not work with greeting sample apps #167

Closed

Mengying-Li pushed a commit that referenced this pull request Apr 28, 2021

Add support for gtp with multiple traffic policies (#162)

d70e586

Fixes #163 Fixes #161 Signed-off-by: Mengying <mengyinglimandy@gmail.com>

itsLucario pushed a commit that referenced this pull request Nov 7, 2022

log error, instead of returning when vs deletion fails (#254) (#162)

2615a04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for gtp with multiple traffic policies #162

Add support for gtp with multiple traffic policies #162

aattuluri commented Jan 4, 2021

josephpeacock Jan 13, 2021

josephpeacock Jan 13, 2021

josephpeacock Jan 13, 2021

josephpeacock Jan 13, 2021

josephpeacock Jan 13, 2021

josephpeacock commented Jan 13, 2021

aattuluri commented Jan 13, 2021

josephpeacock commented Jan 13, 2021

josephpeacock left a comment

Add support for gtp with multiple traffic policies #162

Add support for gtp with multiple traffic policies #162

Conversation

aattuluri commented Jan 4, 2021

josephpeacock Jan 13, 2021

Choose a reason for hiding this comment

josephpeacock Jan 13, 2021

Choose a reason for hiding this comment

josephpeacock Jan 13, 2021

Choose a reason for hiding this comment

josephpeacock Jan 13, 2021

Choose a reason for hiding this comment

josephpeacock Jan 13, 2021

Choose a reason for hiding this comment

josephpeacock commented Jan 13, 2021

aattuluri commented Jan 13, 2021

josephpeacock commented Jan 13, 2021

josephpeacock left a comment

Choose a reason for hiding this comment