Fix per-app middleware injection

[rbarry82]

Issue

The indentation rules for stripPrefix for per-app ingress were incorrect, and inconsistent with per-unit ingressing.

Solution

Fix the indentation, so it works as expected.

As part of this, extract the "common" configuration block generation logic to a new function (only the actual PathPrefix and list of LB servers once "ingress" is reworked to no longer be only "ingress per leader, which is the only configured LB, whether intentionally or not") to avoid this kind of frustrating indentation error.

Rework the unit tests somewhat so the *_per_app tests actually test per-app.

Finally, fix requirer-side fetching of url in IPA.

Additional information

It's pretty annoying unintended behavior from Traefik that a value which is at the wrong level of indentation successfully parses, but does not apply, so long as it can be deserialized/unmarshaled into a map[str][]string, but this actually breaks if the "entire" middlewares block is moved, because a map[str]interface{} (or whatever the backing datatype/struct the middleware configs are supposed to be unmarshalled into) does not, and Traefik would actually fail to route to the charm completely rather than just silently ignoring the "intended" middleware block.

Release Notes

Fix per-app middleware injection. Unify configuration block generation.

[rbarry82]

The difference here is pretty marginal, but if you look below, the additional selector of traefik_router_name is necessary for this to actually work.

[rbarry82]

Works:

http:                                                                                                                                                                                                                                               
  middlewares:                                                                                                                                                                                                                                      
    juju-sidecar-noprefix-ing-catalogue:                                                                                                                                                                                                            
      stripPrefix:                                                                                                                                                                                                                                  
        forceSlash: false                                                                                                                                                                                                                           
        prefixes:                                                                                                                                                                                                                                   
        - /ing-catalogue                                                                                                                                                                                                                            
  routers:                                                                                                                                                                                                                                          
    juju-ing-catalogue-router:                                                                                                                                                                                                                      
      entryPoints:                                                                                                                                                                                                                                  
      - web                                                                                                                                                                                                                                         
      middlewares:                                                                                                                                                                                                                                  
      - juju-sidecar-noprefix-ing-catalogue                                                                                                                                                                                                         
      rule: PathPrefix(`/ing-catalogue`)                                                                                                                                                                                                            
      service: juju-ing-catalogue-service                                                                                                                                                                                                           
  services:                                                                                                                                                                                                                                         
    juju-ing-catalogue-service:                                                                                                                                                                                                                     
      loadBalancer:                                                                                                                                                                                                                                 
        servers:                                                                                                                                                                                                                                    
        - url: http://catalogue-0.catalogue-endpoints.ing.svc.cluster.local:80   

Does not work:

http:
  middlewares:
    juju-sidecar-noprefix-ing-catalogue:
      stripPrefix:
        forceSlash: false
        prefixes:
        - /ing-catalogue
  routers:
    juju-ing-catalogue-router:
      entryPoints:
      - web
      rule: PathPrefix(`/ing-catalogue`)
      service: juju-ing-catalogue-service
    middlewares:
    - juju-sidecar-noprefix-ing-catalogue
  services:
    juju-ing-catalogue-service:
      loadBalancer:
        servers:
        - url: http://catalogue-0.catalogue-endpoints.ing.svc.cluster.local:80

This is, incidentally, kind of a reason to distrust unit tests arbitrarily. This "passed" despite being wrong because the helper method always used per-unit.

[rbarry82]

As kind of an aside here, this is an excellent example of where scenario-based-testing is not a sufficient replacement for integration testing against the "real" workload, unless the expected results from the scenarios are tested against something similar to the Loki server tests.

It feels somewhat like overkill to spawn threads for "fake" servers in tests, but it's still much faster than integration tests, the ongoing maintenance is reasonably low considering how infrequently workload versions (or major versions with API changes, anyway) are changed, and it's significantly more reflective of something like a "real" operation than just unit testing.

There's an entire separate design discussion which needs to be had about whether or not IPA is actually doing what we think it should be doing, because the intention was probably not "ingress per leader". Similarly, there's one to be had around whether there's a good reason why subdomain/host-based routing and path-based routing are orthogonal, because there's no reason these cannot be combined. And with TLS coming Soon™️ , whether there's a reason we're limiting HostSNI lookups to per-unit only on non-HTTP(s) ingress routes, because there are good use cases which would use "per-app" service-based routing.

Lastly, I'll repeat the "good engineers write bugs (because nobody writes perfect code, and good engineers have high output)" mantra, but it doesn't seem that excessively "fluent" test names are helping us. "Excessively long" may seem subjective, so I'll put it as "long enough that the web-based diff in GH truncates them":

Despite the very, very long names of the tests which don't add any meaningful "metadata" which isn't in the first few lines of the test definition anyway, none of the *per_app tests were actually exercising the per-app relation at all, due to a hardcoded name in the helper function. As usual, unit tests are a leaky approximation of real life at best.

The side-effects from the hardcoded helper which nobody noticed earlier (including me) resulted in both:

• a "bad" (parses but doesn't do what we expect it to) IPA stripPrefix configuration to pass with "expected data" (because IPA was never exercised at all)
• this PR (which is still good because it fixed IPU) not actually doing what it says on the box (it did not, in fact, fix catalogue-k8s, which uses IPA, and that was not exercised, so it required an additional small change to the library here)

I repeatedly missed this in code review also. This issue is probably the first time I've really walked through the code in the charm (hence the other design discussions to be had), and it's easy enough to gloss over it for everyone. This assertion in particular kind of feels wrong in a "spidey-sense" kind of way.

Any development workflow which involved actually running the action against an IPA relation would not see a '/0in the output. Maybe it's because I tend to template these kinds of tests based on running commands against an environment, pasting the output into a test, and adjusting to match hydrated/harness data rather than walking back throughpython-libjuju` or similar to see what the output would be in Python, and it's only 2 characters in any case, but it "feels" wrong.

I'll try to do a better job of reviewing PRs like I'm learning a foreign language, or expecting there to be an off-by-one error, or some other metaphor. Or maybe just be less comfortable with the code. It's becoming very easy to look at a PR and say "tests are green, testcases were modified, the actual logic looks ok, so the tests are probably 👌", and for me, at least, walking into any given PR without that level of comfort will make it more likely that at least my set (among the many eyes reviewing) may spot it.