Handle annotations and conflicting paths for MergeableTypes #258

diazjf · 2018-03-23T20:09:08Z

Adds new rules for the MergeableTypes. Minions will not be able to have conflicting locations, and can only have service level annotations. Masters will only be able to have host level annotations.

Fixes #264

diazjf · 2018-03-23T20:17:43Z

@pleshakov just a WIP of the annotation and conflicting path rules. We can talk sometime next week. Have a good weekend!

diazjf · 2018-03-23T20:33:22Z

We can just decide to have the same error that currently appears where there cannot be conflicting hosts for a non-mergeable and mergeable type. I think having the same error is fine because it follows the same flow as today. Users should know that they must be using mergeable-ingress-types to avoid this.

Therefore the below should be shown if there is a non-mergeable type and a mergeable type together.

2018/03/24 03:56:28 [warn] 35#35: conflicting server name "cafe.example.com" on 0.0.0.0:80, ignored
2018/03/24 03:56:28 [warn] 35#35: conflicting server name "cafe.example.com" on 0.0.0.0:443, ignored

diazjf · 2018-03-26T19:18:58Z

nginx-controller/controller/controller.go

@@ -1204,6 +1204,7 @@ func (lbc *LoadBalancerController) getMinionsForMaster(master *nginx.IngressEx)
 	}

 	var minions []*nginx.IngressEx
+IngressLoop:


Instead of using the label we can also set a boolean flag to continue if a certain annotation is found or if there are multiple paths, but using a label is quicker since we won't have to wait for the loop to end.

diazjf · 2018-03-26T19:20:59Z

nginx-controller/controller/controller.go

