Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ingress Group Feature Testing #914

Open
M00nF1sh opened this issue Apr 5, 2019 · 36 comments

Comments

Projects
None yet
@M00nF1sh
Copy link
Collaborator

commented Apr 5, 2019

I’d like to give you an update about the ingress group feature for ALB Ingress Controller.
It’s still in alpha stage, please don’t use it in production environment.
I’m really appreciate if you can test it and give feedbacks on this feature.

Here is the docker image: docker.io/m00nf1sh/aws-alb-ingress-controller:ingress-group-v1, and code is available here: https://github.com/M00nF1sh/aws-alb-ingress-controller/tree/ingress-group (If you’d like to build it yourself, simple make docker-build is enough 😃).

Here are the steps to install it

  1. You need to delete all existing ingresses and previous aws-ingress-controller if you have.
    I’ll provide an patch update to the original aws-ingress-controller so that old aws resources(alb/tg/sg/etc) provisioned can be preserved in this new version, so users can upgrade seamlessly. (In this new version, aws resource are identified based on tags instead of names, so ideally you can bring your own alb/targets etc, but there is no plan to support such case offically).

  2. Install this new version using same steps as before, but use image docker.io/m00nf1sh/aws-alb-ingress-controller:ingress-group-v1.

Here are the steps to test it

  1. Create ingress resources as you originally do, and attach annotations. Almost all(keep updating :D) old annotations should work as before, and we’ll provision one ALB per Ingress resource.
  2. You can add annotation alb.ingress.kubernetes.io/group.name: yourgroupname to your ingresses(even across namespaces) that should be hosted with single ALB.
    1. Simply add that annotation to all your ingresses that should be same group, and apply it will be enough. The groupName shouldn’t contain “/”, and might be limited to [a-z0-9]{1,10} in the future.(based on feedbacks).
    2. Ingresses can join group(add that annotation), leave group(remove that annotation) and change group(change the groupName). Currently it will works as expected, but we may drop the support for this, since the group should be statically decided by application owners before create ingresses.
  3. You can add annotation alb.ingress.kubernetes.io/group.order: 1-1000 to change the rule ordering between ingresses.
    1. By default the rule order between ingresses are determined by ingress’s namespace/name.
    2. You can explicitly denote the order using a number between 1-1000(you need to quote numbers in yaml with “”).
    3. The smaller the order, the rule will be evaluated first. (all ingresses without explicit order setting get order value as 0). And You cannot have two ingress with same explicit order.

Typical use cases:

  1. Share ALB for ingresses across namespaces.
  2. Specify different rule for different ports by create two ingress with same group, with different listen-ports settings.
    1. Some settings are aggregated by ports. (e.g. http routing rule /ssl certs are merged together for same port).
    2. Some settings must be specified on a single ingress. (e.g. ssl policy/inbound cidrs for same port, you still can specify different inbound cidr for different port in different ingress)

I’ll keep update the image and code, and you can reach me on slack by @M00nF1sh 😃.

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 5, 2019

The current approach is built an AWS data model first and deploy it. (the data model is logged in controller logs, so if you encountered any bugs, please send me these logs).
It should only take a few hours to write an cloudformation based deployer, but i hesitate to do it due to some critical cloudformation bugs i found during testing. and it's not possible to maintain backwards compability for existing stack.

@allanyung

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

I tried using this but unfortunately it fails with an error about the Security group being invalid

{
    "level": "error",
    "ts": 1555051159.8551583,
    "logger": "kubebuilder.controller",
    "msg": "Reconciler error",
    "controller": "alb-ingress-controller",
    "request": "/SOME_PATH",
    "error": "ValidationError: Security group 'SOME_SECURITY_GROUP' is not valid\n\tstatus code: 400, request id: SOME_ID",
    "stacktrace": "sigs.k8s.io/aws-alb-ingress-controller/vendor/github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/src/sigs.k8s.io/aws-alb-ingress-controller/vendor/github.com/go-logr/zapr/zapr.go:128\nsigs.k8s.io/aws-alb-ingress-controller/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/src/sigs.k8s.io/aws-alb-ingress-controller/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:217\nsigs.k8s.io/aws-alb-ingress-controller/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func1\n\t/go/src/sigs.k8s.io/aws-alb-ingress-controller/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:158\nsigs.k8s.io/aws-alb-ingress-controller/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil.func1\n\t/go/src/sigs.k8s.io/aws-alb-ingress-controller/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133\nsigs.k8s.io/aws-alb-ingress-controller/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/go/src/sigs.k8s.io/aws-alb-ingress-controller/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:134\nsigs.k8s.io/aws-alb-ingress-controller/vendor/k8s.io/apimachinery/pkg/util/wait.Until\n\t/go/src/sigs.k8s.io/aws-alb-ingress-controller/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88"
}

