Skip to content

Fix jwt unit tests and add metrics integration test#698

Closed
Tharsanan1 wants to merge 6 commits into
wso2:mainfrom
Tharsanan1:main
Closed

Fix jwt unit tests and add metrics integration test#698
Tharsanan1 wants to merge 6 commits into
wso2:mainfrom
Tharsanan1:main

Conversation

@Tharsanan1

@Tharsanan1 Tharsanan1 commented Jan 16, 2026

Copy link
Copy Markdown
Contributor

Purpose

Explain why this feature or fix is required. Describe the underlying problems, issues, or needs driving this feature/fix and include links to related issues in the following format: Resolves issue1, issue2, etc.

Goals

Describe what solutions this feature or fix introduces to address the problems outlined above.

Approach

Describe how you are implementing the solutions. Include an animated GIF or screenshot if the change affects the UI. Include a link to a Markdown file or Google doc if the feature write-up is too long to paste here.

User stories

Summary of user stories addressed by this change>

Documentation

Link(s) to product documentation that addresses the changes of this PR. If no doc impact, enter “N/A” plus brief explanation of why there’s no doc impact

Automation tests

  • Unit tests

    Code coverage information

  • Integration tests

    Details about the test cases and coverage

Security checks

Samples

Provide high-level details about the samples related to this feature

Related PRs

List any other related PRs

Test environment

List all JDK versions, operating systems, databases, and browser/versions on which this feature/fix was tested

Summary by CodeRabbit

  • New Features

    • Added Prometheus metrics exposure for gateway controller and policy engine with dedicated monitoring endpoints (ports 9091 and 9003) enabling enhanced observability.
  • Bug Fixes

    • Improved JWT authentication to support wildcard key managers when issuer-specific configurations are unavailable.
  • Documentation

    • Added setup guidelines for development automation.
  • Chores

    • Updated Go version to 1.25.1.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Jan 16, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The PR adds comprehensive metrics exposure and testing for gateway components. New configuration files establish Copilot guidelines and a setup workflow. Integration tests verify Prometheus metrics endpoints on gateway-controller (9091) and policy-engine (9003), including format validation and specific metric assertions. JWT auth token issuer validation is enhanced to treat empty-issuer key managers as wildcards. Go version is bumped to 1.25.1.

Changes

Cohort / File(s) Summary
Copilot Configuration
.github/copilot-instructions.md, .github/workflows/copilot-setup-steps.yml
New Markdown guidelines for Copilot agent covering project structure, test workflows, and build commands. New GitHub Actions workflow sets up Go 1.25, Docker Buildx, Helm, kubectl, and builds gateway images.
Metrics Integration Tests
gateway/it/docker-compose.test.yaml, gateway/it/features/metrics.feature, gateway/it/steps_metrics.go, gateway/it/suite_test.go, gateway/it/test-config.yaml
Added port mappings (9091, 9003) for metrics exposure. New BDD feature file with four scenarios validating Prometheus metrics endpoints, response format, and specific metrics (gateway_controller_api_operations_total, gateway_controller_apis_total, policy_engine_requests_total). New Go step definitions for metrics endpoint requests and validation. Test configuration updated with metrics settings. Suite initialized with metrics steps and feature.
JWT Auth Policy Update
gateway/policies/jwt-auth/v0.1.0/go.mod, gateway/policies/jwt-auth/v0.1.0/jwtauth.go
Go version bumped to 1.25.1. Token issuer validation logic enhanced: empty-issuer key managers now treated as wildcards, allowing validation to proceed if wildcard managers exist; validation fails only when token has issuer but no applicable managers and no wildcards.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Metrics and tests now glow so bright,
Prometheus endpoints shining in the light!
Wildcards catch what issuers roam,
Gateway components now have a home. 🏠✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is entirely a template with no actual content filled in—all sections contain only placeholder text and lack specific implementation details. Fill in all required sections with actual content: explain the purpose and problems being addressed, describe the goals and solutions, detail the implementation approach, document test coverage, and confirm security checks.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the primary changes: JWT unit test fixes and new metrics integration tests are both present in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
.github/workflows/copilot-setup-steps.yml (1)

28-48: Pin tool versions for reproducible setup.

Lines 31, 43, and 48 use floating versions (1.25, latest), which can introduce breaking changes. Consider pinning explicit versions (or repository variables) for Go patch, Helm, and kubectl to keep CI deterministic.

gateway/it/steps_metrics.go (1)

101-111: Consider more precise metric name matching.

The current substring check could match partial metric names (e.g., searching for requests_total would match policy_engine_requests_total but also other_requests_total_count). For more robust validation, consider matching the metric name at line boundaries.

