Ingress annotations should be set globally by hephy-controller

[Cryptophobia]

hephy-controller or workflow deployment values.yaml should set the ingress annotations for the ingress controller that it prefers to use as default globally. Either it is nginx, kong, or istio, this ingress should be defined globally for all apps as ingress can be of different type when using multiple ingress controllers.

https://github.com/nginxinc/kubernetes-ingress/tree/master/examples/multiple-ingress-controllers

[kingdonb]

To add onto that, you should be able to set any ingress annotations regardless of what ingress you are using. For example, you might want to set tls-acme so that cert-manager will handle your certs:

annotations:
    kubernetes.io/ingress.class: addon-http-application-routing
    kubernetes.io/tls-acme: "true"

Should we just go ahead and provide a generic interface to add any desired ingress annotations like we already have in place for .router.deployment_annotations and .router.service_annotations? Does this still make sense to be a setting under .router given that ingress obviates the use of the router component? I think it does, but I'd consider putting it somewhere else if anyone speaks up.

(I assume those settings are still honored by the controller even if router is not used.)

[Cryptophobia]

I am not sure the router deployment object exists when deploying with --set global.experimental_native_ingress=true. We will have to check that. It would be nice if we can keep the annotations at the same place but it might be better to keep them somewhere else too because they would no longer be related to the router object and that would be confusing.

I think it's best that some annotations need to be managed by the users/set manually while others can be global and therefore we can kept as an ingress_annotation object on the hephy-controller data model or something similar...

[kingdonb]

I agree that a more granular approach is probably better, I also think I remember reading in a software design textbook somewhere that you should not give your users an open-ended rope to hang themselves with, basically what service_annotations and deployment_annotations currently do, to replicate that with ingress_annotations might be the same... or, it might be the expected degree of freedom, as these are all Kubernetes standard resources and they should behave the same anywhere that Kubernetes conformance suite compliance is guaranteed...

I think the section on rope was near the paragraph on "No Knobs"

Yes, I found it, here, this is what I'm thinking of:

https://github.com/concourse/concourse/wiki/Anti-Patterns#kitchen-sink-config

By allowing the wildcard "set any env vars or config flags you want", we can no longer know what features people are relying on, and the code becomes dangerous to refactor.

It is indeed a few chapters after "No Knobs"

https://github.com/concourse/concourse/wiki/Anti-Patterns#knobs

[Cryptophobia]

Yes, I agree. Thank you for the links @kingdonb. We cannot cover all scenarios of annotations so it is definitely an anti-pattern to assume we can cover all possible annotations even now. Not to speak about future. Therefore, I think manual annotations on the ingress objects is still unavoidable for many ingress implementations.

The thing I want to set is default fine-tune parameters for applications in hephy globally. For example:
kubernetes.io/ingress.class:, kubernetes.io/tls-acme... Basically all global settings that may not be specific to ingress-type or implementation of each ingress.

[kingdonb]

OK. As a compromise I think we can start with two knobs that are pre-defined, and users can add your own knobs as needed. I would propose we plan to remove the experimental_native_ingress value from the global section in a future release, but until then, both values are accepted.

The new standard would be to set ingress.enabled: true like it is done in every other Helm chart. I think that ingress could be a section under controller, eg. controller.ingress.enabled.

Here is an example ingress section adapted from another chart that we might use for config, it is adapted to reflect that users will not be setting hosts: at all since those are created dynamically by the controller along with each app.

You can set your own annotations. platform_domain is still set directly on the controller as it has always been. You can enable tls and cert-manager or kube-lego is not enabled by default. The settings shown are an example config and are not reflective of what I'd expect to see as defaults...

controller:
  app_pull_policy: ..
  registration_mode: ..
  platform_domain: example.com
  ## Configure the controller to dynamically generate ingress resources for new applications and
  ## for custom app domain settings.
  ## ref: http://kubernetes.io/docs/user-guide/ingress/
  ##
  ingress:
    ## Set to true to enable ingress record generation.
    enabled: true
    ## Set this to true in order to enable TLS on the ingress records the controller will generate.
    tls: true

    ## If TLS is set to true, you may provide a secret that will store the wildcard key/certificate for TLS
    ## If you're using cert-manager, this is unneeded, as it will create the secret for you if it is not set.
    # platformTlsSecret: star-example.com

    ## Ingress annotations done as key:value pairs
    ## Please be sure that ingress.class is set to the correct value for your ingress controller.
    ## Possible values are:
    ## nginx
    ## kong
    ## istio
    ## addon-http-application-routing
    annotations:
      kubernetes.io/ingress.class: addon-http-application-routing
    ## Set this to true if you're using cert-manager.
      kubernetes.io/tls-acme: true

  secrets:
  ## If you're providing your own certificates, please use this to add the certificates as secrets
  ## key and certificate should start with -----BEGIN CERTIFICATE----- or
  ## -----BEGIN RSA PRIVATE KEY-----
  ##
  ## name should line up with the platformTlsSecret set further up
  ## If you're using cert-manager, this is unneeded, as it will create the secret for you if it is not set
  ##
  ## It is also possible to create and manage the certificates outside of this helm chart
  ## Please see README.md for more information
  # - name: deis.example.com
  #   key:
  #   certificate:

Here are the old router config docs for tls: https://docs.teamhephy.info/managing-workflow/platform-ssl/

We should either ensure that the ingresses generated by controller can work with a TLS certificate configured in this way, or ... something like the secrets section above, since you might not use tls-acme, and want to import your own SSL certificate instead. Or we could just fall back on the original deis-router-platform-cert which is already documented for the router in Platform SSL?

(This is a less attractive option, again not only because it says router but also that it says deis)

I think the only case that can't be easily described in a config like the one shown here is the case that you have enabled TLS and provisioned a custom app domain, and you are using hand crafted TLS certificates rather than cert-manager or similar. In this case, I think it's fine for the Hephy controller to provision the ingress with a platform domain certificate, leaving it up to the admin to add the correct TLS certificate and update the ingress rule. (As long as the controller only patches ingress rules after they are created and never replaces them outright, this should work fine.)

[kingdonb]

@Cryptophobia could you assign this to me please, I went looking for this feature today and came back to revisit this issue report when I realized we actually hadn't implemented it yet at all!

[Cryptophobia]

You got it @kingdonb ! Thank you for taking a look into this.