The configs are exactly the same as they were before with the exception that I added the alb.ingress.kubernetes.io/group.name annotation to the ingresses. The logs say that the model was successfully built. I don't want to post it here without anonymising some of the data, but that may render it useless to you. Let me know if it's needed. Cheers

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2019

@allanyung Hi, would you share the securityGroup name to me? It's based on the groupName, and i haven't do validations to it. In the mean while, you can change groupname to [a-z0-9] only.

@allanyung

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

Ah, the security group name does contain hyphens. I expect that's why it isn't working. The groups are created in advance by another process. Will hyphens be supported or is it a dealbreaker?

I'm won't be in a position to try this again for about a week, but am happy to try again if it would still be helpful at that time.

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2019

Ah, the security group name does contain hyphens. I expect that's why it isn't working. The groups are created in advance by another process. Will hyphens be supported or is it a dealbreaker?

I'm won't be in a position to try this again for about a week, but am happy to try again if it would still be helpful at that time.

hyphens is fine. It will be super helpful if you can share your group name to me 😄

@allanyung

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

I can share the security group name and group name, nothing sensitive there

security group name: app-alb-ingress
group name: apps

Hope it helps!

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2019

@allanyung Just fixed it 😄 Delete & reinstall the controller will solve it 😄

@nisan270390

This comment has been minimized.

Copy link

commented Apr 16, 2019

@M00nF1sh this is great feature, thanks a lot :)
We've one ALB which serve 5 different services (outside k8s), the routing configured by path filter.
We want to migrate our existing services to k8s step by step, Is there a way can we assign the existing ALB which spin up through cloudformation, and route traffic for /api/customers for example to service powered by k8s and another routes to existing service target groups (cloudformation)?

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 16, 2019

@nisan270390 Currently it's not possible, since the controller don't know how to cooperate with existing rules on ALB.
An alternative solution is make EndpointBinding an CRD, so you can manually create targetGroup & add rules, and let the controller manage the targets for that targetGroup to be sync with k8s service. I haven't exposed it as CRD mainly because this feature won't be needed for everyone, and require these ppl to install an extra CRD seems not good.

BTW, have you considered this way to migrate existing service?

@nisan270390

This comment has been minimized.

Copy link

commented Apr 17, 2019

@M00nF1sh thank you, Migrating From Legacy is very helping for our migration to k8s.

@allanyung

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@M00nF1sh tried this again and it looks like it works great!

One thing I would say is that I wanted one specific ingress group to be ordered last, and don't really care about the ordering of the other groups. The current implementation meant that I had to specify the alb.ingress.kubernetes.io/group.order for all the other groups, whereas the ideal situation would be if I could just specify alb.ingress.kubernetes.io/group.order: 1000 for the one group I want resolved last, and have everything else auto resolve to something < 1000.

Would such a change be reasonable? Thanks!

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 18, 2019

