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

Implement V1 of certificate_transfer #407

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

saltiyazan
Copy link

Issue

The new version (V1) of the certificate_transfer_interface is incompatible of its V0 version. Applications that implement V1 of cert transfer won't be able to share their CAs with Traefik.

Solution

This change is to allow application the implement the V1 version of the interface to integrate with Traefik to provide their CAs that Traefik needs to trust.
Note: The solution might seem a bit hacky, I tried to follow the same solution with IngressPerAppV0 and IngressPerAppV1.

Context

Testing Instructions

  • Deploy Traefik from this branch
  • Deploy notary-k8s from this branch
  • Deploy self-signed-certificates
  • Integrate Notary and self-signed-certificates using the certificates interface
  • Integrate Notary and Traefik using the receive-ca-cert-v1 interface
  • Observe the CA file being created in the correct directory in the Traefik container
  • Remove the integration and observe the file being removed

Upgrade Notes

@saltiyazan
Copy link
Author

Hi, @sed-i @simskij
Could someone please take a look at this? happy to re-work it if needed.

metadata.yaml Outdated
Comment on lines 104 to 110
receive-ca-cert-v1:
interface: certificate_transfer
description: |
Receive a CA certs for traefik to trust.
This relation can be used with a local CA to obtain the CA certs that was used to sign proxied
endpoints.
This is meant for applications that use the certificate-transfer-interface v1.
Copy link
Contributor

Choose a reason for hiding this comment

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

My uninformed first impression is that I dislike adding a new relation to handle a different version of the same interface. This feels like a change that should be transparent to the user if at all possible. Ideally, the existing receive-ca-cert relation should just work for both v0 and v1. Is that possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the interface is the same, we shouldn't use a new relation. The promise is supposed to be that any provider of an interface works with any consumer.

Copy link
Author

Choose a reason for hiding this comment

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

V0 and V1 are not compatible. There was breaking change in the interface too not only the library.
Here you can see examples of the data expected in V0 and the data expected in V1.

Isn't it the same with Ingress V1 and V2?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same for ingress but handled differently not to require a new relation, traefik only has one relation for ingress and we do our checks internally within the charm code to validate if client is talking v0/v1

Copy link
Author

Choose a reason for hiding this comment

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

I see your point thank you.
I'll update the PR.

Copy link
Author

Choose a reason for hiding this comment

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

I updated the PR to follow the example of Ingress.
Could you please take a look @IbraAoad ?

@saltiyazan saltiyazan requested a review from IbraAoad December 5, 2024 10:31
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