Add the new mcoa-obs-api as hosts in the observability-server-certs #2407
Add the new mcoa-obs-api as hosts in the observability-server-certs #2407coleenquadros wants to merge 9 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coleenquadros The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExports Obs API gateway constants, adds a second MCOA gateway, updates GetObsAPIExternalURL signature and its call sites, propagates gateway selection through controllers and certificate host generation, extends RBAC for Thanos resources, updates tests, tweaks renderer whitespace, and bumps Go version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Controller as Controller
participant Config as Config (GetObsAPIExternalURL)
participant RouteAPI as Cluster Route/Ingress
participant Certs as Certificates
participant Renderer as Renderer
Controller->>Config: GetObsAPIExternalURL(obsapi, namespace)
Config->>RouteAPI: GetRouteHost(obsapi, namespace)
RouteAPI-->>Config: return external URL
Config-->>Controller: return parsed URL (host)
Controller->>Certs: request SANs (include ObsAPI host, MCOA service)
Certs-->>Controller: return SAN list
Controller->>Renderer: pass obsAPIHost and SANs to renderer options
Renderer-->>Controller: rendered manifests/deployment configs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
proxy/pkg/proxy/proxy_test.go (1)
350-356: Disambiguate this subtest name for cleaner failure triage.Line 350 uses the same name as the existing case at Line 358, so test output gets less searchable (Go adds numeric suffixes behind the scenes). Consider renaming this one to explicitly include the
{}matcher variant. Small change, big debugging ergonomics. 🚨🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/pkg/proxy/proxy_test.go` around lines 350 - 356, Rename the ambiguous subtest name string in the test table entry that currently reads "should handle GET to label values endpoint with correct metric" to a unique, explicit name that indicates the "{} matcher variant (e.g., include "{}` matcher" or "with {} matcher") so it no longer collides with the other case; update the test case with that new name where the table item uses method "GET" and path containing config.RBACProxyLabelMetricName + "{}" to make failures easy to triage (leave other fields like path, expectedBody and expectedToHandle unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yaml`:
- Around line 95-97: The RBAC rule granting verbs
["get","list","watch","create","update","delete","patch"] on apiGroup
"monitoring.thanos.io" for resources
["thanosreceives","thanosqueries","thanoscompacts","thanosstores","thanosrulers"]
is overly permissive; either remove this rule entirely or change the verbs to
read-only ["get","list","watch"] (if only querying), and if mutation verbs are
truly required add an inline comment explaining why full
create/update/delete/patch access to those Thanos CRDs is necessary; update the
ClusterRole entry that declares apiGroups: ["monitoring.thanos.io"] accordingly.
---
Nitpick comments:
In `@proxy/pkg/proxy/proxy_test.go`:
- Around line 350-356: Rename the ambiguous subtest name string in the test
table entry that currently reads "should handle GET to label values endpoint
with correct metric" to a unique, explicit name that indicates the "{} matcher
variant (e.g., include "{}` matcher" or "with {} matcher") so it no longer
collides with the other case; update the test case with that new name where the
table item uses method "GET" and path containing config.RBACProxyLabelMetricName
+ "{}" to make failures easy to triage (leave other fields like path,
expectedBody and expectedToHandle unchanged).
🪄 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: Pro
Run ID: 1cf40cdb-886b-4e02-9b1d-61fb8468aae4
📒 Files selected for processing (3)
operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yamlproxy/pkg/proxy/proxy.goproxy/pkg/proxy/proxy_test.go
coleenquadros
force-pushed
the
add_thanos_op
branch
from
b2e66e5 to
83b9c3b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operators/multiclusterobservability/pkg/config/config_test.go (1)
315-367:⚠️ Potential issue | 🟡 MinorFix undefined constant reference at line 264 in TestGetObsAPIRouteHost. 🚨
The changed test segments (lines 315-367) look solid—routes are created with
ObsAPIGateway, calls pass the exported constant, and you've got good coverage on the happy path, custom URL override, and error handling for malformed URLs. That's the foundation we want.However, the verification uncovered a landmine: Line 264 references
obsAPIGateway(unexported), but this constant has no definition anywhere in the config package. That test will fail at runtime or compilation. Update line 264 to useObsAPIGateway(the exported constant) to match the rest of the codebase—consistency is how we avoid these traps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/multiclusterobservability/pkg/config/config_test.go` around lines 315 - 367, The test references an undefined, unexported constant obsAPIGateway; replace that usage with the exported ObsAPIGateway constant so TestGetObsAPIRouteHost (and related tests like TestGetObsAPIExternalHost / TestGetObsAPIRouteHost) consistently use ObsAPIGateway when creating routes and calling GetObsAPIExternalURL/GetObsAPIRouteHost; ensure all occurrences of obsAPIGateway in the test file are updated to ObsAPIGateway so the code compiles and uses the defined exported symbol.
🧹 Nitpick comments (1)
operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go (1)
256-259: Useconfig.ObsAPIGatewayfor consistency at line 256. 🔧The local
obsAPIGatewayconstant inobservatorium.gohas the same value as the exportedconfig.ObsAPIGateway. At line 256, passingconfig.ObsAPIGatewaywould align with the function's expected input and make the intent clearer.Note: The local
obsAPIGatewayconstant still needs to exist inobservatorium.gosince it's used 5+ times for Kubernetes resource naming (concatenated with prefixes for Route and Service names).♻️ Suggested fix for line 256
- obsAPIURL, err := config.GetObsAPIExternalURL(ctx, r.Client, obsAPIGateway, config.GetDefaultNamespace()) + obsAPIURL, err := config.GetObsAPIExternalURL(ctx, r.Client, config.ObsAPIGateway, config.GetDefaultNamespace())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go` around lines 256 - 259, At the call to GetObsAPIExternalURL in multiclusterobservability_controller.go replace the local obsAPIGateway argument with the exported config.ObsAPIGateway to be consistent with the function's expected input and project conventions; keep the local obsAPIGateway constant in observatorium.go for resource naming where it’s used elsewhere, and ensure the call uses config.ObsAPIGateway when invoking GetObsAPIExternalURL (function name: GetObsAPIExternalURL; identifier to change: obsAPIGateway -> config.ObsAPIGateway).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@operators/multiclusterobservability/pkg/config/config_test.go`:
- Around line 315-367: The test references an undefined, unexported constant
obsAPIGateway; replace that usage with the exported ObsAPIGateway constant so
TestGetObsAPIRouteHost (and related tests like TestGetObsAPIExternalHost /
TestGetObsAPIRouteHost) consistently use ObsAPIGateway when creating routes and
calling GetObsAPIExternalURL/GetObsAPIRouteHost; ensure all occurrences of
obsAPIGateway in the test file are updated to ObsAPIGateway so the code compiles
and uses the defined exported symbol.
---
Nitpick comments:
In
`@operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go`:
- Around line 256-259: At the call to GetObsAPIExternalURL in
multiclusterobservability_controller.go replace the local obsAPIGateway argument
with the exported config.ObsAPIGateway to be consistent with the function's
expected input and project conventions; keep the local obsAPIGateway constant in
observatorium.go for resource naming where it’s used elsewhere, and ensure the
call uses config.ObsAPIGateway when invoking GetObsAPIExternalURL (function
name: GetObsAPIExternalURL; identifier to change: obsAPIGateway ->
config.ObsAPIGateway).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fa3f800-086b-423d-a5a8-2eec226ca758
📒 Files selected for processing (7)
operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.gooperators/multiclusterobservability/controllers/placementrule/hub_info_secret.gooperators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yamloperators/multiclusterobservability/pkg/certificates/certificates.gooperators/multiclusterobservability/pkg/config/config.gooperators/multiclusterobservability/pkg/config/config_test.gooperators/multiclusterobservability/pkg/rendering/renderer_mcoa.go
✅ Files skipped from review due to trivial changes (1)
- operators/multiclusterobservability/pkg/rendering/renderer_mcoa.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yaml (1)
99-101:⚠️ Potential issue | 🟠 MajorThanos CRD permissions are still broader than least privilege requires.
Line 99-101 still grants mutate verbs (
create/update/delete/patch) across fivemonitoring.thanos.ioresources without evidence in the provided context of direct write paths. This is the same security concern raised earlier and appears unresolved.🔒 Tighten to read-only unless write paths are explicitly required
- apiGroups: ["monitoring.thanos.io"] resources: ["thanosreceives", "thanosqueries", "thanoscompacts", "thanosstores", "thanosrulers"] - verbs: ["get", "list", "watch", "create", "update", "delete", "patch"] + verbs: ["get", "list", "watch"]As per coding guidelines,
**/manifests/**/*.yaml: “Check for security best practices (RBAC, SecurityContext, NetworkPolicies)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yaml` around lines 99 - 101, The ClusterRole rule granting apiGroups: monitoring.thanos.io for resources thanosreceives, thanosqueries, thanoscompacts, thanosstores, thanosrulers currently includes mutate verbs (create, update, delete, patch); change those verbs to read-only verbs ("get", "list", "watch") to follow least-privilege. If any controller truly requires write access, scope it to the minimal resource(s) and verbs needed or create a separate ClusterRole/Role for that write path and document the justification; update the rule that references apiGroups: monitoring.thanos.io and resources: thanosreceives, thanosqueries, thanoscompacts, thanosstores, thanosrulers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yaml`:
- Around line 95-97: The ClusterRole for
multicluster-observability-addon-manager is under-scoped for
route.openshift.io/routes; update the ClusterRole (resource block currently
listing apiGroups ["route.openshift.io"] resources ["routes"]) to include create
and update permissions (and consider patch) so the addon manager can perform
GenerateAPIGatewayRoute operations called from observatorium.go
(GenerateAPIGatewayRoute at lines ~289 and ~324) without hitting forbidden
errors; ensure the verbs include at least "create" and "update" (plus "patch" if
you use partial updates) in addition to the existing read verbs.
---
Duplicate comments:
In
`@operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yaml`:
- Around line 99-101: The ClusterRole rule granting apiGroups:
monitoring.thanos.io for resources thanosreceives, thanosqueries,
thanoscompacts, thanosstores, thanosrulers currently includes mutate verbs
(create, update, delete, patch); change those verbs to read-only verbs ("get",
"list", "watch") to follow least-privilege. If any controller truly requires
write access, scope it to the minimal resource(s) and verbs needed or create a
separate ClusterRole/Role for that write path and document the justification;
update the rule that references apiGroups: monitoring.thanos.io and resources:
thanosreceives, thanosqueries, thanoscompacts, thanosstores, thanosrulers
accordingly.
🪄 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: Pro Plus
Run ID: 66d66897-9007-4bb4-a564-d7a5376d3ee7
📒 Files selected for processing (1)
operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yaml
coleenquadros
changed the title
Signed-off-by: Coleen Iona Quadros <[email protected]>
Signed-off-by: Coleen Iona Quadros <[email protected]>
Signed-off-by: Coleen Iona Quadros <[email protected]>
Signed-off-by: Coleen Iona Quadros <[email protected]>
coleenquadros
force-pushed
the
add_thanos_op
branch
from
edea76c to
a4c0160
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yaml (1)
99-101:⚠️ Potential issue | 🟠 MajorTrim Thanos RBAC verbs or document why full mutation is required.
Lines [99]-[101] grant full write access (
create/update/delete/patch) on multiple Thanos CRDs. That’s a bigger wrench than this change appears to need. Please reduce to least privilege (get/list/watch) unless mutation is required, and if required, add an inline rationale in this rule.🔧 Suggested least-privilege baseline
- apiGroups: ["monitoring.thanos.io"] resources: ["thanosreceives", "thanosqueries", "thanoscompacts", "thanosstores", "thanosrulers"] - verbs: ["get", "list", "watch", "create", "update", "delete", "patch"] + verbs: ["get", "list", "watch"]As per coding guidelines
**/manifests/**/*.yaml: "Check for security best practices (RBAC, SecurityContext, NetworkPolicies)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yaml` around lines 99 - 101, The ClusterRole rule granting verbs ["create","update","delete","patch"] for apiGroups "monitoring.thanos.io" on resources ["thanosreceives","thanosqueries","thanoscompacts","thanosstores","thanosrulers"] is overly permissive; change the verbs to the least-privilege set ["get","list","watch"] unless your controller truly needs to mutate these Thanos CRDs, and if mutation is required keep only the specific verbs needed and add an inline YAML comment next to this rule explaining the exact justification (who/what requires mutation and why) so reviewers can validate the exception.
🧹 Nitpick comments (1)
operators/multiclusterobservability/pkg/config/config.go (1)
402-420: Add input validation to fail fast on emptyobsapiparameter.Since
GetObsAPIExternalURLnow accepts a free-form gateway name instead of using a hardcoded constant, add a guard to catch empty strings early. Without it, an emptyobsapiwould result inGetRouteHostconstructing an invalid hostname like-namespace.domainin its fallback path.🛠️ Suggested guard
func GetObsAPIExternalURL(ctx context.Context, client client.Client, obsapi string, namespace string) (*url.URL, error) { + if strings.TrimSpace(obsapi) == "" { + return nil, fmt.Errorf("obsapi route name cannot be empty") + } + mco := &observabilityv1beta2.MultiClusterObservability{} err := client.Get(ctx, types.NamespacedName{ Name: GetMonitoringCRName(),All current call sites use constants, so this is defensive housekeeping against future misuse. 🔍
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/multiclusterobservability/pkg/config/config.go` around lines 402 - 420, GetObsAPIExternalURL lacks input validation for the obsapi parameter, so an empty obsapi can produce invalid hostnames via GetRouteHost; add a guard at the start of GetObsAPIExternalURL (function GetObsAPIExternalURL) to return a clear error when obsapi == "" (or strings.TrimSpace(obsapi) == "") before calling GetRouteHost, returning a descriptive error value so callers fail fast and avoid constructing invalid hostnames.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@operators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yaml`:
- Around line 99-101: The ClusterRole rule granting verbs
["create","update","delete","patch"] for apiGroups "monitoring.thanos.io" on
resources
["thanosreceives","thanosqueries","thanoscompacts","thanosstores","thanosrulers"]
is overly permissive; change the verbs to the least-privilege set
["get","list","watch"] unless your controller truly needs to mutate these Thanos
CRDs, and if mutation is required keep only the specific verbs needed and add an
inline YAML comment next to this rule explaining the exact justification
(who/what requires mutation and why) so reviewers can validate the exception.
---
Nitpick comments:
In `@operators/multiclusterobservability/pkg/config/config.go`:
- Around line 402-420: GetObsAPIExternalURL lacks input validation for the
obsapi parameter, so an empty obsapi can produce invalid hostnames via
GetRouteHost; add a guard at the start of GetObsAPIExternalURL (function
GetObsAPIExternalURL) to return a clear error when obsapi == "" (or
strings.TrimSpace(obsapi) == "") before calling GetRouteHost, returning a
descriptive error value so callers fail fast and avoid constructing invalid
hostnames.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 14f11b2b-f02d-4a10-b125-cd6b460a67d0
📒 Files selected for processing (8)
go.modoperators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.gooperators/multiclusterobservability/controllers/placementrule/hub_info_secret.gooperators/multiclusterobservability/manifests/base/multicluster-observability-addon/cluster_role.yamloperators/multiclusterobservability/pkg/certificates/certificates.gooperators/multiclusterobservability/pkg/config/config.gooperators/multiclusterobservability/pkg/config/config_test.gooperators/multiclusterobservability/pkg/rendering/renderer_mcoa.go
✅ Files skipped from review due to trivial changes (1)
- operators/multiclusterobservability/pkg/rendering/renderer_mcoa.go
🚧 Files skipped from review as they are similar to previous changes (5)
- operators/multiclusterobservability/controllers/placementrule/hub_info_secret.go
- operators/multiclusterobservability/pkg/config/config_test.go
- operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go
- go.mod
- operators/multiclusterobservability/pkg/certificates/certificates.go
|
/test pr-image-mirror-rbac-query-proxy |
Signed-off-by: Coleen Iona Quadros <[email protected]>
|
@coleenquadros: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
1 participant
Summary by CodeRabbit
New Features
Improvements