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

externalize traefik container and config management #269

Merged
merged 17 commits into from
Nov 28, 2023

Conversation

PietroPasotti
Copy link
Contributor

@PietroPasotti PietroPasotti commented Oct 10, 2023

Issue

The traefik's charm code is somewhat outdated and needed some freshening up.
Adopting some more modern charming patterns is the start of that process.

Solution

In this PR we add a Traefik class that takes care of all pebble interactions and all traefik-workload-specific configuration tasks.
This way the charm can be unaware of:

  • the yaml config file structure traefik wants
  • the locations at which the traefik files need to be
  • how to manage the traefik process (start it, restart it...)

The charm remains responsible for:

  • juju event handling
  • status management
  • relation interface bookkeeping

Context

A next PR will add a manifest-driven filesystem manager, to prevent unnecessary pushes/restarts unless the backing juju state 'really' has changed.

Testing Instructions

We're going to need careful testing especially in combination with the TLS bundle to catch regressions.

Release Notes

src/traefik.py Outdated Show resolved Hide resolved
src/traefik.py Outdated Show resolved Hide resolved
src/traefik.py Outdated Show resolved Hide resolved
src/traefik.py Outdated Show resolved Hide resolved
src/traefik.py Show resolved Hide resolved
src/traefik.py Outdated Show resolved Hide resolved
src/traefik.py Show resolved Hide resolved
src/traefik.py Show resolved Hide resolved
src/traefik.py Outdated Show resolved Hide resolved
tests/scenario/conftest.py Show resolved Hide resolved
src/traefik.py Outdated Show resolved Hide resolved
src/traefik.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/traefik.py Outdated Show resolved Hide resolved
src/traefik.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
@sed-i
Copy link
Contributor

sed-i commented Oct 19, 2023

Two yamls for the same app, one with http (relation was previously removed), one with https:

$ juju run external-ca/0 get-ca-certificate --format json | jq -r '."external-ca/0".results."ca-certificate"' > external.cert

$ curl --cacert external.cert https://10.43.8.206/pt-prom-0/-/ready
Client sent an HTTP request to an HTTPS server.

$ juju ssh --container traefik trfk/0 ls /opt/traefik/juju
certificates.yaml			  server.cert
juju_ingress_ingress-per-unit_2_prom.yaml  server.key
juju_ingress_ingress-per-unit_3_prom.yaml  # <--- TWO YAMLS FOR THE SAME APP

After manually deleting, it works

Bundle:

bundle: kubernetes
applications:
  ca:
    charm: self-signed-certificates
    channel: edge
    revision: 40
    scale: 1
    constraints: arch=amd64
  external-ca:
    charm: self-signed-certificates
    channel: edge
    revision: 40
    scale: 1
    constraints: arch=amd64
  prom:
    charm: prometheus-k8s
    channel: edge
    revision: 152
    series: focal
    resources:
      prometheus-image: 130
    scale: 1
    constraints: arch=amd64
    storage:
      database: kubernetes,1,1024M
    trust: true
  trfk:
    charm: local:traefik-k8s-0
    series: focal
    scale: 1
    constraints: arch=amd64
    storage:
      configurations: kubernetes,1,1024M
    trust: true
relations:
- - prom:ingress
  - trfk:ingress-per-unit
- - trfk:certificates
  - external-ca:certificates
- - prom:certificates
  - ca:certificates
- - trfk:receive-ca-cert
  - ca:send-ca-cert

@PietroPasotti
Copy link
Contributor Author

PietroPasotti commented Oct 20, 2023

The bug you found is real, but I don't think that's a regression. I've seen that in the past. I think it's because the ipa requirer misses an observer for relation-departed. Adding that to the mix, and let's see if we can make this pass.

By the way, hadn't we made the same fix to IPA? I guess we forgot to port it to IPU as well?

@sed-i
Copy link
Contributor

sed-i commented Oct 23, 2023

I think it's because the ipa requirer misses an observer for relation-departed.

I think that in this context, a bug in the requirer side of things shouldn't matter to the provider side: the provider gets a relation event from juju that should have resulted in deleting the stale file. I'm guessing it's 888's fault.

foo.bndl Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
@sed-i sed-i mentioned this pull request Nov 22, 2023
5 tasks
Copy link
Contributor

@Abuelodelanada Abuelodelanada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
I've only added to minor suggestions.

src/traefik.py Outdated Show resolved Hide resolved
src/traefik.py Show resolved Hide resolved
@PietroPasotti PietroPasotti merged commit 816a337 into main Nov 28, 2023
13 checks passed
@PietroPasotti PietroPasotti deleted the manifest-refactor/traefik-class branch November 28, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants