Conversation
|
[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. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rshriram
left a comment
There was a problem hiding this comment.
Please remove the hacky code inside the gateway. I am okay with any contained changes in the ingress code as it affects just the ingress users.
| out = append(out, gateways) | ||
| //gateways := ConvertIngressV1alpha3(*ingress, c.domainSuffix) | ||
| //out = append(out, gateways) | ||
| } |
There was a problem hiding this comment.
The auto generated gateway was a big deal for the ingress. As such our conversion tool is sufficient to convert to gateway and virtual service.
There was a problem hiding this comment.
I can uncomment it - but it doesn't really work. Secrets from TLS can't be loaded (there is no code to read the secrets in ingress - they need to be mounted). Other than that - a host-* and port 80/443 is all gateway needed, why generate one gateway per ingress ?
I will add the Gateway with the proper name to helm - so users will not have to do anything, but separate PR since there is a whole separate discussion on how to setup secrets for TLS and which LB to use.
|
|
||
| for n, cfg := range cfgs { | ||
| // Not clear if this is right - should probably be under input ns | ||
| if cfg.ConfigMeta.Namespace != "istio-system" { |
There was a problem hiding this comment.
You cannot have an ingress controller per namespace. So it doesn’t matter which namespace we convert all the ingress resource into.
There was a problem hiding this comment.
Ack - yes, the secrets must be in same namespace with the gateway.
There was a problem hiding this comment.
There is also a constant for Istio-system FYI
| for _, c := range configs { | ||
| vs := c.Spec.(*networking.VirtualService) | ||
| for _, h := range vs.Hosts { | ||
| if !strings.HasSuffix(c.Name, model.IstioIngressGatewayName) { |
There was a problem hiding this comment.
This is deduplicating for non ingress. Why?
There was a problem hiding this comment.
Sorry, mixed separate fix - it seems we still generate duplicate domains and RDS rejects it.
I can send you the log.
I can remove this part of the PR and move it to separate.
| vse, e := hostsToVS[h] | ||
| if e { | ||
| vse.Http = append(vse.Http, vs.Http...) | ||
| log.Infof("Merging ingress to vs %s %s", h, c.Name) |
There was a problem hiding this comment.
The append was already done in the conversion. Why iterate over this again?
There was a problem hiding this comment.
Good point, will remove ( I implemented this first - then figured it's easy to merge during conversion ).
| if !strings.HasSuffix(c.Name, model.IstioIngressGatewayName) { | ||
| vse, e := hostsToVS[h] | ||
| if e { | ||
| log.Warnf("Duplicated hostname %s OLD:%v NEW:%v", h, vse, vs) |
There was a problem hiding this comment.
?? Why are you doing this for non ingress stuff?
There was a problem hiding this comment.
See above, will move to separate PR or file a bug. We probably need to do same trick Nate used to use the timestamp to reject most recent virtual service in cases of duplicated hostname.
…d back the previously and still broken gateway generation
Codecov Report
@@ Coverage Diff @@
## release-1.0 #7144 +/- ##
===========================================
+ Coverage 72% 72% +1%
===========================================
Files 356 356
Lines 30537 30490 -47
===========================================
- Hits 21780 21767 -13
+ Misses 7853 7808 -45
- Partials 904 915 +11
Continue to review full report at Codecov.
|
| // An updated ingress may also trigger an Add or Delete for one of its constituent sub-rules. | ||
| switch typ { | ||
| case model.Gateway.Type: | ||
| config, _ := ConvertIngressV1alpha3(*ingress, c.domainSuffix) |
There was a problem hiding this comment.
nit. If you are going to be adding a gateway for the ingress, you can essentially get rid of the whole Gateway case and have just the model.VirtualService ..
There was a problem hiding this comment.
Yes, that was my intent. But will do it in separate PR now.
| ConvertIngressVirtualService(*ingress, c.domainSuffix, ingressByHost) | ||
| case model.Gateway.Type: | ||
| gateways, _ := ConvertIngressV1alpha3(*ingress, c.domainSuffix) | ||
| gateways := ConvertIngressV1alpha3(*ingress, c.domainSuffix) |
There was a problem hiding this comment.
same comment as above. and you can get rid fo the convertIngressV1alpha3 as well
| Namespace: model.IstioIngressNamespace, | ||
| Domain: domainSuffix, | ||
| }, | ||
| Spec: gateway, |
There was a problem hiding this comment.
And when you add the gateway manually to helm, this entire function can go away.
| rdsName := model.GatewayRDSRouteName(server) | ||
| routeCfg := configgen.buildGatewayInboundHTTPRouteConfig(env, node, nameToServiceMap, gatewayNames, []*networking.Server{server}, rdsName) | ||
| routeCfg := configgen.buildGatewayInboundHTTPRouteConfig(env, node, push, | ||
| nameToServiceMap, gatewayNames, []*networking.Server{server}, rdsName) |
There was a problem hiding this comment.
Please get rid of this fully. I had kept this here while adding RDS but forgot to remove these things after RDS started working.
There was a problem hiding this comment.
Yes, noticed it in another place. Will do - but maybe in separate PR, still need to add some more metrics.
| nameToServiceMap, gatewayNames, []*networking.Server{server}, rdsName) | ||
| if routeCfg == nil { | ||
| log.Warnf("omitting HTTP listeners for port %d filter chain %d due to no routes", server.Port, i) | ||
| continue |
There was a problem hiding this comment.
Oh crap.. Also get rid of this if block. since routeCfg is no longer present.
| continue | ||
| } | ||
| } | ||
| vse.Http = append([]*networking.HTTPRoute{r}, vse.Http...) |
There was a problem hiding this comment.
??
This is merging user generated virtual services with user generated ingress. FYI, this is pretty much what we did before 0.8, and there was a never ending stream of bugs. Keeping the entire ingress separate made life much more easier. And we made a very conscious decision to stop with just the conversion and not do any runtime merging with user generated artifacts precisely due to all the bugs we faced. So, lets please not bring this back, one week before 1.0 launch :).
There was a problem hiding this comment.
Come to think of it, this entire logic is of no use. The gateway code is for a single gateway (istio: ingress, or istio: ingressgateway). So, the list of configs you have here for the current gateway being processed (istio: ingress) are all the auto generated gateways in conversion.go.
| if f { | ||
| push.Add(model.DuplicatedDomains, key, node, | ||
| fmt.Sprintf("Duplicated virtual host %s use %s:%s rejecting %s:%s", | ||
| key, old.Namespace, old.Name, v.Namespace, v.Name)) |
There was a problem hiding this comment.
I don't think this is that simple (string match). Gateway's allow wildcards and we respect wildcards and absorb hosts under a given wildcard. The issues you were seeing were mostly due to the incorrect merging you attempt below. As such, you shouldn't be hitting any case of duplicate virtual hosts unless you generate two virtual services for saem ingress host, in conversion.go. So I would check the logic there.
There was a problem hiding this comment.
Well - still having 2 vhosts with exact same string will be rejected.
Having 2 VirtualServices with same domain is likely to be a common error ( in particular if they're defined across namespaces), and the effect is pretty bad ( all routes rejected). This is not perfect - but still one step forward.
|
|
||
| out := make([]model.Config, 0) | ||
|
|
||
| ingressByHost := map[string]*model.Config{} |
There was a problem hiding this comment.
please keep initialization consistent i.e. make for map.
There was a problem hiding this comment.
I am actually quite consistent in using this syntax :-) Both are fine.
There was a problem hiding this comment.
it's inconsistent with the line above and the rest of the file. usual principle is to follow the established format.
There was a problem hiding this comment.
Sorry, I am a bit frustrated. The real problem with this code is that it allocates shitload of objects and is horribly inefficient. Would be great if reviews focused less on how maps are created and more on why they are created and how to structure the code to reuse. At this point any contributor who can figure out the spaghetti of structures and missdirections will have little problems with either of the styles of creating a map (or order of imports for that matter - most IDEs don't even expose that)
| case model.VirtualService.Type: | ||
| _, virtualServices := ConvertIngressV1alpha3(*ingress, c.domainSuffix) | ||
| out = append(out, virtualServices) | ||
| ConvertIngressVirtualService(*ingress, c.domainSuffix, ingressByHost) |
There was a problem hiding this comment.
it would be better to have this return a value if possible. otherwise, another option would be to add a method to Ingress type.
There was a problem hiding this comment.
Why ? Seems to work fine.
There was a problem hiding this comment.
it's not clear from the function what is being mutated and how. also inconsistent with ConvertIngressV1alpha3 below.
There was a problem hiding this comment.
That's intended - the other method is supposed to be deleted soon, and all the passing by value and un-needed copy and allocations that is used in this (and lots of other places) will need to be fixed as well. When we know we have allocation/memory problems - keeping consistent with inefficient code is not really desirable.
| "k8s.io/api/extensions/v1beta1" | ||
| "k8s.io/apimachinery/pkg/util/intstr" | ||
|
|
||
| "path" |
| } | ||
|
|
||
| func ConvertIngressVirtualService(ingress v1beta1.Ingress, domainSuffix string, ingressByHost map[string]*model.Config) { | ||
| // Ingress allows a single host - if missing '*' is assumed |
There was a problem hiding this comment.
public functions should be documented. i suggest moving much of the text like "mixing *" to the function comment since it will explain the behavior of the function to the reader without needing to go into the code.
| } | ||
|
|
||
| host := rule.Host | ||
| namePrefix := strings.Replace(host, ".", "-", -1) |
There was a problem hiding this comment.
even though it's trivial, wrapping this in a helper function will document why this replace is needed.
There was a problem hiding this comment.
And likely make the code harder to read (context switch). I can add a comment if it is not obvious ( dot not valid in names)
There was a problem hiding this comment.
why would namePrefix := replaceInvalidDotsInName(host) be harder to read? this is self commenting.
There was a problem hiding this comment.
It is not - it doesn't say how they are replaced, why they are invalid ( they are not invalid in a hostname ), and in the unlikely event it will be reused accidentally can lead to bigger problems. It's a one liner. Maybe 'replaceAllDotsWithMinus' would be self commenting. In go it is considered ok to duplicate small pieces of code.
| "k8s.io/apimachinery/pkg/util/intstr" | ||
|
|
||
| networking "istio.io/api/networking/v1alpha3" | ||
|
|
There was a problem hiding this comment.
I'm running "make format" - I think manual management of imports (or spending time reviewing the order) is not best use of time, adding imports is also automated by the IDE.
There was a problem hiding this comment.
sure, i just happened to notice it.
There was a problem hiding this comment.
If linter complains - it'll have to be fixed. Otherwise - it's far more valuable to spend your time looking at efficiency/correctness of the code and the big picture...
| Spec: v1beta1.IngressSpec{ | ||
| Rules: []v1beta1.IngressRule{ | ||
| { | ||
| Host: "my.host.com", |
There was a problem hiding this comment.
define this as const and use here and below.
There was a problem hiding this comment.
I think it also makes things harder to read. A string is immutable too.
There was a problem hiding this comment.
maybe, but it's prone to typos. compiler cannot check that you are using the right string.
(actually myHostName doesn't seem less readable to me than "my.host.com").
anyway, it's not going to lead to a tragic error in this file so not a biggie, up to you.
There was a problem hiding this comment.
I'm not writing code for the compiler :-) The proper way to do this test is to move it to a file and
read the file - in which case there will be no constant. I wrote it this way only because I am not sure what future this code has and it may not be worth adding too many more tests for ingress if we'll get rid of it soon...
The test was mostly to help debugging and fixing the code - actual coverage will be in the test env (separate PR updating the stability cluster - and someone will need to bring back ingress tests in simple e2e).
| virtualServices := mergeIngress(allVirtualServices) | ||
|
|
||
| virtualHosts := make([]route.VirtualHost, 0, len(virtualServices)) | ||
| dedupHosts := map[string]*model.Config{} |
| vs := c.Spec.(*networking.VirtualService) | ||
| for _, h := range vs.Hosts { | ||
| if !strings.HasSuffix(c.Name, model.IstioIngressGatewayName) { | ||
| vse, e := hostsToVS[h] |
There was a problem hiding this comment.
see earlier comment about testing for map keys.
| vs := c.Spec.(*networking.VirtualService) | ||
| for _, h := range vs.Hosts { | ||
| if strings.HasSuffix(c.Name, model.IstioIngressGatewayName) { | ||
| vse, e := hostsToVS[h] |
| if s.Tls == nil { | ||
| log.Warnf("TLS server without TLS options %s %s", name, s.String()) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Please remove this. I have added validation to ensure that HTTPS/TLS servers should have TLS settings and plain text HTTP servers cannot have any TLS settings.
There was a problem hiding this comment.
https://github.com/istio/istio/pull/7175/files#diff-5a64dc2088020f89bc6d77354b9ebb22R414 .. Will ensure that you cannot have a TLS server with non TLS port, and vice versa.
There was a problem hiding this comment.
Got it, thanks - will merge your PR and remove this. Forgot to check - do you warn with enough info or have a counter ? I found it very hard to use the errors in pilot without having the namespace and name of the config.
There was a problem hiding this comment.
Its part of validation. So if using istioctl, user will see the error immediately
| if old, exists := listenerMap[listenerMapKey]; exists { | ||
| env.PushStatus.Add(model.ProxyStatusConflictInboundListener, node.ID, node, | ||
| fmt.Sprintf("Rejected %s for %s", instance.Service.Hostname, listenerMapKey)) | ||
| fmt.Sprintf("Rejected %s, used %s for %s", instance.Service.Hostname, old.Service.Hostname, listenerMapKey)) |
There was a problem hiding this comment.
I think this is outdated.. @nmittler revamped this logic. We now print all services that conflict. So you can undo the changes to this function
There was a problem hiding this comment.
oh ignore.. this is for inbound
* Remove api.* attributes from accesslog as they are yet to be implemented (#7019) * mixer: setup initial liveness probe (#7065) * Setup initial liveness probe for Mixer * Prefer /version to /metrics * remove unused legacy v1alpha1 routing apis (#7091) * Updating Kiali to v0.5.0 in Helm Installer (#7007) * Updating Kiali to v0.5.0 * Adding quotaspecs and quotaspecbindings in clusterole * Upgrading Kiali Versrion on Requirements File * better logging for attribute bag (#7031) Signed-off-by: Kuat Yessenov <kuat@google.com> * Istio-remote helm: allow setup pilot svc & endpoint for controlPlaneSecurity (#7094) When controlPlaneSecurityEnabled the spiffe configured by sidecar injector assumes that the pilot discoveryAddress is a hostname.namespace formatted string. This change makes the istio-remote helm chart setup a selectorless service and endpoint from the pilotAddress when the remotePilotCreateSvcEndpoint arg is true and sets the MeshConfig discoveryAddress to be the hostname for istio-pilot. * remove MeshPolicy/DefaultDestRule when mtls isn't enabled (#7095) * Change job name to indicate release version (#7092) * Polishing for messages, reporting, metrics (#6946) * Make version, timestamp available in service for k8s * More error and metrics cleanup * Address Shriram's comments * Format * Fix verbose log, format, init * More work - the push is async, and things are trickier than they look * Rename mixer job, so upgrade works * Add back iperf3 without sidecar * Format * Switch to *env, no copy by value * Cleanup, resolve conflicts * Also convert node to pointer, and add explicit param * Polish, cleanups * Revert changes moved to other PRs * Logging too much * Polish, less log * Extra safety, sometimes a stale message is stuck until config change * fmt and lint errors * Addres review comments * Add the ttls test * Revert the review fix - it breaks tests (some send old style). Will update post 1.1 * Fix for ingress conflict with tracing ingress (#7039) * force to use proxyv2 for istio-telemetry pod (#7111) * delete unix socket, before calling listen (#7110) * typo: connctionDuration -> connectionDuration (#7134) * Add RbacConfig CRD to Helm chart (#7125) * Fix two data races (#7119) * TCP routing cleanups/bug fixes: part 1 (#7101) * TCP routing cleanups: part 1 Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * missing sort Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * sort destination cidrs * proxy sha update * undo istio proxy sha * fixups * compile fixes * nil check Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * nits Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * Update proxy sha based on Piotrs changes * undo Piotrs changes * add reporterKind attribute to accesslog and tcpaccesslog (#7147) * Improve outbound listener conflict messages (#7113) This is needed to better support e2e testing of conflicts. * Update accesslog config (#7158) * Update hub and tag in values.yaml in use daily releases (#7116) * add latest tag to values.yaml * switch latest in istioctl * configurable mixer port (#7156) Signed-off-by: Kuat Yessenov <kuat@google.com> * Adding unit test for TCP over TCP conflicts. (#7160) * Adding unit test for TCP over TCP conflicts. * addressing comments. * addressing comments. * addressing comments. * Assorted small fixes for setup (#7123) * Assorted small fixes * spaces * Fix certmanager (#7141) * Adding e2e test for outbound listener conflicts (#7173) * remove static route generation. improve gateway validation (#7175) * remove static route generation Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * more validation for gateways Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * validation fixes Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * fix test Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * Updating Kiali to v0.5.0 in Ansible Installer (#7042) * Multicluster make zipkin conditional (#7085) This change makes the zipkin arguement in the sidecar config map to be conditional based on zipkin being used in remote clusters. * Yet another place of the connctionDuration typo (#7177) * Put back citadel templates and re-generate them when running updateVersion.sh (#7178) * make bookinfo sample build repeatable (#7152) * make bookinfo sample build repeatable * update bookinfo image references to 1.8 * delete me later-- reference temporary images * remove v1 registration from readiness probe and use debu/endpointz (#7189) * bookinfo fixes (#7109) * update bookinfo productpage service * update bookinfo reviews service * update routing rules * fix me after-- update image references * update e2e tests * fix buildinfo build bug from #6988 * Cleaning up inject test files (#7188) There are a lot of files and its confusing which files are used by inject_test vs webhook_test. Splitting out the files into two subdirectories under testdata/ to make it clear. Making copies of files that are shared. * add TCP mixerfilter test (#7172) Signed-off-by: Kuat Yessenov <kuat@google.com> * Fix broken ingress (#7144) * Fix ingress * Move merging ingress into VirtualService and dedup to separate PR. Add back the previously and still broken gateway generation * Another attempt to avoid duplicated vhosts and merge ingress * Missed a pointer * Remove again mergeIngress * Improved messages. Will need to be turned into metrics soon * Merge, assorted fixes * Leave the push param in gateay, it will be needed later * deps: update proxy (#7196) * update sha Signed-off-by: Kuat Yessenov <kuat@google.com> * merge Signed-off-by: Kuat Yessenov <kuat@google.com> * fix tests Signed-off-by: Kuat Yessenov <kuat@google.com> * typo Signed-off-by: Kuat Yessenov <kuat@google.com> * rbac: add e2e tests for envoy based rbac. (#7193) See #6377. * Cleanup default metrics configuration (#7034) * WIP: Cleanup default metrics configuration * Remove last vestige of destination_namespace * Update accesslog config * Update syntax used to configure xDS servers. (#7201) Signed-off-by: Piotr Sikora <piotrsikora@google.com> * Update go-control-plane to 8cef83f9. (#7202) Signed-off-by: Piotr Sikora <piotrsikora@google.com> * bump bookinfo version (#7195) Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * Add contents of install/ansible to release script (#7194) Fixes: #4359, istio/old_issues_repo#223 * update for 1.0.0 path (#7088) * update api sha (#7204) * Delete namespace after tests when using helm (#7051) * Delete namespace after tests when using helm Signed-off-by: Neil Hickey <nhickey@pivotal.io> * Do not return error if namespace deletion fails If the helm chart being deleted has the namespace manifest then the HelmDelete would've already deleted the namespace * Add an e2e test for RedisQuota Adapter (#6720) * Add an e2e test for RedisQuota Adapter Fix RedisQuota Unit Test and lint error in kuberntes.go Remove additional log added in redisquota adapter delete the route rule after test finishes in mixer_test.go Fix vectorValue function and remove local port forwarding to redisquota server Add an e2e test for RedisQuota Adapter Fix RedisQuota Unit Test and lint error in kuberntes.go Remove additional log added in redisquota adapter delete the route rule after test finishes in mixer_test.go Add an e2e test for RedisQuota Adapter Fix RedisQuota Unit Test and lint error in kuberntes.go Remove additional log added in redisquota adapter delete the route rule after test finishes in mixer_test.go Fix vectorValue function and remove local port forwarding to redisquota server Add an e2e test for RedisQuota Adapter Fix RedisQuota Unit Test and lint error in kuberntes.go Remove additional log added in redisquota adapter delete the route rule after test finishes in mixer_test.go Remove destination version label for quota test Make calculations more tight for RedisQuota Format test Format test * Fix tests based on metrics change * Scale istio-policy pod to 2 for redisquota test * Fix spelling mistake in comment in kube_utils.go * Fix lint error with mixer_test.go * Fix lint error with mixer_test.go * Fix path for networking and policy rules * Move Kubernetes Ansible to proper location (#7207) * Fixing a wrong Json key for the CreationTimestamp field (#7206) * rename key for istio-remote chart repository->hub (#7209) * Move "repository" to "hub" and hardcode image name (#7187) * Move "repository" to "hub" and hardcode image name The imagename hard code comes from the pattern laid down in certmanager. I'm not super keen on this as it makes determining the image names for airgapped installs a bit more difficult. The good news is we can always add an "image" field for each service if we want to expose that information in values.yaml or just document it appropriately. * Move hyperkube back to globals * Fix a typo * Fix error in change * Sort configs by creation time before returning (#7216) * Sort configs by creation time before returning Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * format * Moved RBAC CRDs to group rbac.istio.io from config.istio.io (#6874) * Moved RBAC CRDs to group rbac.istio.io from config.istio.io * Review comments addressed * Modify global RbacConfig group too * Updated new files added in recent PRs * Updated a recently added RbacConfig CRD too * rbac.istio.io/v1alpha2 -> rbac.istio.io/v1alpha1 * Change protos version to v1alpha1 * Updated a test file that was recently added * Add useful warnings when quota specs do not match (#7226) * Add named logging scope for model * Add warning and tests to QuotaSpecByDestination 1. Add warning messages under correct conditions to inform user that some common error conditions were encountered. 2. Add test coverage for QuotaSpecByDestination. 3. Add previously omitted support for simple wildcard match. * review comments * update envoy sha - fix memory leak (#7232) Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * More debug/troubleshooting (#7227) * More debug/troubleshooting * Build error * Fix istio-remote sidecar-injector-configmap rendered yaml parse error (#7245) Fixes: #7244 * use receive only channel (#6960) * Tool to aggregate third party licenses (#7220) * Tool to aggregate third party licenses * Clean up and add match info as an option * Add readme, change binary name * Update readme, make --branch optional * Don't run licensee if not required * Lint * part 2 TCP routing cleanups/bug fixes for header matcher (#7236) * update envoy sha - fix memory leak Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * Fixing assorted bugs in service entries using filter chain matches Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * format * bug fix Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * lint Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * lint Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * multicluster bug fixes Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * backward compatibility Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * use new header match specifier Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * mixer: add sidecar health probe (#7102) * sidecar health Signed-off-by: Kuat Yessenov <kuat@google.com> * typos Signed-off-by: Kuat Yessenov <kuat@google.com> * remove tracing of health checks Signed-off-by: Kuat Yessenov <kuat@google.com> * change to 15093 Signed-off-by: Kuat Yessenov <kuat@google.com> * Add proxy version to the proxy-status command (#7269) * use new websocket option and remove deprecated one (#7247) * use new websocket option and remove deprecated one Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * backward compatibility Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * consistency Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * cleanup Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * dont append upgrade configs * remove requestedServerName attribute (#7278) it does not work, requires debugging and possibly implementation, will not be in time before the release * Handle virtual service sni_hosts matches in gateway. (#7192) * dirty poc * working poc * comments + cleanup * lint * add simple e2e test to egress * minor refactoring * cleanups, lots of comments Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * more duplication for clarity Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * simplifying gateway opaque tcp logic * final cleanups Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * nil pointer check Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * Add add operation into expression language (#6776) * add op. * fix duplicated test. * Mixer Conditional Quota (#7265) * add tests for conditional and unknown quota * add conditional and unknown quota * Option to disable pilot sidecar (#7280) * Remove whitespace between host: and port in 'istioctl authn tls-check' (#7084) * Remove whitespace between host: and port * Typo
Ingress was not working at all - this change makes it work enough to get certmanager to get certs, and will likely work for most common uses of Ingress.
What is not supported ( and didn't work before either ) is the TLS part: users should manage certificates using the Gateway config and associated secretes in ingressgateway. Since getting a cert and configuring the secret loading are complex operations - setting a proper Gateway is not a huge extra step, while loading them from Ingress resources would be confusing and complex.
With the new code:
To allow use cases like certmanager, the generated Ingress is merged with user-specified VirtualService with same host by appending Ingress-defined rules at the end. Probably not perfect - but should work
well enough while users migrate away from Ingress and to support other apps creating Ingress using k8s APIs.
Note that since no gateway is generated, users will need to great a Gateway with the proper name and select their desired gateway in order for ingress to be active. This can be added to the current ingress deployment or it can point to ingressgateway, both seem to work. We can discuss if we still want to keep the old ingress load balancer/IP/workload.