tetratelabs / tetrate-service-bridge-sandbox

Deploy Tetrate Service Bridge Demo on Azure Kubernetes Service (AKS), Google Kubernetes Engine (GKE) and/or Elastic Kubernetes Service (EKS) using Terraform
Apache License 2.0
12 stars 10 forks source link

Add External DNS add-on #216

Closed nacx closed 1 year ago

nacx commented 1 year ago

Adds an ExternalDNS add-on to automatically configure the DNS names for applications exposed in TSB Ingress gateways. It is default configured to watch the Istio Gateways so that ingress gateways created by TSB will be automatically configured in the DNS. Proper filters have been configured so that only hostnames exposed in the same DNS zone than the MP DNS-name will be generated, to avoid generating DNS records for internal hostnames, etc.

Currently, it only adds it for GCP, but the generic add-on module is already prepared to accommodate the other cloud providers. The only thing that needs to be added to the generic add-on is the provider-specific values file and the provider-specific variables.

Since external-dns requires credentials to update the DNS records and that's different in each cloud provider, I've built it as follows:

nacx commented 1 year ago

this is a perfect start however, we should decouple and make it manageable to reproduce across the three of the clouds, the first call to make:

Starting with one cloud, if everything is backwards-compatible is good enough, even more when adding new features no one is still using. And I rely on you already saying this is an OK approach and that you'd take care of adding the other providers. I count on that after this PR is merged.

external-dns_dependecies per cluster - we should create subzone and related service account external-dns function that actually deploys the function (1) and (2) can be the same function or can be split, it is much easier to make a call when the implementation is being accounted/evaluated/written at least for two clouds.

I think the subzone thing should be discussed. We have the option to use shared, public in the user's project or a private zone in GCP already, and this PR already accounts for that. Instead, I think the FQDN of the other providers should be aligned with this ability to support user zones, and this change keep in alignment with the FQDN module (which is how it works right now).

The logic should follow provision dependencies per given cluster > export the dependencies as variables, not just as a state > supply variables to the external-dns function to satisfy the install

That's great, but can't be done as the private key for the credentials can't be retrieved in a data provider; it's only returned at create time. Since the resource is created only once, but needs to be accessed by all clusters, it needs to be stored locally.

Let me know if you want to re-write it so I won't sound like I am declining your changes...

Definitely no. We already discussed how collaboration works and we should just stick to our protocol guidelines.

we should avoid functions like credentials as they basically a module inside of an environment, so to keep linear and manageable let's keep all the helpers inside the modules.

I can move the credentials module to the generic module folder. However, this is a dependency we have to manage a bit differently given the nature of all the differences between cloud providers. This is exactly the same as the FQDN module. If you look, we have the tsb/fqdn module that we manage separately, for a reason. If this is an OK approach for the TSB module, it should be an OK approach for an add-on.

smarunich commented 1 year ago

apparently the reviews are not showing up in the main thread, so have to be followed under the files review.

nacx commented 1 year ago

To summarize:

so i am bias to the approach, as this is a field facing repo, so somebody has to take a lead on the decisions, and this is my call to tell you that you may disagree, but this is the reasons as we see them as a field team, so if you want to have GCP fork - we can always have a branch that describes and fits your needs, but main story around this repo is not you, but field.

i love the comments and energy, but @nacx this is not a monorepo, so you cannot lead by authority, so please do take into consideration the fact this is not a toy for you to overtake, but a field facing repo that has a consumers beside you and was built as my personal toy project which appeared to be of use to the people so I will still do :) pet this repo, until I am here so :) consensus and balance that what I need you to account here.

Even though this works perfectly fine and is a good starting point (for an add-on, not core functionality), instead of favoring collaboration and an incremental approach, this is going to be pushed back unless it aligns with your mental model. Looks like this repo, given its history has a benevolent dictator, and there is no intention to change that, or at least to have meaningful arguments beyond "this is not yours" or "believe me". benevolent dictators may be great, but that does not favor collaboration, and gatekeeping on this repo towards opinions, instead of favouring incremental progress seem to be what's is being imposed. ACK. but I encourage you to revisit the decision of moving this and getting it back to your personal account if this is going to be the case.

I'm closing this as +30 comments for an add-on is already too much of a discussion that is not worth the effort.