@allanyung Yeah, it's make sense. I think the most common use case of explicitly use group.order should be put an catch-all rule at the last. So make the default order to be 0 make sense.
(don't like to make the default order to be like 500, which is hard to use and understand :D)
If it make sense to you, I'll provide a update today.

BTW, Another approach is use the creationTimestamp as default order(nginx controller used that approach), i didn't adopt that cause it's introduced unstable behavior, so hard to implement infrastructure automation.

@allanyung

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@M00nF1sh I think default 0 makes the most sense.

CreationTimestamp sounds like a recipe for disaster 😅

@Annegies

This comment has been minimized.

Copy link

commented Apr 25, 2019

@M00nF1sh Tested the ingress group feature and it works like a charm!
I've tested the grouping of ingresses both inside and cross namespace.

@allanyung

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Hi @M00nF1sh I've encountered something which is probably fine, but looks a bit weird.

We use kustomize to create namespaced versions of our application.

So we have a default that looks like this

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: app
  namespace: default
  annotations:
    kubernetes.io/ingress.class: alb
    alb.ingress.kubernetes.io/group.name: app
    alb.ingress.kubernetes.io/target-type: ip
    alb.ingress.kubernetes.io/scheme: internet-facing
    alb.ingress.kubernetes.io/security-groups: app-alb-ingress
    alb.ingress.kubernetes.io/certificate-arn: SOME_CERT
    alb.ingress.kubernetes.io/listen-ports: '[{"HTTP": 80}, {"HTTPS":443}]'
    alb.ingress.kubernetes.io/actions.ssl-redirect: '{"Type": "redirect", "RedirectConfig": { "Protocol": "HTTPS", "Port": "443", "StatusCode": "HTTP_301"}}'
spec:
  rules:
  - http:
      paths:
      - backend:
          serviceName: ssl-redirect
          servicePort: use-annotation
  - host: SOME_URL
    http:
      paths:
      - backend:
          serviceName: app
          servicePort: 80

and kustomize additionally generates this resource (different namespace, URL, and SSL cert)

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: app
  namespace: non-default
  annotations:
    kubernetes.io/ingress.class: alb
    alb.ingress.kubernetes.io/group.name: app
    alb.ingress.kubernetes.io/target-type: ip
    alb.ingress.kubernetes.io/scheme: internet-facing
    alb.ingress.kubernetes.io/security-groups: app-alb-ingress
    alb.ingress.kubernetes.io/certificate-arn: SOME_OTHER_CERT
    alb.ingress.kubernetes.io/listen-ports: '[{"HTTP": 80}, {"HTTPS":443}]'
    alb.ingress.kubernetes.io/actions.ssl-redirect: '{"Type": "redirect", "RedirectConfig": { "Protocol": "HTTPS", "Port": "443", "StatusCode": "HTTP_301"}}'
spec:
  rules:
  - http:
      paths:
      - backend:
          serviceName: ssl-redirect
          servicePort: use-annotation
  - host: SOME_OTHER_URL
    http:
      paths:
      - backend:
          serviceName: app
          servicePort: 80

They are merged into a single alb. However, this results in the SSL redirection rule appearing multiple times in the listener rules for port 80. It still works fine, but looks a bit odd. I can probably fix this by moving stuff around in kustomize templates so that the SSL redirect stuff doesn't get defined twice. But I guess this could happen via other means and maybe the ingress group feature could deduplicate occurrences of the SSL redirect rule? No worries if that's too difficult or out of scope, things still work regardless. Thanks!

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 26, 2019

@allanyung
Actually I preferer another way to do http->https redirection after ingress group(define different rule for different port in different ingress 🤣 )

(dedupe make sense too, but it might be error prone when we support advanced routing like header/verb/query based, I need to think more about it.)

  1. for all our normal HTTPS rule, create them with alb.ingress.kubernetes.io/listen-ports: '[{"HTTPS":443}]'
  2. create an single ingress in default namespace(or other), with alb.ingress.kubernetes.io/listen-ports: '[{"HTTP":80}]' and the direction rule.

It's more clean than rely on a trick which omit redirection rule on 443 port.

@Spareo

This comment has been minimized.

Copy link

commented Apr 27, 2019

I'm trying this out right now and running into an issue. I have one ALB where I use the alb.ingress.kubernetes.io/inbound annotation for specifying a list of allowed CIDR blocks but I keep getting errors like this

"msg":"Reconciler error","controller":"alb-ingress-controller","request":"/MySecurityGroup","error":"InvalidParameterValue: CIDR block 1.2.3.4/32 is malformed

This results in the security group getting created but not having any rules inside of it.

On the stable release version, I don't get this error.

@enmand

This comment has been minimized.

Copy link

commented Apr 28, 2019

@allanyung

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@allanyung
Actually I preferer another way to do http->https redirection after ingress group(define different rule for different port in different ingress )

(dedupe make sense too, but it might be error prone when we support advanced routing like header/verb/query based, I need to think more about it.)

  1. for all our normal HTTPS rule, create them with alb.ingress.kubernetes.io/listen-ports: '[{"HTTPS":443}]'
  2. create an single ingress in default namespace(or other), with alb.ingress.kubernetes.io/listen-ports: '[{"HTTP":80}]' and the direction rule.

It's more clean than rely on a trick which omit redirection rule on 443 port.

@M00nF1sh This is a much nicer solution! I can confirm that this works, and there are no longer duplicates in the port 80 rules. Did you get a chance to cut a release where the default group order is 0 instead of 1000? Thanks :)

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 30, 2019

@Spareo
Hi, Thanks for reporting this error. I have fixed the issue, reinstall(delete/apply) the controller will fix it if you have 'imagePullPolicy: Always'. 😄 (Also, IPV6 CIDR should work fine too)

@allanyung
Hi, I have pushed the image to the same tag. reinstall(delete/apply) the controller will fix it if you have 'imagePullPolicy: Always'. The default order is 0, and users can explicitly set it to [1, 1000] 😄

@arminc

This comment has been minimized.

Copy link

commented May 7, 2019

What are the things that need to be done/verified to get this feature to master and released?

@mixman

This comment has been minimized.

Copy link

commented May 9, 2019

An ALB seems to have a 100 rule limit, which I assume means a theoretical 100 service limit (given single path specified).

How does one support over 100 services? Is it possible to have multiple ALB's running for this grouping feature?

@rabbitfang

This comment has been minimized.

Copy link

commented May 9, 2019

