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

R53 int1 #13

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

R53 int1 #13

wants to merge 3 commits into from

Conversation

smarunich
Copy link
Collaborator

@smarunich smarunich commented Oct 12, 2023

@smarunich smarunich marked this pull request as draft October 12, 2023 15:30
Makefile Outdated
@@ -15,7 +15,7 @@ deploy_infra_%:
@/bin/sh -c './make/infra_$*.sh deploy'

.PHONY: deploy_addons
deploy_addons: deploy_addons_load-balancer-controller deploy_addons_fluxcd deploy_addons_external-dns ## Deploy the default addons
deploy_addons: deploy_addons_load-balancer-controller deploy_addons_fluxcd deploy_addons_external-dns #deploy_addons_route53-controller## Deploy the default addons
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let's uncomment route53-controller if it is ready to test

Makefile Outdated
@@ -34,6 +34,7 @@ demo_01-deploy-application: demo_01-deploy-application ## Deploy the demo applic
demo_02-mtls: demo_01-deploy-application demo_02-mtls ## Lunch the mTLS demo
demo_03-zero-trust: demo_01-deploy-application demo_03-zero-trust ## Lunch the Zero Trust demo
demo_04-publish-service: demo_01-deploy-application demo_04-publish-service ## Lunch the Service Publishing demo
#demo_04-r53-publish-service: demo_01-deploy-application demo_04-r53-publish-service ## Lunch the Service Publishing demo
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let's uncomment route53-controller if it is ready to test

Makefile Outdated
@@ -43,7 +44,7 @@ demo_%:
destroy: destroy_infra destroy_local ## Destroy the complete demo stack

.PHONY: destroy_addons
destroy_addons: destroy_addons_external-dns ## Destroy the infra-integrated addons
destroy_addons: destroy_addons_external-dns destroy_addons_load-balancer-controller## Destroy the infra-integrated addons
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need to be added to destroy

@@ -21,13 +21,14 @@ module "external_dns" {
k8s_client_token = data.terraform_remote_state.k8s_auth.outputs.token
oidc_provider_arn = data.terraform_remote_state.infra.outputs.oidc_provider_arn
cluster_oidc_issuer_url = data.terraform_remote_state.infra.outputs.cluster_oidc_issuer_url
vpc_id = data.terraform_remote_state.infra.outputs.vpc_id
cluster_oidc_id = trimprefix (data.terraform_remote_state.infra.outputs.cluster_oidc_issuer_url, "https://")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

external-dns have to be removed in favour to route53 controller...

@@ -52,7 +52,7 @@ spec:
http:
- name: bookinfo
port: 80
hostname: bookinfo.tse.tetratelabs.io
hostname: bookinfo.$cluster_name.aws-ce.sandbox.tetrate.io
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

$cluster_name.$external_dns_aws_dns_zone or similar to it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -33,6 +33,29 @@ if [[ ${ACTION} = "deploy_load-balancer-controller" ]]; then
cd "../../.."
fi

if [[ ${ACTION} = "destroy_load-balancer-controller" ]]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it should go as a separate PR anyway, the focus is route53 controller

variable "external_dns_annotation_filter" {
default = ""
}
# variable "external_dns_annotation_filter" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this cleanup activity? should be another PR for cleanup or if its required for changes should be deleted.

}

provisioner "local-exec" {
when = destroy
command = "sh ${self.triggers.output_path}/${self.triggers.name_prefix}-external-dns-aws-cleanup.sh"
command = "sh ${self.triggers.output_path}/${self.triggers.cluster_name}-external-dns-aws-cleanup.sh"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

external-dns dependencies should be fitted into route53 controller module, technically we should remove external-dns and commit a new route53 controller module so it be correlated easier...


variable "interval" {
}
# variable "interval" {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whatever is not needed have to be removed

@smarunich
Copy link
Collaborator Author

in general external-dns module as being fitted for route53-controller module, so let's basically rename the module to route53-controller... do the final touches so we can start testing, the direction looks okay to me.

@AlexeySkvortsov
Copy link
Contributor

@smarunich @shamusx comments have been addressed in the latest commit. please review.
next:

  • demo sh script for 04-publish-service in case "$external_dns_zone" != "null" --- separate PR
  • readme file for updated tse-sandbox with external dns and r53-controller --- separate PR

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.

3 participants