Skip to content

Remove deprecated flags#486

Open
clyang82 wants to merge 1 commit intoopenshift-online:mainfrom
clyang82:deprecated_flags
Open

Remove deprecated flags#486
clyang82 wants to merge 1 commit intoopenshift-online:mainfrom
clyang82:deprecated_flags

Conversation

@clyang82
Copy link
Contributor

Remove the following flags and related code:

  • enable-jwt
  • enable-authz
  • enable-ocm-mock
  • enable-sentry

@coderabbitai
Copy link

coderabbitai bot commented Feb 11, 2026

Walkthrough

Removed Sentry error reporting, JWT-based authentication & authorization middleware, and the OCM client and related config/flags; corresponding imports, mocks, middleware applications, and test JWT utilities were deleted across server, config, client, middleware, and tests.

Changes

Cohort / File(s) Summary
Auth subsystem removed
pkg/auth/auth_middleware.go, pkg/auth/auth_middleware_mock.go, pkg/auth/authz_middleware.go, pkg/auth/authz_middleware_mock.go, pkg/auth/context.go, pkg/auth/helpers.go
Deleted JWT auth/authorization middleware, mocks, JWT context/claim parsing, helper utilities, interfaces, and related public types/functions.
OCM client removed
pkg/client/ocm/client.go, pkg/client/ocm/authorization.go, pkg/client/ocm/authorization_mock.go
Removed OCM Client, Config, OCMAuthorization interface and mock, and all access-review/self-access logic and initialization.
Sentry & logging removals
pkg/config/sentry.go, cmd/maestro/environments/framework.go, pkg/api/error.go, pkg/api/metadata.go, pkg/db/transaction_middleware.go, pkg/logger/operationid_middleware.go, cmd/maestro/server/server.go
Deleted Sentry config, initialization, CaptureException calls, and per-request Sentry scope tagging; removed related imports and teardown calls.
Configuration & flags
pkg/config/config.go, pkg/config/http_server.go, pkg/config/ocm.go, cmd/maestro/environments/e_development.go, cmd/maestro/environments/e_integration_testing.go, cmd/maestro/environments/e_production.go, cmd/maestro/environments/types.go
Removed OCM and Sentry config structs and fields; removed JWT/authz HTTP server config fields and their flags; removed feature flags for authz, ocm-mock, ocm-debug, and enable-sentry.
Server routing & middleware application
cmd/maestro/server/api_server.go, cmd/maestro/server/routes.go
Removed Sentry and JWT middleware setup and application; removed auth/authz middleware from routes; simplified CORS AllowedOrigins configuration.
OpenAPI client change
pkg/api/openapi/client.go
Always initializes body buffer unconditionally in setBody (removed nil-check branch).
Tests: JWT/JWK cleanup
test/helper.go, test/integration/* (consumers_test.go, controller_test.go, db_listener_test.go, pagelist_test.go, resource_test.go)
Removed JWT/JWK test infra, key generation, JWK mock server, and JWT helpers; replaced authenticated test contexts with plain context.Background() across integration tests.
Dependencies & docs
go.mod, CLAUDE.md
Removed Sentry, jwt/v4 and other direct dependencies; added jwt/v5 indirectly; removed auth middleware mention from CLAUDE.md.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client as Client
participant Server as API Server
participant Auth as Auth Middleware
participant Authz as AuthZ Service/OCM
participant Handler as Handler
participant DB as Database

Note over Client,DB: Old flow (before removals)
Client->>Server: HTTP request
Server->>Auth: pass to JWT auth
Auth-->>Server: authenticated request (username in context)
Server->>Authz: authorization check (OCM access review)
Authz-->>Server: allowed/denied
alt allowed
    Server->>Handler: dispatch to handler
    Handler->>DB: DB operations
    DB-->>Handler: result
    Handler-->>Client: response
else denied
    Server-->>Client: 403/401

Note over Client,DB: New flow (after removals)
Client->>Server: HTTP request
Server->>Handler: direct dispatch (no auth/authz)
Handler->>DB: DB operations
DB-->>Handler: result
Handler-->>Client: response

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Remove deprecated flags' directly and accurately summarizes the main objective of this pull request, which is to remove the enable-jwt, enable-authz, enable-ocm-mock, and enable-sentry flags along with their related code.
Description check ✅ Passed The description is directly related to the changeset, listing the specific flags being removed (enable-jwt, enable-authz, enable-ocm-mock, enable-sentry) that align with the comprehensive changes across config, auth, OCM client, and test files.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
cmd/maestro/environments/framework_test.go (1)

28-31: Guard klog flag initialization to prevent re-initialization on repeated test runs.

Calling klog.InitFlags(nil) and pflag.CommandLine.AddGoFlagSet(...) directly in a test will fail if the test runs more than once (e.g., in parameterized or repeated test scenarios), causing "flag already registered" panics. Wrap with sync.Once to safely handle multiple invocations.

🔧 Suggested fix (sync.Once)
 import (
 	"flag"
 	"os/exec"
 	"reflect"
+	"sync"
 	"testing"

 	"github.com/spf13/pflag"
 	"k8s.io/klog/v2"
 )
+
+var initKlogFlagsOnce sync.Once
+
+func initTestFlags() {
+	initKlogFlagsOnce.Do(func() {
+		klog.InitFlags(nil)
+		pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
+	})
+}

 func TestLoadServices(t *testing.T) {
-	// Initialize klog flags first (same as main.go does)
-	klog.InitFlags(nil)
-	pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
+	initTestFlags()

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cmd/maestro/server/api_server.go (1)

41-54: ⚠️ Potential issue | 🔴 Critical

Restore the CORS allowed origins configuration.

The AllowedOrigins was changed to an empty slice []string{}, which blocks all cross-origin requests. The previous configuration allowed specific origins including https://console.redhat.com, https://cloud.redhat.com, https://qa.console.redhat.com, https://api.openshift.com, and others. An empty slice will break CORS for all UI applications that depend on this API.

Restore the list of allowed origins or remove the AllowedOrigins option entirely if all origins should be permitted.

🧹 Nitpick comments (1)
test/integration/consumers_test.go (1)

53-79: Remove JWT-derived Authorization header now that contexts are unauthenticated.

Line 74 and Line 133 pull openapi.ContextAccessToken from context.Background(), which is nil; fmt.Sprintf("Bearer %s", nil) produces a bogus header value and makes the invalid-JSON check depend on a meaningless token. Drop the header (or inject a fixed dummy token) so the test focuses solely on request validation.

Proposed cleanup
-	jwtToken := ctx.Value(openapi.ContextAccessToken)
 	restyResp, err := resty.R().
 		SetHeader("Content-Type", "application/json").
-		SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)).
 		SetBody(`{ this is invalid }`).
 		Post(h.RestURL("/consumers"))
-	jwtToken := ctx.Value(openapi.ContextAccessToken)
 	restyResp, _ := resty.R().
 		SetHeader("Content-Type", "application/json").
-		SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)).
 		SetBody(`{ this is invalid }`).
 		Patch(h.RestURL("/consumers/foo"))

Also applies to: 102-138

Signed-off-by: clyang82 <chuyang@redhat.com>
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.

1 participant