@mixman The default rule does not count towards the 100 rule limit, so the theoretical max is 101 services served by a single ALB. It does not appear to be possible to request an increase for that limit, so in order to support more than 100/101 services, you would need to use multiple ALBs (and therefore multiple ingress groups), using host names to partition the ALBs. Since this feature is meant to group multiple ingresses into a single ALB, I doubt it would be able to automatically manage multiple ALBs when you go over the rule limit.

@iverberk

This comment has been minimized.

Copy link

commented May 15, 2019

Hi, we are waiting for this functionality to be merged. Could you give us a status update on the progress and if we can help in any way to get this merged?

@eugenekainara

This comment has been minimized.

Copy link

commented May 15, 2019

Hello, I'm also checked this functionality at my EKS - and it works good for my use case. Now I have just 2 ALB instead of 6. Thanks!
When we can expect this feature to be merged?

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented May 15, 2019

Hi, sorry the late update. I didn't get enough time to thoroughly test all features out.(it's almost rewrite and needs thoroughly testing). I'll submit an PR to merge this by next week.

@EricDube

This comment has been minimized.

Copy link

commented May 21, 2019

@M00nF1sh I ran into a similar issue to the CIDR block malformed but I believe its from using ip-address-type dualstack where staging is my stack.

"msg":"Reconciler error","controller":"alb-ingress-controller","request":"staging/staging-ingress","error":"InvalidParameterValue: CIDR block ::/0 is malformed

It is creating all the target groups and a security group with no inbound rules.

@arminc

This comment has been minimized.

Copy link

commented May 29, 2019

@M00nF1sh any date on the PR? Don't want to pressure but there is a lot of need for it. If I can do something to help let me know.

@M00nF1sh

This comment has been minimized.

Copy link
Collaborator Author

commented May 31, 2019

@arminc
There is still two pending implementation left in my dev environment to process with this.

  1. omit worker node securityGroup. (currently, we are creating an worker node SG per ingress, in the newer version, it will directly use the existing securityGroup on worker node with k8s tag instead).
  2. add existing tags on resources to align with the tagging strategy with the newer version. (the newer version uses tags instead of names to identify existing resources, by doing this, it will allow us to preserve ALB/TargetGroup/SG created via old alb controller version when migrate to new alb controller version).
    I'm still working on the two implementations to ensure there are smooth migrations between versions.
@rverma-nikiai

This comment has been minimized.

Copy link

commented Jun 11, 2019

@M00nF1sh eagerly waiting for this pr to get merged.

@rafaelgaspar

This comment has been minimized.

Copy link

commented Jun 12, 2019

I've tested it and it's working like it should, only thing is right now it's not respecting --alb-name-prefix.

@runningman84

This comment has been minimized.

Copy link

commented Jun 12, 2019

I've also tested it. Compared to the traefik ingress the path handling seems to be different.

In traefik I can access all deep links like /wp-login.php in alb I can only access / with this config:

Name:             demo-wordpress
Namespace:        demo
Address:          k8s-wordpress-96977de690-941162455.eu-central-1.elb.amazonaws.com
Default backend:  default-http-backend:80 (<none>)
Rules:
  Host                                           Path  Backends
  ----                                           ----  --------
  demo.example.com  
                                                 /   demo-wordpress:http (10.1.40.88:8080)
Annotations:
  alb.ingress.kubernetes.io/group.name:                 wordpress
  alb.ingress.kubernetes.io/target-type:                ip
  traefik.ingress.kubernetes.io/preserve-host:          true
  traefik.ingress.kubernetes.io/redirect-entry-point:   https
  alb.ingress.kubernetes.io/certificate-arn:            arn:aws:acm:eu-central-1:xxxxx:certificate/yyyyy-d094-4e5e-b75d-4427810762a6
  alb.ingress.kubernetes.io/listen-ports:               [{"HTTPS": 443}]
  alb.ingress.kubernetes.io/scheme:                     internet-facing
  ingress.kubernetes.io/ssl-proxy-headers:              X-Forwarded-Proto:https
  kubernetes.io/ingress.class:                          alb
  traefik.ingress.kubernetes.io/frontend-entry-points:  http,https
Events:                                                 <none>

It looks like the alb rules have a fixed path match. I would expect alb to do not create a path rule if there is only a single path like /.

@rifelpet

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

You need to use a path of /* rather than /

@runningman84

This comment has been minimized.

Copy link

commented Jun 12, 2019

which is a pain if you want to easily switch between ingresses.... this issue seems to mention the same problem:
#699

@rverma-nikiai

This comment has been minimized.

Copy link

commented Jun 13, 2019

I do prefer /* over / since it's consistent with alb behaviour. Also gce ingress controller behaves same and various helm charts(e.g. spinnaker) are following the convention. Having this change will break compatibility across exisiting services

@xxthegonzxx

This comment has been minimized.

Copy link

commented Jun 13, 2019

Let's get this bad boy in soon! I appreciate all the hardwork. You're a rockstar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.