♻️ Optional: More precise metric name matching
 func (m *MetricsSteps) theResponseShouldContainMetric(metricName string) error {
 	body := m.httpSteps.LastBody()
 	bodyStr := string(body)
 
-	if !strings.Contains(bodyStr, metricName) {
-		return fmt.Errorf("response does not contain metric '%s'", metricName)
+	// Check for metric name at the start of a line (handles both HELP/TYPE comments and metric lines)
+	lines := strings.Split(bodyStr, "\n")
+	for _, line := range lines {
+		if strings.HasPrefix(line, metricName) ||
+			strings.Contains(line, "# HELP "+metricName) ||
+			strings.Contains(line, "# TYPE "+metricName) {
+			return nil
+		}
 	}
-
-	return nil
+	return fmt.Errorf("response does not contain metric '%s'", metricName)
 }

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 68f4195 and 992c4ff.

⛔ Files ignored due to path filters (1)
  • gateway/policies/jwt-auth/v0.1.0/go.sum is excluded by !**/*.sum
📒 Files selected for processing (9)
  • .github/copilot-instructions.md
  • .github/workflows/copilot-setup-steps.yml
  • gateway/it/docker-compose.test.yaml
  • gateway/it/features/metrics.feature
  • gateway/it/steps_metrics.go
  • gateway/it/suite_test.go
  • gateway/it/test-config.yaml
  • gateway/policies/jwt-auth/v0.1.0/go.mod
  • gateway/policies/jwt-auth/v0.1.0/jwtauth.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T06:57:38.504Z
Learnt from: tharindu1st
Repo: wso2/api-platform PR: 514
File: gateway/gateway-controller/config/config.yaml:0-0
Timestamp: 2025-12-19T06:57:38.504Z
Learning: In gateway/gateway-controller/config/config.yaml, the default admin user with plaintext password "admin" is intentionally provided for testing purposes only and must be changed to secure hashed passwords for production deployments.

Applied to files:

  • gateway/it/test-config.yaml
🧬 Code graph analysis (2)
gateway/it/steps_metrics.go (2)
gateway/it/state.go (1)
  • TestState (61-82)
gateway/it/steps/http_steps.go (1)
  • HTTPSteps (38-45)
gateway/it/suite_test.go (1)
gateway/it/steps_metrics.go (1)
  • RegisterMetricsSteps (44-50)
🔇 Additional comments (13)
gateway/policies/jwt-auth/v0.1.0/jwtauth.go (1)

715-730: Add/confirm test coverage for issuer-less key manager fallback.

Line 715 now accepts issuer-less key managers even in strict mode; please confirm a unit test covers this branch to lock in the intended behavior.

.github/copilot-instructions.md (1)

17-62: Clear, actionable Copilot testing guidance.

The rebuild/test/revert flow is explicit and should prevent CI drift. Nicely documented.

gateway/policies/jwt-auth/v0.1.0/go.mod (1)

3-3: Verify Go 1.25.1 availability across CI/dev toolchains.

Line 3 bumps the module to Go 1.25.1; please confirm CI runners and local dev setups support this version and remain aligned across other Go modules.

gateway/it/suite_test.go (1)

71-74: Metrics feature wired into the suite.

Adding the metrics feature path and step registration looks consistent and should exercise the new scenarios.

Also applies to: 221-223

gateway/it/docker-compose.test.yaml (1)

33-33: LGTM!

The metrics port mappings for gateway-controller (9091) and policy-engine (9003) are correctly configured and consistent with the test configuration and step definitions.

Also applies to: 63-63

gateway/it/test-config.yaml (2)

220-227: LGTM!

The gateway controller metrics configuration is well-documented and properly enables Prometheus metrics on port 9091, consistent with the docker-compose and test step configurations.


302-309: LGTM!

The policy engine metrics configuration mirrors the gateway controller structure with clear comments and port 9003 aligns with the exposed docker-compose port and test step constants.

gateway/it/features/metrics.feature (3)

24-35: LGTM!

The Background and basic accessibility scenarios follow Gherkin best practices with clear structure. The scenarios properly verify that both metrics endpoints return 200 and contain valid Prometheus-formatted data.


37-42: Consider adding a deployment wait step before querying metrics.

This scenario creates an API but immediately queries metrics without waiting for deployment. If the metrics gateway_controller_api_operations_total or gateway_controller_apis_total are only incremented after successful deployment, this test could be flaky.

Compare with the more robust pattern used in the "Policy engine metrics reflect request processing" scenario (lines 44-67), which includes an explicit wait step.


44-67: LGTM!

This scenario is well-structured with proper setup including API configuration, deployment wait, and traffic generation before asserting on policy engine metrics. Good use of inline JSON for API configuration.

gateway/it/steps_metrics.go (3)

29-35: LGTM!

Constants for metrics ports are well-documented and consistent with the docker-compose and test configuration files.


37-50: LGTM!

Good design reusing the existing HTTPSteps for HTTP operations rather than duplicating HTTP client logic. The step registration follows the established Godog patterns in the project.


64-99: LGTM!

The Prometheus format validation is thorough - it checks for standard headers (# HELP, # TYPE) and ensures actual metric data exists beyond just comments. The logic correctly handles edge cases like empty lines and comment-only responses.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Tharsanan1 Tharsanan1 closed this Jan 16, 2026
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Tharsanan1
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

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