ROSAENG-59880: Add Grafana ingress with GitHub OAuth#616
Open
iamkirkbater wants to merge 2 commits into
Open
Conversation
Expose Grafana behind an internet-facing ALB with GitHub OAuth via oauth2-proxy, replacing the finicky port-forward workflow. Gated behind the `enable_grafana_ingress` feature flag (enabled for integration). Terraform module (grafana-ingress): ALB with ACM TLS termination, Route53 DNS record, security groups. oauth2-proxy runs plain HTTP behind the ALB; a TargetGroupBinding registers pod IPs with the target group. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
Collaborator
|
@iamkirkbater: This pull request references ROSAENG-59880 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds an optional internet-facing Grafana OAuth2 proxy ingress. A new ChangesGrafana OAuth2 Proxy Ingress Feature
Sequence Diagram(s)sequenceDiagram
participant Terraform as Terraform<br/>(regional-cluster)
participant BootstrapScript as bootstrap-argocd.sh
participant ECSTask as ECS Bootstrap Task
participant ClusterSecret as local-cluster-identity<br/>Secret
participant ArgoCD as ArgoCD ApplicationSet
participant GrafanaChart as Grafana Helm Chart
participant ExternalSecrets as External Secrets<br/>Operator
participant SecretsManager as AWS Secrets Manager
participant ALB as Internet-Facing ALB
rect rgba(99, 179, 237, 0.5)
note over Terraform: Infrastructure provisioning
Terraform->>ALB: create ALB, target group (port 4180), ACM cert, Route53 record
Terraform-->>BootstrapScript: output grafana_target_group_arn
end
rect rgba(154, 230, 180, 0.5)
note over BootstrapScript: Bootstrap ARN propagation
BootstrapScript->>ECSTask: pass GRAFANA_TARGET_GROUP_ARN env var
ECSTask->>ClusterSecret: kubectl annotate grafana_target_group_arn
end
rect rgba(246, 173, 85, 0.5)
note over ClusterSecret: Helm chart rendering
ArgoCD->>ClusterSecret: read grafana_target_group_arn annotation
ArgoCD->>GrafanaChart: deploy with oauth2-proxy.enabled, grafana.targetGroup.arn
GrafanaChart->>ExternalSecrets: create ExternalSecret grafana-oauth-proxy
ExternalSecrets->>SecretsManager: fetch client-id, client-secret, cookie-secret
SecretsManager-->>ExternalSecrets: secret values
GrafanaChart->>ALB: create TargetGroupBinding grafana-oauth2-proxy (port 4180)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@argocd/config/regional-cluster/grafana/values.yaml`:
- Around line 210-212: The YAML file has a duplicate root-level `grafana` key
that overrides the entire earlier Grafana configuration block. Instead of
defining `grafana` again with only `targetGroup.arn`, nest the `targetGroup` and
`arn` properties under the existing `grafana` key that appears earlier in the
file. Remove the duplicate `grafana` key definition and add `targetGroup.arn` as
a nested property within the original `grafana` configuration block to ensure
all Grafana settings are preserved and merged correctly.
In `@config/defaults.yaml`:
- Line 98: The docstring comment on line 98 of config/defaults.yaml incorrectly
refers to an "internet-facing NLB" when the configuration actually provisions an
"ALB" (Application Load Balancer). Update the text in the `@doc` annotation to
replace "NLB" with "ALB" to provide accurate documentation to operators who will
be setting up and debugging this Grafana ingress configuration.
In `@deploy/ephemeral/us-east-1/_merged_config.yaml`:
- Around line 100-103: The documentation comment for the grafana_ingress.enabled
configuration incorrectly specifies "NLB" (Network Load Balancer) when the
actual implementation provisions an "ALB" (Application Load Balancer). Locate
the source template file containing this doc comment (the _merged_config.yaml
file is a generated artifact), update the comment to replace "NLB" with "ALB"
for accuracy, and then regenerate the merged configuration artifact. Apply the
same correction to the other affected location mentioned in the review (around
lines 149-152) in the source template before regenerating.
In `@terraform/modules/grafana-ingress/main.tf`:
- Around line 17-27: The aws_lb resource named "grafana" is missing the security
configuration to drop invalid HTTP headers. Add the
enable_drop_invalid_header_fields attribute set to true within the aws_lb
"grafana" resource block to explicitly enable dropping of malformed and invalid
HTTP headers as a security best practice.
In `@terraform/modules/grafana-ingress/variables.tf`:
- Around line 35-37: The current regex pattern in the validation condition for
var.domain_name accepts single-label domain names (like `grafana`) which fail
later during ACM certificate provisioning. Update the regex pattern to require
at least one dot, ensuring the domain name is in FQDN (Fully Qualified Domain
Name) format with at least a domain and top-level domain (e.g.,
`grafana.example.com`). Modify the condition in the validation block to enforce
this requirement while keeping the character restrictions.
- Around line 1-4: The regional_id variable lacks length validation, which can
cause ALB and target-group resource creation failures when the variable is too
long. Add a validation block to the regional_id variable in the variables
definition that enforces a reasonable maximum length constraint. This will
ensure that when regional_id is used in constructing resource names like
${var.regional_id}-grafana, the resulting names stay within AWS ALB and
target-group name length limits. Determine the appropriate max length by
accounting for the "-grafana" suffix and AWS naming constraints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 41fbecd0-3af8-4f0b-9b08-44b60a732ceb
📒 Files selected for processing (30)
argocd/config/regional-cluster/grafana/Chart.yamlargocd/config/regional-cluster/grafana/templates/oauth-external-secret.yamlargocd/config/regional-cluster/grafana/templates/targetgroupbinding.yamlargocd/config/regional-cluster/grafana/values.yamlconfig/defaults.yamlconfig/integration/defaults.yamlconfig/templates/argocd-bootstrap/applicationset.yaml.j2config/templates/pipeline-regional-cluster-inputs/terraform.json.j2deploy/ephemeral/us-east-1/_merged_config.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/ephemeral/us-east-1/argocd-values-regional-cluster.yamldeploy/ephemeral/us-east-1/pipeline-regional-cluster-inputs/terraform.jsondeploy/integration/us-east-1/_merged_config.yamldeploy/integration/us-east-1/argocd-bootstrap-management-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-bootstrap-regional-cluster/applicationset.yamldeploy/integration/us-east-1/argocd-values-regional-cluster.yamldeploy/integration/us-east-1/pipeline-regional-cluster-inputs/terraform.jsonscripts/bootstrap-argocd.shterraform/config/regional-cluster/main.tfterraform/config/regional-cluster/outputs.tfterraform/config/regional-cluster/variables.tfterraform/modules/ecs-bootstrap/main.tfterraform/modules/grafana-ingress/acm.tfterraform/modules/grafana-ingress/dns.tfterraform/modules/grafana-ingress/main.tfterraform/modules/grafana-ingress/outputs.tfterraform/modules/grafana-ingress/security-groups.tfterraform/modules/grafana-ingress/variables.tfterraform/modules/grafana-ingress/versions.tf
Comment on lines
+100
to
+103
| grafana_ingress: | ||
| # @doc observability.grafana_ingress.enabled Enable Grafana OAuth proxy (internet-facing NLB + GitHub OAuth). Requires Secrets Manager secrets for GitHub OAuth credentials and TLS cert. | ||
| # @used-by observability.grafana_ingress.enabled _context | ||
| enabled: false |
Contributor
There was a problem hiding this comment.
Correct load balancer type in the doc comment.
Line 101 says “NLB”, but this rollout provisions an ALB. Please update the source template comment and regenerate this artifact to avoid operational confusion.
Also applies to: 149-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/ephemeral/us-east-1/_merged_config.yaml` around lines 100 - 103, The
documentation comment for the grafana_ingress.enabled configuration incorrectly
specifies "NLB" (Network Load Balancer) when the actual implementation
provisions an "ALB" (Application Load Balancer). Locate the source template file
containing this doc comment (the _merged_config.yaml file is a generated
artifact), update the comment to replace "NLB" with "ALB" for accuracy, and then
regenerate the merged configuration artifact. Apply the same correction to the
other affected location mentioned in the review (around lines 149-152) in the
source template before regenerating.
Comment on lines
+1
to
+4
| variable "regional_id" { | ||
| description = "Regional cluster identifier for resource naming" | ||
| type = string | ||
| } |
Contributor
There was a problem hiding this comment.
Add a max-length validation for regional_id to prevent ALB/TG name apply failures.
Line 18 and Line 41 in terraform/modules/grafana-ingress/main.tf build names as ${var.regional_id}-grafana. ALB and target-group names are length-limited, so an unbounded regional_id can fail at apply time.
Suggested fix
variable "regional_id" {
description = "Regional cluster identifier for resource naming"
type = string
+
+ validation {
+ condition = length(var.regional_id) <= 24
+ error_message = "regional_id must be <= 24 characters to satisfy ALB/target group naming limits."
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| variable "regional_id" { | |
| description = "Regional cluster identifier for resource naming" | |
| type = string | |
| } | |
| variable "regional_id" { | |
| description = "Regional cluster identifier for resource naming" | |
| type = string | |
| validation { | |
| condition = length(var.regional_id) <= 24 | |
| error_message = "regional_id must be <= 24 characters to satisfy ALB/target group naming limits." | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terraform/modules/grafana-ingress/variables.tf` around lines 1 - 4, The
regional_id variable lacks length validation, which can cause ALB and
target-group resource creation failures when the variable is too long. Add a
validation block to the regional_id variable in the variables definition that
enforces a reasonable maximum length constraint. This will ensure that when
regional_id is used in constructing resource names like
${var.regional_id}-grafana, the resulting names stay within AWS ALB and
target-group name length limits. Determine the appropriate max length by
accounting for the "-grafana" suffix and AWS naming constraints.
- Move targetGroup.arn under existing grafana subchart key to avoid duplicate root-level YAML key that would override subchart config - Fix NLB→ALB in @doc annotation and remove stale TLS cert mention - Add drop_invalid_header_fields on internet-facing ALB - Require FQDN (at least one dot) in domain_name validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
needs-rebase
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
grafana-ingress) creates an ALB with ACM TLS termination, Route53 DNS record (grafana.<deployment>.<domain>), and security groupsenable_grafana_ingressfeature flag — enabled for integration, disabled by defaultHow it works
Manual prerequisites (per environment, one-time)
https://grafana.<deployment>.<domain>/oauth2/callback<regional_id>-grafana-oauth-proxywith keys:client-id,client-secret,cookie-secretTest plan
make helm-lintpasses (verified locally)uv run scripts/render.py --checkpasses (verified locally)terraform validateonterraform/config/regional-cluster/JIRA: ROSAENG-59880
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
observability.grafana_ingress.enabled(default: disabled).