+			if key == "nginx.org/mergible-ingress-type" || !strings.Contains(key, "nginx.org/") {
+				continue
+			}
+			if !strings.Contains(val, "serviceName=") {


I was thinking minion annotations should only contain annotations where serviceName is applied.

We can make "serviceName=" a global since it is used in multiple places.

I think we should handle most of the annotations that could be applied per a location block in the NGINX config or per an upstream (service). Such a handling enables new use cases.

For example, consider an Ingress resource with multiple paths and multiple services. If we want to choose a load balancing method, other than the default round robin, we need to use nginx.org/lb-method annotation. However, this annotation applies to all services in the Ingress resource. What if we want to change the lb method only for one service. To overcome this limitation, we could potentially change the nginx.org/lb-method to support specifying a service, similar to what we did for session persistence. However, with mergible Ingresses, we can split the Ingress resource into multiple ones and in each minion Ingress resource have only one path/service and configure the lb method using the nginx.org/lb-method annotation for each service.

Another example is nginx.org/location-snippets annotation.

That works for me. So for example:

Minion_1 contains /tea and /coffee. If we want to change the load-balancing for both then we apply nginx.org/lb-method which configures both. Now if we wanted it only for /coffee then we split it up into 2 minions and apply it only to the 1 with /coffee.

I can remove the serviceName check and then apply all the master's annotations to the minions and then from each minion apply only their specific annotations overwriting the masters. All within the functions which generates the Nginx Configuration.

@diazjf that sounds good

diazjf · 2018-03-26T19:26:09Z

nginx-controller/controller/controller.go

+			}
+		}
+
+		for _, path := range ings.Items[i].Spec.Rules[0].HTTP.Paths {


Here we will skip any minion with the same path as another minion. Therefore only 1 of the 2 minions will be applied.

Is there any type of order used when listing ingresses using lbc.ingLister.List()? Should I apply some type of sorting after the list is generated, so that we know it will always be the first successful minion that will be active?

I want this to happen at the controller level so that it doesn't even reach any logic which involves the generation of the configuration.

I think sorting based on the creation time stamp would be a good approach. This way we'll get consistent results, including the case of having multiple Ingress controller replicas running.

diazjf · 2018-03-26T19:27:08Z

nginx-controller/controller/controller.go

+		if key == "nginx.org/mergible-ingress-type" || !strings.Contains(key, "nginx.org/") {
+			continue
+		}
+		if strings.Contains(val, "serviceName=") {


Master configuration should be able to take any annotation that is not only for a specific location.

pleshakov

I think we should allow configuring most of the annotations both in the master and in minions. This way, minions can override the values of the master. Also, in minions, an annotation can be applied only to a particular path/service, rather than all path/services. This enables new use cases.

pleshakov · 2018-03-27T07:14:56Z

nginx-controller/controller/controller.go

+			}
+		}
+
+		for _, path := range ings.Items[i].Spec.Rules[0].HTTP.Paths {


I think sorting based on the creation time stamp would be a good approach. This way we'll get consistent results, including the case of having multiple Ingress controller replicas running.

pleshakov · 2018-03-27T08:58:55Z

nginx-controller/controller/controller.go

+			if key == "nginx.org/mergible-ingress-type" || !strings.Contains(key, "nginx.org/") {
+				continue
+			}
+			if !strings.Contains(val, "serviceName=") {


I think we should handle most of the annotations that could be applied per a location block in the NGINX config or per an upstream (service). Such a handling enables new use cases.

For example, consider an Ingress resource with multiple paths and multiple services. If we want to choose a load balancing method, other than the default round robin, we need to use nginx.org/lb-method annotation. However, this annotation applies to all services in the Ingress resource. What if we want to change the lb method only for one service. To overcome this limitation, we could potentially change the nginx.org/lb-method to support specifying a service, similar to what we did for session persistence. However, with mergible Ingresses, we can split the Ingress resource into multiple ones and in each minion Ingress resource have only one path/service and configure the lb method using the nginx.org/lb-method annotation for each service.

Another example is nginx.org/location-snippets annotation.

diazjf · 2018-03-29T18:12:58Z

nginx-controller/controller/controller.go

@@ -1243,6 +1264,11 @@ func (lbc *LoadBalancerController) findMasterForMinion(minion *extensions.Ingres
 		return &extensions.Ingress{}, err
 	}

+	// ingresses are sorted by creation time


Also we can add a master count and at the end of this function log an error if there is more than 1. We will still return the 1st master found by creation time.

Let's skip this check for now. Currently, in the Ingress controller we don't handle the case when there are multiple regular Ingresses for the same host. I think once implemented, the logic for handling that conflict would also work for multiple masters for the same host.

diazjf · 2018-03-29T18:15:44Z

nginx-controller/nginx/configurator.go

@@ -84,6 +84,16 @@ func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngr
 	var upstreams []Upstream
 	var keepalive string

+	for _, blacklistAnn := range mergeableIngressAnnotationBlacklist["master"] {


So if there is a blacklisted annotation present we log an error and will remove it before generating the configuration. ConfigMaps will always overwrite any config value so we are not touching them.

diazjf · 2018-03-29T18:17:52Z

@pleshakov I updated the PR. Let me know what you think about the path I am taking. I want to add some unit tests as well.

pleshakov

Looks good. Please see my comments

pleshakov · 2018-04-03T15:35:17Z

nginx-controller/nginx/ingress.go

 	Minions []*IngressEx
 }
+
+var mergeableIngressAnnotationBlacklist = map[string][]string{


I think it is better to have two maps of map[string]bool for two black lists -- the master one and the minion one.

To keep it simple, can we just add a []string

pleshakov · 2018-04-03T15:41:46Z

nginx-controller/controller/controller.go

@@ -1243,6 +1264,11 @@ func (lbc *LoadBalancerController) findMasterForMinion(minion *extensions.Ingres
 		return &extensions.Ingress{}, err
 	}

+	// ingresses are sorted by creation time


Let's skip this check for now. Currently, in the Ingress controller we don't handle the case when there are multiple regular Ingresses for the same host. I think once implemented, the logic for handling that conflict would also work for multiple masters for the same host.

pleshakov · 2018-04-03T15:50:01Z

nginx-controller/controller/controller.go

@@ -1222,6 +1229,19 @@ func (lbc *LoadBalancerController) getMinionsForMaster(master *nginx.IngressEx)
 			glog.Errorf("Ingress Resource %v/%v with the 'nginx.org/mergible-ingress-type' annotation set to 'minion' must contain a Path", ings.Items[i].Namespace, ings.Items[i].Name)
 			continue
 		}
+
+		for _, path := range ings.Items[i].Spec.Rules[0].HTTP.Paths {
+			for _, minion := range minions {


to check for map conflicts, it is better to use a map[string]*nginx.IngressEx, where the map key is a path, rather iterating over an array of minions.

Using map[sting]*extensions.Ingress, where key is the path, and value is the minion Ingress.

pleshakov · 2018-04-03T15:54:27Z

nginx-controller/controller/controller.go

+						glog.Errorf("Ingress Resource %v/%v with the 'nginx.org/mergible-ingress-type' annotation set to 'minion' cannot contain the same path as another ingress resource, %v/%v",
+							ings.Items[i].Namespace, ings.Items[i].Name, minion.Ingress.Namespace, minion.Ingress.Name)
+						continue IngressLoop
+					}


In this case, if an Ingress resource has a conflicting path and is not the oldest Ingress resource, the Ingress resource is completely Ignored. Maybe it is better to not ignore it, but only ignore that conflicting path?

I agree, we can just log an error and not include the newest path. The configuration will also contain which Ingress Resource generates which path. This can also aid the user in debugging. I can also add more instructions on this feature.

diazjf

Thanks for the review! I will update the PR!

diazjf · 2018-04-04T19:14:41Z

nginx-controller/nginx/ingress.go

 	Minions []*IngressEx
 }
+
+var mergeableIngressAnnotationBlacklist = map[string][]string{


To keep it simple, can we just add a []string

diazjf · 2018-04-04T19:18:12Z

nginx-controller/controller/controller.go

+						glog.Errorf("Ingress Resource %v/%v with the 'nginx.org/mergible-ingress-type' annotation set to 'minion' cannot contain the same path as another ingress resource, %v/%v",
+							ings.Items[i].Namespace, ings.Items[i].Name, minion.Ingress.Namespace, minion.Ingress.Name)
+						continue IngressLoop
+					}


I agree, we can just log an error and not include the newest path. The configuration will also contain which Ingress Resource generates which path. This can also aid the user in debugging. I can also add more instructions on this feature.

diazjf · 2018-04-04T19:18:55Z

nginx-controller/controller/controller.go

@@ -1222,6 +1229,19 @@ func (lbc *LoadBalancerController) getMinionsForMaster(master *nginx.IngressEx)
 			glog.Errorf("Ingress Resource %v/%v with the 'nginx.org/mergible-ingress-type' annotation set to 'minion' must contain a Path", ings.Items[i].Namespace, ings.Items[i].Name)
 			continue
 		}
+
+		for _, path := range ings.Items[i].Spec.Rules[0].HTTP.Paths {
+			for _, minion := range minions {


Using map[sting]*extensions.Ingress, where key is the path, and value is the minion Ingress.

pleshakov

Please see my comments

pleshakov · 2018-04-05T11:06:32Z

examples/mergable-ingress-types/README.md

-locations to an ingress resource with the Master value. The annotations of minions are replaced with the annotations of
-their master. TLS configurations are not allowed. There can be multiple minions which must have the same host as the master.
+locations to an ingress resource with the Master value. TLS configurations are not allowed. Multiple minions can be
+applied per master as long as they do not have conflicting paths. Any conflicting paths will be ignored. Also note that the


It is worth mentioning that in case of several minions having the same path, the path defined in the oldest minion will be used.

pleshakov · 2018-04-05T11:08:19Z

nginx-controller/controller/controller.go

@@ -1243,6 +1266,11 @@ func (lbc *LoadBalancerController) findMasterForMinion(minion *extensions.Ingres
 		return &extensions.Ingress{}, err
 	}

+	// ingresses are sorted by creation time
+	sort.Slice(ings.Items[:], func(i, j int) bool {


sorting ingresses is unnecessary here, as we will not check for duplicated masters for now

pleshakov · 2018-04-05T11:14:30Z

nginx-controller/nginx/configurator.go

@@ -84,6 +84,16 @@ func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngr
 	var upstreams []Upstream
 	var keepalive string

+	for _, blacklistAnn := range masterBlacklist {


could we have a function for filtering the master annotations?

pleshakov · 2018-04-05T11:15:11Z

nginx-controller/nginx/configurator.go

@@ -101,20 +111,36 @@ func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngr

 	minions := mergeableIngs.Minions
 	for _, minion := range minions {
+
+		for _, blacklistAnn := range minionBlacklist {


could we have a function for filtering minion annotations?

would it be more efficient if we iterate over all minion annotations and look up if a particular annotation is in the black list using a map?

I optimized it, since annotations are already in a map[string]string, I just look over all the blacklist items and if any are in the master they are removed. Looking through the blacklist since its small and there could be many annotations in an Ingress Resource.

pleshakov · 2018-04-05T11:23:59Z

nginx-controller/nginx/configurator.go

@@ -84,6 +84,16 @@ func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngr
 	var upstreams []Upstream
 	var keepalive string

+	for _, blacklistAnn := range masterBlacklist {
+		for _, masterAnn := range mergeableIngs.Master.Ingress.Annotations {


would it be more efficient if we iterate over all master annotations and look up if a particular annotation is in the black list using a map?

I optimized it, since annotations are already in a map[string]string, I just look over all the blacklist items and if any are in the master they are removed. Looking through the blacklist since its small and there could be many annotations in an Ingress Resource.

pleshakov · 2018-04-05T11:30:07Z

@diazjf could we also remove the default backend in minions if it is present? Otherwise, if a minion has the default backend, the configurator will generate the "/" for it. Consider the example below:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: minon-ingress
  annotations:
   . . .
spec:
  backend:
    serviceName: testsvc
    servicePort: 80
. . .

diazjf

Requested Changes have been added.

Thanks @pleshakov

diazjf · 2018-04-05T16:26:22Z

examples/mergable-ingress-types/README.md

-locations to an ingress resource with the Master value. The annotations of minions are replaced with the annotations of
-their master. TLS configurations are not allowed. There can be multiple minions which must have the same host as the master.
+locations to an ingress resource with the Master value. TLS configurations are not allowed. Multiple minions can be
+applied per master as long as they do not have conflicting paths. Any conflicting paths will be ignored. Also note that the


diazjf · 2018-04-05T16:29:16Z

nginx-controller/controller/controller.go

@@ -1243,6 +1266,11 @@ func (lbc *LoadBalancerController) findMasterForMinion(minion *extensions.Ingres
 		return &extensions.Ingress{}, err
 	}

+	// ingresses are sorted by creation time
+	sort.Slice(ings.Items[:], func(i, j int) bool {


diazjf · 2018-04-05T16:59:01Z

nginx-controller/nginx/configurator.go

@@ -84,6 +84,16 @@ func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngr
 	var upstreams []Upstream
 	var keepalive string

+	for _, blacklistAnn := range masterBlacklist {


diazjf · 2018-04-05T17:03:09Z

nginx-controller/nginx/configurator.go

@@ -84,6 +84,16 @@ func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngr
 	var upstreams []Upstream
 	var keepalive string

+	for _, blacklistAnn := range masterBlacklist {
+		for _, masterAnn := range mergeableIngs.Master.Ingress.Annotations {


I optimized it, since annotations are already in a map[string]string, I just look over all the blacklist items and if any are in the master they are removed. Looking through the blacklist since its small and there could be many annotations in an Ingress Resource.

diazjf · 2018-04-05T17:03:28Z

nginx-controller/nginx/configurator.go

@@ -101,20 +111,36 @@ func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngr

 	minions := mergeableIngs.Minions
 	for _, minion := range minions {
+
+		for _, blacklistAnn := range minionBlacklist {


I optimized it, since annotations are already in a map[string]string, I just look over all the blacklist items and if any are in the master they are removed. Looking through the blacklist since its small and there could be many annotations in an Ingress Resource.

pleshakov

@diazjf
looks good! there is a bug with merging the master annotations into the minion annotations. otherwise, it works

pleshakov · 2018-04-06T11:32:48Z

examples/mergable-ingress-types/README.md

+locations to an ingress resource with the Master value. TLS configurations are not allowed. Multiple minions can be
+applied per master as long as they do not have conflicting paths. If a conflicting path is present then the path defined
+on the oldest minion will be used. Also note that the
+`/` path will be applied to all minions and any duplicates of it will be ignored.


Also note that the/ path will be applied to all minions and any duplicates of it will be ignored.

I think this is redundant.

Removed. It can be assumed from the general conflicting paths sentence.

pleshakov · 2018-04-06T12:44:07Z

nginx-controller/nginx/configurator.go

@@ -766,6 +767,37 @@ func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error {
 	return nil
 }

+func (cnf *Configurator) filterMaster(master *IngressEx) *IngressEx {


can we change this function to

func filterMasterAnnotations(annotations map[string]string)

I think the function should

Stay as filterMasterAnnotations(master *IngressEx) *IngressExfor the reason of logging which Ingress Resource was affected by the filter.

Have the function return a []string of errors, which will just generate a log of all the annotations which were incorrect for a certain Ingress Resource.

I'm going with step 2 since it will have way cleaner logs.

pleshakov · 2018-04-06T12:49:25Z

nginx-controller/nginx/configurator.go

+	return master
+}
+
+func (cnf *Configurator) filterMinion(minion *IngressEx, master *IngressEx) *IngressEx {


I think we need to split this func into two:

func filterMinionAnnotations(annotations map[string]string) func mergeMasterAnnotationsIntoMinion(minion map[string]string, master map[string]string)

It looks like we will need to call mergeMasterAnnotationsIntoMinion before filterMinionAnnotations

pleshakov · 2018-04-06T12:50:22Z

nginx-controller/nginx/configurator.go

+
+func (cnf *Configurator) filterMinion(minion *IngressEx, master *IngressEx) *IngressEx {
+	// Overwrite minion annotation with acceptable master annotation
+	for _, masterAnn := range master.Ingress.Annotations {


this logic is incorrect. The values of the master annotations are added to the minion annotations as keys.
Could you add unit tests for all filtering and merging functions?

Ahh, I see, I don't know why I did that lol. I got the keys and Values mixed up when going through the map. Yeah I still need to add a couple more Unit tests.

pleshakov · 2018-04-06T12:55:35Z

Could you also remove "kubernetes.io/ingress.class" from the minion black list?

diazjf

@pleshakov updating the PR. Thanks for all the helpful suggestions.

diazjf · 2018-04-06T16:13:31Z

examples/mergable-ingress-types/README.md

+locations to an ingress resource with the Master value. TLS configurations are not allowed. Multiple minions can be
+applied per master as long as they do not have conflicting paths. If a conflicting path is present then the path defined
+on the oldest minion will be used. Also note that the
+`/` path will be applied to all minions and any duplicates of it will be ignored.


Removed. It can be assumed from the general conflicting paths sentence.

diazjf · 2018-04-06T16:20:02Z

nginx-controller/nginx/configurator.go

@@ -766,6 +767,37 @@ func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error {
 	return nil
 }

+func (cnf *Configurator) filterMaster(master *IngressEx) *IngressEx {


I think the function should

Stay as filterMasterAnnotations(master *IngressEx) *IngressExfor the reason of logging which Ingress Resource was affected by the filter.

Have the function return a []string of errors, which will just generate a log of all the annotations which were incorrect for a certain Ingress Resource.

I'm going with step 2 since it will have way cleaner logs.

diazjf · 2018-04-06T16:46:41Z

nginx-controller/nginx/configurator.go

+
+func (cnf *Configurator) filterMinion(minion *IngressEx, master *IngressEx) *IngressEx {
+	// Overwrite minion annotation with acceptable master annotation
+	for _, masterAnn := range master.Ingress.Annotations {


Ahh, I see, I don't know why I did that lol. I got the keys and Values mixed up when going through the map. Yeah I still need to add a couple more Unit tests.

pleshakov

Works as expected. I suggested to change the filter* and the merge functions.

pleshakov · 2018-04-09T15:40:26Z

nginx-controller/nginx/configurator.go

+		}
+	}
+
+	return minionAnnotations, minionOverwrites


I don't think we need to return minionOverwrites.
Moreover, this function should not return anything, because the map minionAnnotations is passed by reference and will be changed by the function.

pleshakov · 2018-04-09T15:41:33Z

nginx-controller/nginx/configurator.go

+	return annotations, removedAnnotations
+}
+
+func filterMinionAnnotations(annotations map[string]string) (map[string]string, []string) {


we don't need to return a map, only the removedAnnotations.

pleshakov · 2018-04-09T15:42:13Z

nginx-controller/nginx/configurator.go

+
+		// Add acceptable master annotations to minion
+		minion.Ingress.Annotations, masterOverwrites = mergeMasterAnnotationsIntoMinion(minion.Ingress.Annotations, mergeableIngs.Master.Ingress.Annotations)
+		if len(masterOverwrites) != 0 {


no need to report overwritten annotations

pleshakov · 2018-04-09T15:43:18Z

nginx-controller/nginx/configurator.go

@@ -766,6 +786,45 @@ func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error {
 	return nil
 }

+func filterMasterAnnotations(annotations map[string]string) (map[string]string, []string) {


we don't need to return a map, only the removedAnnotations.

diazjf · 2018-04-09T16:05:20Z

@pleshakov Done! And updated the unit tests. Thanks again.

pleshakov

A few minor suggestions

pleshakov · 2018-04-09T18:51:18Z

nginx-controller/nginx/configurator.go

+}
+
+func mergeMasterAnnotationsIntoMinion(minionAnnotations map[string]string, masterAnnotations map[string]string) {
+	var minionOverwrites []string


var minionOverwrites []strings is not used

pleshakov · 2018-04-09T19:07:04Z

nginx-controller/nginx/configurator.go

+	var minionOverwrites []string
+
+	for key, val := range masterAnnotations {
+		if _, ok := minionAnnotations[key]; !ok {


we also need to check that an annotation is not in the minion black list as well. If we don't do that, then an non-supported annotation will be merged into a minion. That will lead to the filterMinionAnnotations function returning that annotation in its removedAnnotations list. That will lead to an error message in the log, which should not happen!

Good catch as well! Thanks.

diazjf

@pleshakov We should be good now. Sorry for missing those little items.

diazjf · 2018-04-09T19:30:17Z

nginx-controller/nginx/configurator.go

+}
+
+func mergeMasterAnnotationsIntoMinion(minionAnnotations map[string]string, masterAnnotations map[string]string) {
+	var minionOverwrites []string


diazjf · 2018-04-09T20:01:09Z

nginx-controller/nginx/configurator.go

+	var minionOverwrites []string
+
+	for key, val := range masterAnnotations {
+		if _, ok := minionAnnotations[key]; !ok {


Good catch as well! Thanks.

pleshakov

There is a small bug. Added a few suggestions.

pleshakov · 2018-04-10T12:31:05Z

nginx-controller/nginx/configurator_test.go

+	}
+}
+
+func TestMergeMasterAnnotationsIntoMinion(t *testing.T) {


Since we also check if a master annotation is in the minion black list before merging, could you test thing logic in the unit test here?

pleshakov · 2018-04-10T12:31:11Z

nginx-controller/nginx/configurator.go

+
+	for key, val := range masterAnnotations {
+		if _, ok := minionAnnotations[key]; !ok {
+			for _, blacklistAnn := range minionBlacklist {


the check if an annotation is in the black list has a bug -- isBlacklisted must be initialized to false prior to each iteration of the loop.

Instead of iterating over the minionBlacklist, I suggest to change the minionBlacklist from []string to map[string]bool for performance reason. The code will look like this:

for key, val := range masterAnnotations { if _, ok := minionAnnotations[key]; !ok && !minionBlacklist[key] { minionAnnotations[key] = val } }

I can change to maps in another PR if that ok.

sounds good!

Adds new rules for the MergeableTypes. Minions will not be able to have conflicting locations, and can only have service level annotations. Masters will only be able to have host level annotations. Fixes nginxinc#264

pleshakov · 2018-04-10T15:45:53Z

@diazjf thank you!!

diazjf commented Mar 26, 2018

View reviewed changes

pleshakov reviewed Mar 27, 2018

View reviewed changes

diazjf force-pushed the more-mergeable-type-rules branch 2 times, most recently from f809a49 to 84c34d4 Compare March 29, 2018 18:03

diazjf commented Mar 29, 2018

View reviewed changes

diazjf force-pushed the more-mergeable-type-rules branch from 84c34d4 to 05d3cd2 Compare March 29, 2018 21:40

pleshakov mentioned this pull request Apr 3, 2018

Mergeable Ingress Types ignore root "/" paths #264

Closed

pleshakov reviewed Apr 3, 2018

View reviewed changes

diazjf commented Apr 4, 2018

View reviewed changes

diazjf force-pushed the more-mergeable-type-rules branch from 05d3cd2 to 155ea14 Compare April 4, 2018 19:44

pleshakov reviewed Apr 5, 2018

View reviewed changes

diazjf commented Apr 5, 2018

View reviewed changes

diazjf force-pushed the more-mergeable-type-rules branch 2 times, most recently from 4e2b9ff to 2df1afe Compare April 5, 2018 18:21

pleshakov reviewed Apr 6, 2018

View reviewed changes

diazjf commented Apr 7, 2018

View reviewed changes

diazjf force-pushed the more-mergeable-type-rules branch from 2df1afe to 31d9e1a Compare April 7, 2018 04:45

pleshakov reviewed Apr 9, 2018

View reviewed changes

diazjf force-pushed the more-mergeable-type-rules branch from 31d9e1a to 0123919 Compare April 9, 2018 16:04

pleshakov reviewed Apr 9, 2018

View reviewed changes

diazjf force-pushed the more-mergeable-type-rules branch from 0123919 to c899dca Compare April 9, 2018 20:00

diazjf commented Apr 9, 2018

View reviewed changes

pleshakov reviewed Apr 10, 2018

View reviewed changes

diazjf force-pushed the more-mergeable-type-rules branch from c899dca to bcb730a Compare April 10, 2018 14:37

Handle annotations and conflicting paths for MergeableTypes

1bccb05

Adds new rules for the MergeableTypes. Minions will not be able to have conflicting locations, and can only have service level annotations. Masters will only be able to have host level annotations. Fixes nginxinc#264

diazjf force-pushed the more-mergeable-type-rules branch from bcb730a to 1bccb05 Compare April 10, 2018 14:39

pleshakov merged commit 56665db into nginxinc:master Apr 10, 2018

Handle annotations and conflicting paths for MergeableTypes #258

Handle annotations and conflicting paths for MergeableTypes #258

Conversation

diazjf commented Mar 23, 2018 • edited Loading

diazjf commented Mar 23, 2018

diazjf commented Mar 23, 2018 • edited Loading

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

diazjf Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleshakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diazjf Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diazjf Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

diazjf commented Mar 29, 2018

pleshakov left a 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

diazjf left a 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

pleshakov left a 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

pleshakov commented Apr 5, 2018

diazjf left a 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

pleshakov left a 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

pleshakov commented Apr 6, 2018

diazjf left a 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

pleshakov left a 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

diazjf commented Mar 23, 2018 •

edited

Loading

diazjf commented Mar 23, 2018 •

edited

Loading

diazjf Mar 26, 2018 •

edited

Loading

diazjf Mar 29, 2018 •

edited

Loading

diazjf Mar 29, 2018 •

edited

Loading