-
Notifications
You must be signed in to change notification settings - Fork 18
added cert-manager release modules #2137
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
base: master
Are you sure you want to change the base?
Conversation
_sub/security/cert-manager/vars.tf
Outdated
| default = "external-dns" | ||
| } | ||
|
|
||
| variable "namespace" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this wont work and you cannot test that you can actually change this.
_sub/security/cert-manager/vars.tf
Outdated
| type = string | ||
| } | ||
|
|
||
| variable "deploy_name" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please hardcode this one too.
_sub/security/cert-manager/vars.tf
Outdated
| description = "The AWS region the EKS cluster is deployed in" | ||
| } | ||
|
|
||
| variable "is_debug_mode" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be much faster and simpler to just change this value in platform-apps if you need changes. I would remove this one.
| @@ -0,0 +1,3 @@ | |||
| output "namespace_name" { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this output? Is it used anywhere?
| spec: | ||
| serviceAccountName: helm-controller | ||
| values: | ||
| %{ if is_debug_mode ~} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If loglevel isn't a variable no patching is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a variable!
| terraform { | ||
| required_version = ">= 1.6.0" | ||
|
|
||
| required_providers { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of the providers are not even in use, why are they here? You only use the Github one.
compute/k8s-services/vars.tf
Outdated
| # -------------------------------------------------- | ||
| # Cert-Manager | ||
| # -------------------------------------------------- | ||
| variable "cert_manager_is_debug_mode" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this once removed from _sub module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need this
wcarlsen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good, but there are few things that needs adjustments.
Also I think cert-manager would need an AWS IAM role at some point and that is not present in the current setup. Consider adding one using this module https://github.com/terraform-aws-modules/terraform-aws-iam/tree/master/modules/iam-role-for-service-accounts
Really important for DNSChallenge |
I think it will be relevant only when deploying (cluster)issuer(s). so not really needed for the deployment of the cert-manager itself |
Of course with cert-manager you can do self signed certs, but the strength will come from giving it AWS access to Route53. I also think this is the main driver for putting it in, so it makes little sense that it is not there. |
|
cert-manager can run without issuer for now. I can ofcorse deploy the clusterissuer in namespace cert-manager if that what you are talking but I wanted to postpone it until I know how to handle certificate requests in namespaces/or on loadbalancer level |
This is a decent argument. But I guess you could also just test this in your sandbox cluster now right. |
Describe your changes
This pull request introduces a new Terraform module for managing FluxCD manifests for cert-manager, integrating it into the Kubernetes services deployment workflow. The changes add module configuration, resource definitions, templated manifest files, and necessary variables to support automated deployment and management of cert-manager via GitOps.
Key changes:
Cert-Manager FluxCD Module Introduction
_sub/security/cert-manager) that generates and manages FluxCD manifests for cert-manager, including CRD and Helm release resources, and integrates with GitHub repositories for GitOps workflows. [1] [2] [3] [4] [5] [6] [7]Module Integration and Configuration
cert_manager_flux_manifestsmodule into the main Kubernetes services Terraform configuration (compute/k8s-services/main.tf), with parameters for cluster, repository, and deployment settings.cert_manager_is_debug_modeto control debug logging for cert-manager deployments.Issue ticket number and link
Checklist before requesting a review
test/integrationfolder to apply my changes in QA. Read the guide on adding environment variables in QAIs it a new release?
release:(major|minor|patch), following semantic versioning in this guide ornoreleaseif there is no changes to the Terraform code