Skip to content

Conversation

@aaronschweig
Copy link
Contributor

@aaronschweig aaronschweig commented Oct 7, 2025

Summary by CodeRabbit

  • New Features
    • Added a cluster-scoped Invite resource (email, optional role). Operator now processes Invites and sends invitation emails via Keycloak.
  • RBAC
    • Added cluster roles for invites: admin, editor, and viewer.
  • Configuration
    • New Keycloak invite settings and operator subcommand binding; API export updated to include invites and one legacy APIExport/APIResourceSchema removed.
  • Samples
    • Added a sample Invite manifest.
  • Tests
    • Added unit tests covering Invite processing and error scenarios.
  • Chores
    • Updated CRDs, resource schemas, and dependency graph.

@coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025

Walkthrough

Adds a cluster-scoped Invite API (types, CRD, sample), deepcopy generation, reconciler wiring and lifecycle subroutine integrating with Keycloak (including tests), RBAC roles, app config/flag wiring, APIExport/APIResourceSchema updates, and Go dependency changes.

Changes

Cohort / File(s) Change summary
API types (Invite)
api/v1alpha1/invite_types.go, api/v1alpha1/zz_generated.deepcopy.go
Add cluster-scoped Invite CRD types (Invite, InviteSpec, InviteStatus, InviteList), condition accessors (GetConditions, SetConditions), register with scheme, and add autogenerated deepcopy helpers.
Controller wiring
cmd/operator.go, internal/controller/invite_controller.go
Add InviteReconciler, NewInviteReconciler, Reconcile, SetupWithManager, and wire the reconciler into operator startup.
Invite subroutine & tests
internal/subroutine/invite/subroutine.go, internal/subroutine/invite/subroutine_test.go
Implement lifecycle subroutine for Keycloak invite flow (OIDC password grant, user lookup/create, execute-actions-email), exported required-action constants, and unit tests with mocked Keycloak/K8s interactions.
App config and flags
internal/config/config.go, cmd/root.go
Add InviteConfig to application config and bind operator command flags during init.
CRD manifests & kustomize
config/crd/bases/core.platform-mesh.io_invites.yaml, config/crd/kustomization.yaml
Add Invite CRD manifest and include it in CRD kustomization resources.
RBAC roles (Invite)
config/rbac/invite_admin_role.yaml, config/rbac/invite_editor_role.yaml, config/rbac/invite_viewer_role.yaml, config/rbac/kustomization.yaml
Add ClusterRoles for invite admin/editor/viewer and include them in RBAC kustomization.
APIExport / APIResourceSchema changes
config/resources/apiexport-core.platform-mesh.io.yaml, config/resources/apiresourceschema-invites.core.platform-mesh.io.yaml, config/resources/apiexport-fga.openmfp.org.yaml (removed), config/resources/apiresourceschema-authorizationmodels.fga.openmfp.org.yaml (removed)
Add invites schema entry to APIExport, introduce Invite APIResourceSchema (replacing prior Store semantics), and remove legacy APIExport/AuthorizationModel schema files.
Samples
config/samples/core_v1alpha1_invite.yaml, config/samples/kustomization.yaml
Add Invite sample manifest and include it in samples kustomization.
Dependencies
go.mod
Modify module graph: adjust Go version, add/promote dependencies (e.g., github.com/coreos/go-oidc, golang.org/x/oauth2) and update several indirects.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title concisely and accurately describes the introduction of the initial invite implementation feature, clearly communicating the main change without extraneous detail.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/invitation-controller

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

Copy link
Contributor

@nexus49 nexus49 left a comment

Choose a reason for hiding this comment

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

Do we need to consider if a IDP was configured, i assume if an IDP was configured, then we don't need to create the user in keycloak, is that right?

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.

Actionable comments posted: 6

🧹 Nitpick comments (6)
config/crd/bases/core.platform-mesh.io_invites.yaml (1)

42-46: Strengthen email validation (add format/pattern via Go type tags).

Add kubebuilder validation to InviteSpec.Email (format "email", optional max length/pattern) so controller-gen emits stronger schema here. Edit Go type, not this generated file.

Example (api/v1alpha1/invite_types.go):

// +kubebuilder:validation:Format=email
// +kubebuilder:validation:MaxLength=320
Email string `json:"email"`
internal/subroutine/invite/subroutine_test.go (3)

200-202: Use oidc.ClientContext to inject HTTP client.

oidc.NewProvider reads http.Client from oidc.ClientContext, not oauth2.HTTPClient. Wrap the test client to ensure discovery uses the httptest server.

ctx := oidc.ClientContext(t.Context(), srv.Client())

98-101: Assert required actions payload is sent.

The real Keycloak endpoint expects a JSON array of actions in the request body. Decode and assert here to prevent regressions.

mux.HandleFunc("PUT /admin/realms/acme/users/{id}/execute-actions-email", func(w http.ResponseWriter, r *http.Request) {
  var actions []string
  assert.NoError(t, json.NewDecoder(r.Body).Decode(&actions))
  assert.ElementsMatch(t, []string{"UPDATE_PASSWORD", "VERIFY_EMAIL"}, actions)
  w.WriteHeader(http.StatusNoContent)
})

61-67: Name the test case.

Empty desc makes failures harder to diagnose. Give it a short, descriptive name.

internal/subroutine/invite/subroutine.go (2)

53-56: Include OAuth2 scope and allow HTTP client injection via context.

Keycloak commonly expects the openid scope; also, ensure custom http.Client can be injected for discovery/token calls.

 config := oauth2.Config{
   ClientID: cfg.Invite.KeycloakClientID,
   Endpoint: provider.Endpoint(),
+  Scopes:   []string{"openid"},
 }

In tests or callers, set the client using oidc.ClientContext(ctx, httpClient).


80-85: Defensive type assertion.

Avoid panic on unexpected types; return an operator error instead.

inv, ok := instance.(*v1alpha1.Invite)
if !ok {
  return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("unexpected object type %T", instance), false, false)
}
invite := inv
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ecc5018 and 6f5735e.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (22)
  • PROJECT (1 hunks)
  • api/v1alpha1/invite_types.go (1 hunks)
  • api/v1alpha1/zz_generated.deepcopy.go (1 hunks)
  • cmd/operator.go (1 hunks)
  • cmd/root.go (1 hunks)
  • config/crd/bases/core.platform-mesh.io_invites.yaml (1 hunks)
  • config/crd/kustomization.yaml (1 hunks)
  • config/rbac/invite_admin_role.yaml (1 hunks)
  • config/rbac/invite_editor_role.yaml (1 hunks)
  • config/rbac/invite_viewer_role.yaml (1 hunks)
  • config/rbac/kustomization.yaml (1 hunks)
  • config/resources/apiexport-core.platform-mesh.io.yaml (1 hunks)
  • config/resources/apiexport-fga.openmfp.org.yaml (0 hunks)
  • config/resources/apiresourceschema-authorizationmodels.fga.openmfp.org.yaml (0 hunks)
  • config/resources/apiresourceschema-invites.core.platform-mesh.io.yaml (3 hunks)
  • config/samples/core_v1alpha1_invite.yaml (1 hunks)
  • config/samples/kustomization.yaml (1 hunks)
  • go.mod (3 hunks)
  • internal/config/config.go (2 hunks)
  • internal/controller/invite_controller.go (1 hunks)
  • internal/subroutine/invite/subroutine.go (1 hunks)
  • internal/subroutine/invite/subroutine_test.go (1 hunks)
💤 Files with no reviewable changes (2)
  • config/resources/apiresourceschema-authorizationmodels.fga.openmfp.org.yaml
  • config/resources/apiexport-fga.openmfp.org.yaml
🧰 Additional context used
🧬 Code graph analysis (6)
api/v1alpha1/invite_types.go (1)
api/v1alpha1/groupversion_info.go (1)
  • SchemeBuilder (16-16)
cmd/operator.go (1)
internal/controller/invite_controller.go (1)
  • NewInviteReconciler (23-45)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/invite_types.go (4)
  • Invite (23-37)
  • InviteList (52-56)
  • InviteSpec (9-11)
  • InviteStatus (14-16)
internal/subroutine/invite/subroutine_test.go (4)
internal/subroutine/mocks/mock_Client.go (2)
  • MockClient (22-24)
  • NewMockClient (773-783)
api/v1alpha1/invite_types.go (2)
  • Invite (23-37)
  • InviteSpec (9-11)
internal/subroutine/invite/subroutine.go (1)
  • New (42-66)
internal/config/config.go (2)
  • Config (11-38)
  • InviteConfig (3-8)
internal/controller/invite_controller.go (3)
internal/config/config.go (1)
  • Config (11-38)
api/v1alpha1/invite_types.go (1)
  • Invite (23-37)
internal/subroutine/invite/subroutine.go (1)
  • New (42-66)
internal/subroutine/invite/subroutine.go (2)
internal/config/config.go (1)
  • Config (11-38)
api/v1alpha1/invite_types.go (1)
  • Invite (23-37)
🪛 Checkov (3.2.334)
config/rbac/invite_admin_role.yaml

[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles

(CKV_K8S_49)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: pipe / dockerBuild / docker
  • GitHub Check: pipe / lint / lint
  • GitHub Check: pipe / testSource / test
🔇 Additional comments (4)
internal/controller/invite_controller.go (4)

1-17: LGTM!

The imports are well-organized and all dependencies are necessary for the lifecycle-based reconciler implementation.


19-21: LGTM!

The InviteReconciler struct follows the standard lifecycle pattern, encapsulating the lifecycle manager cleanly.


47-49: LGTM!

The Reconcile method correctly delegates to the lifecycle manager with the appropriate resource type.


51-63: LGTM!

The SetupWithManager method correctly wires the lifecycle manager with condition management and cluster context support, following the standard pattern for lifecycle-based controllers in this codebase.

Comment on lines 156 to 163
if err = json.NewDecoder(res.Body).Decode(&users); err != nil {
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}

newUser = users[0]

log.Debug().Str("email", invite.Spec.Email).Str("id", newUser.ID).Msg("User created")

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Guard against empty user list after creation to avoid panic.

Indexing users[0] without checking length can panic if the API returns an empty list.

-	if err = json.NewDecoder(res.Body).Decode(&users); err != nil {
+	if err = json.NewDecoder(res.Body).Decode(&users); err != nil {
 		return ctrl.Result{}, errors.NewOperatorError(err, true, true)
 	}
 
-	newUser = users[0]
+	if len(users) == 0 {
+		return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("created user not found by email"), true, true)
+	}
+	newUser = users[0]
📝 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
if err = json.NewDecoder(res.Body).Decode(&users); err != nil {
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}
newUser = users[0]
log.Debug().Str("email", invite.Spec.Email).Str("id", newUser.ID).Msg("User created")
++ b/internal/subroutine/invite/subroutine.go
@@ Lines 156-163
if err = json.NewDecoder(res.Body).Decode(&users); err != nil {
return ctrl.Result{}, errors.NewOperatorError(err, true, true)
}
-
if len(users) == 0 {
return ctrl.Result{}, errors.NewOperatorError(
fmt.Errorf("created user not found by email"),
true,
true,
)
}
newUser = users[0]
log.Debug().
Str("email", invite.Spec.Email).
Str("id", newUser.ID).
Msg("User created")
🤖 Prompt for AI Agents
In internal/subroutine/invite/subroutine.go around lines 156 to 163, the code
decodes an API response into users and immediately indexes users[0] which can
panic if the returned slice is empty; update the code to check the length of
users after decoding, return an appropriate OperatorError (or ctrl.Result error)
when users is empty, and only assign newUser and log when users has at least one
element to safely guard against an empty user list.

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.

Actionable comments posted: 0

♻️ Duplicate comments (2)
internal/subroutine/invite/subroutine.go (2)

160-160: Critical: Guard against empty user list to prevent panic (duplicate issue).

This issue was previously flagged but remains unresolved. Indexing users[0] without checking length will panic if Keycloak returns an empty list after user creation.

Apply this diff to fix:

 	if err = json.NewDecoder(res.Body).Decode(&users); err != nil { // coverage-ignore
 		return ctrl.Result{}, errors.NewOperatorError(err, true, true)
 	}
 
+	if len(users) == 0 {
+		return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("created user not found by email"), true, true)
+	}
+
 	newUser = users[0]

164-178: Critical: Keycloak execute-actions-email requires request body (duplicate issue).

This issue was previously flagged but remains unresolved. Keycloak's execute-actions-email endpoint expects a JSON array of required actions in the request body. Sending http.NoBody will likely fail in real environments.

Apply this diff to send the actions with proper Content-Type:

-	req, err := http.NewRequestWithContext(ctx, http.MethodPut, fmt.Sprintf("%s/admin/realms/%s/users/%s/execute-actions-email", s.keycloakBaseURL, realm, newUser.ID), http.NoBody)
-	if err != nil {
-		return ctrl.Result{}, errors.NewOperatorError(err, true, true)
-	}
+	actions := []string{RequiredActionUpdatePassword, RequiredActionVerifyEmail}
+	var actionBuf bytes.Buffer
+	if err := json.NewEncoder(&actionBuf).Encode(actions); err != nil {
+		return ctrl.Result{}, errors.NewOperatorError(err, true, true)
+	}
+	req, err := http.NewRequestWithContext(ctx, http.MethodPut, fmt.Sprintf("%s/admin/realms/%s/users/%s/execute-actions-email", s.keycloakBaseURL, realm, newUser.ID), &actionBuf)
+	if err != nil {
+		return ctrl.Result{}, errors.NewOperatorError(err, true, true)
+	}
+	req.Header.Set("Content-Type", "application/json")
🧹 Nitpick comments (2)
internal/subroutine/invite/subroutine.go (2)

42-66: Consider validating config parameters and password.

The constructor doesn't validate critical config fields (KeycloakBaseURL, KeycloakClientID, KeycloakUser) or the password parameter before use. This could lead to confusing errors later.

Consider adding validation at the start of the function:

 func New(ctx context.Context, cfg *config.Config, cl client.Client, pwd string) (*subroutine, error) {
+	if cfg.Invite.KeycloakBaseURL == "" {
+		return nil, fmt.Errorf("KeycloakBaseURL is required")
+	}
+	if cfg.Invite.KeycloakClientID == "" {
+		return nil, fmt.Errorf("KeycloakClientID is required")
+	}
+	if cfg.Invite.KeycloakUser == "" {
+		return nil, fmt.Errorf("KeycloakUser is required")
+	}
+	if pwd == "" {
+		return nil, fmt.Errorf("password is required")
+	}
+
 	s := &subroutine{
 		keycloakBaseURL: cfg.Invite.KeycloakBaseURL,
 		cl:              cl,
 	}

100-118: Consider extracting duplicate user query logic.

The user query logic (GET request, decode, error handling) is duplicated at lines 100-118 and lines 145-163. Extracting this into a helper method would improve maintainability.

Example helper method:

func (s *subroutine) queryUsersByEmail(ctx context.Context, realm, email string) ([]keycloakUser, error) {
	v := url.Values{
		"email":               {email},
		"max":                 {"1"},
		"briefRepresentation": {"true"},
	}
	
	res, err := s.keycloak.Get(fmt.Sprintf("%s/admin/realms/%s/users?%s", s.keycloakBaseURL, realm, v.Encode()))
	if err != nil {
		return nil, fmt.Errorf("failed to query users: %w", err)
	}
	defer res.Body.Close()
	
	if res.StatusCode != http.StatusOK {
		return nil, fmt.Errorf("failed to query users: %s", res.Status)
	}
	
	var users []keycloakUser
	if err = json.NewDecoder(res.Body).Decode(&users); err != nil {
		return nil, fmt.Errorf("failed to decode users: %w", err)
	}
	
	return users, nil
}

Also applies to: 145-163

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f5735e and 8c6c319.

📒 Files selected for processing (1)
  • internal/subroutine/invite/subroutine.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/subroutine/invite/subroutine.go (2)
internal/config/config.go (1)
  • Config (11-38)
api/v1alpha1/invite_types.go (1)
  • Invite (23-37)
🪛 GitHub Actions: ci
internal/subroutine/invite/subroutine.go

[error] 49-51: Uncovered lines detected by coverage tool (49-51).


[error] 59-61: Uncovered lines detected by coverage tool (59-61).


[error] 69-71: Uncovered lines detected by coverage tool (69-71).


[error] 74-74: Uncovered line detected by coverage tool (74).


[error] 77-77: Uncovered line detected by coverage tool (77).


[error] 115-118: Uncovered lines detected by coverage tool (115-118).


[error] 152-154: Uncovered lines detected by coverage tool (152-154).


[error] 165-167: Uncovered lines detected by coverage tool (165-167).


[error] 176-178: Uncovered lines detected by coverage tool (176-178).


[error] -1--1: Uncovered lines detected by coverage tool (74 and 77).

🔇 Additional comments (1)
internal/subroutine/invite/subroutine.go (1)

93-93: Verify hardcoded AccountInfo key
Ensure s.cl.Get(ctx, client.ObjectKey{Name: "account"}, &accountInfo) uses the exact name of the AccountInfo CR in all environments.

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.

Actionable comments posted: 1

♻️ Duplicate comments (2)
internal/subroutine/invite/subroutine.go (2)

154-160: Guard against empty user list (repeat).

We still index users[0] without verifying len(users) > 0, so an empty response will panic. Please handle the empty case before dereferencing.


162-176: Send required actions payload with execute-actions-email (repeat).

Keycloak expects a JSON array body; calling with http.NoBody fails on real servers. Please encode the required actions and set Content-Type.

🧹 Nitpick comments (1)
internal/subroutine/invite/subroutine_test.go (1)

62-104: Name the happy-path test case.

The happy-path entry uses an empty description, so t.Run reports a blank name. Please set a real label (e.g. "success") to keep sub-test output readable.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c6c319 and 743f718.

📒 Files selected for processing (2)
  • internal/subroutine/invite/subroutine.go (1 hunks)
  • internal/subroutine/invite/subroutine_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/subroutine/invite/subroutine.go (2)
internal/config/config.go (1)
  • Config (11-38)
api/v1alpha1/invite_types.go (1)
  • Invite (23-37)
internal/subroutine/invite/subroutine_test.go (4)
internal/subroutine/mocks/mock_Client.go (2)
  • MockClient (22-24)
  • NewMockClient (773-783)
api/v1alpha1/invite_types.go (2)
  • Invite (23-37)
  • InviteSpec (9-11)
internal/subroutine/invite/subroutine.go (1)
  • New (42-64)
internal/config/config.go (2)
  • Config (11-38)
  • InviteConfig (3-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: pipe / dockerBuild / docker

@aaronschweig
Copy link
Contributor Author

Do we need to consider if a IDP was configured, i assume if an IDP was configured, then we don't need to create the user in keycloak, is that right?

How do we want to handle that case?

I could of course check for the existence of any IDP inside the realm, but depending on the organization configuration the email domain of the user might not match the one of the idp.

We could also think of creating the user anyway, but instead of requiring a password udpate, we could require the linking of an IDP - if an IDP is present in the org.

I don't have a strong opinion on both, just wondering what the most sensible approach would be for the future.

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.

Actionable comments posted: 2

🧹 Nitpick comments (4)
internal/subroutine/invite/subroutine_test.go (4)

26-51: Consider error handling in HTTP handlers.

Using assert.NoError inside HTTP handlers (lines 37, 48) could cause test panics if encoding fails during concurrent execution. While unlikely with simple structures, consider logging errors or using require.NoError for clearer test failures.

Optionally, you can handle errors explicitly:

 		err := json.NewEncoder(w).Encode(&map[string]string{
 			"issuer":                 fmt.Sprintf("%s/realms/master", baseURL),
 			"authorization_endpoint": fmt.Sprintf("%s/realms/master/protocol/openid-connect/auth", baseURL),
 			"token_endpoint":         fmt.Sprintf("%s/realms/master/protocol/openid-connect/token", baseURL),
 		})
-		assert.NoError(t, err)
+		if err != nil {
+			t.Errorf("failed to encode OIDC config: %v", err)
+			w.WriteHeader(http.StatusInternalServerError)
+		}

188-189: Use Kubernetes API errors for realistic simulation.

The mock returns a generic error. Use apierrors.NewNotFound from k8s.io/apimachinery/pkg/api/errors to simulate a more realistic Kubernetes API response pattern.

Apply this diff:

+import (
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+)

 			setupK8sMocks: func(m *mocks.MockClient) {
 				// Simulate k8s Get failure (AccountInfo missing)
-				m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(fmt.Errorf("accountinfo not found"))
+				m.EXPECT().Get(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(
+					apierrors.NewNotFound(schema.GroupResource{Group: "accounts.platform-mesh.io", Resource: "accountinfos"}, "test"),
+				)
 			},

53-232: Consider adding test cases for additional scenarios.

The current test suite covers basic success and error paths but is missing coverage for:

  • User already exists in Keycloak (idempotency check)
  • execute-actions-email endpoint failure (500 response)
  • AccountInfo exists but Organization.Name is empty
  • Concurrent invite processing

Adding these cases would improve confidence in the subroutine's resilience and edge-case handling.


234-256: Consider expanding Finalize test coverage.

The TestHelperFunctions validates basic metadata but only tests Finalize with a minimal Invite object. Consider adding test cases where Finalize is called with various invite states (e.g., with status fields populated) to ensure cleanup logic handles all scenarios correctly.

Example expansion:

func TestHelperFunctions(t *testing.T) {
	// ... existing setup ...

	t.Run("Finalize with populated status", func(t *testing.T) {
		invite := &v1alpha1.Invite{
			Spec: v1alpha1.InviteSpec{Email: "[email protected]"},
			Status: v1alpha1.InviteStatus{/* add status fields if defined */},
		}
		res, finalizerErr := s.Finalize(ctx, invite)
		assert.Nil(t, finalizerErr)
		assert.Equal(t, ctrl.Result{}, res)
	})
}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 743f718 and 5ac4629.

📒 Files selected for processing (1)
  • internal/subroutine/invite/subroutine_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/subroutine/invite/subroutine_test.go (4)
internal/subroutine/mocks/mock_Client.go (2)
  • MockClient (22-24)
  • NewMockClient (773-783)
api/v1alpha1/invite_types.go (2)
  • Invite (23-37)
  • InviteSpec (9-11)
internal/subroutine/invite/subroutine.go (1)
  • New (42-64)
internal/config/config.go (2)
  • Config (11-38)
  • InviteConfig (3-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: pipe / dockerBuild / docker
  • GitHub Check: pipe / lint / lint
🔇 Additional comments (2)
internal/subroutine/invite/subroutine_test.go (2)

1-24: LGTM! Clean imports and package structure.

The external test package pattern and import organization follow Go best practices.


203-203: No changes needed: go.mod specifies Go 1.24.5 (≥1.21) which supports t.Context()

Copy link
Contributor

@akafazov akafazov left a comment

Choose a reason for hiding this comment

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

LGTM

@akafazov akafazov requested a review from nexus49 October 9, 2025 10:03
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.

Actionable comments posted: 0

♻️ Duplicate comments (3)
internal/subroutine/invite/subroutine.go (2)

168-174: Guard against empty user list before indexing.

Keycloak can legitimately return an empty result (replication lag, filtering, etc.); indexing users[0] will panic and crash the reconciliation loop. Bail out when the list is empty instead.

Apply this diff:

 	if err = json.NewDecoder(res.Body).Decode(&users); err != nil { // coverage-ignore
 		return ctrl.Result{}, errors.NewOperatorError(err, true, true)
 	}
 
-	newUser = users[0]
+	if len(users) == 0 {
+		return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("created user not found by email"), true, true)
+	}
+	newUser = users[0]

176-189: Send required actions payload when triggering Keycloak email.

Keycloak’s execute-actions-email endpoint requires a JSON array of required action names; issuing the PUT with an empty body results in a 400 or missing actions. Encode and send the action list.

Apply this diff:

-	req, err := http.NewRequestWithContext(ctx, http.MethodPut, fmt.Sprintf("%s/admin/realms/%s/users/%s/execute-actions-email", s.keycloakBaseURL, realm, newUser.ID), http.NoBody)
-	if err != nil {
-		return ctrl.Result{}, errors.NewOperatorError(err, true, true)
-	}
+	actionPayload := []string{RequiredActionUpdatePassword, RequiredActionVerifyEmail}
+	var actionBuf bytes.Buffer
+	if err := json.NewEncoder(&actionBuf).Encode(actionPayload); err != nil {
+		return ctrl.Result{}, errors.NewOperatorError(err, true, true)
+	}
+	req, err := http.NewRequestWithContext(ctx, http.MethodPut, fmt.Sprintf("%s/admin/realms/%s/users/%s/execute-actions-email", s.keycloakBaseURL, realm, newUser.ID), &actionBuf)
+	if err != nil {
+		return ctrl.Result{}, errors.NewOperatorError(err, true, true)
+	}
+	req.Header.Set("Content-Type", "application/json")
internal/controller/invite_controller.go (1)

25-45: Constructor structure is sound, but the error handling issue remains unresolved.

The overall approach is appropriate:

  • Reading the Keycloak password from a mounted file is standard practice for Kubernetes secrets
  • Using the context for one-time OIDC provider initialization and OAuth2 token acquisition during startup is acceptable
  • The builder pattern with condition management and multi-cluster support is correctly applied

However, the log.Fatal() calls on lines 28 and 33 remain unresolved from the previous review. Please address the existing feedback to return errors instead of terminating the process.

🧹 Nitpick comments (1)
internal/controller/invite_controller.go (1)

1-1: Consider more targeted coverage exclusions.

The package-level // coverage-ignore comment excludes the entire file from coverage. Since line 51 already uses a method-specific exclusion for SetupWithManager, consider applying targeted exclusions only where needed (e.g., constructor and setup boilerplate) rather than excluding the entire package. This ensures that the core Reconcile logic remains covered.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ac4629 and 86c9a80.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • cmd/operator.go (1 hunks)
  • go.mod (6 hunks)
  • internal/config/config.go (2 hunks)
  • internal/controller/invite_controller.go (1 hunks)
  • internal/subroutine/invite/subroutine.go (1 hunks)
  • internal/subroutine/invite/subroutine_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/controller/invite_controller.go (3)
internal/config/config.go (1)
  • Config (11-41)
api/v1alpha1/invite_types.go (1)
  • Invite (23-37)
internal/subroutine/invite/subroutine.go (1)
  • New (44-66)
cmd/operator.go (1)
internal/controller/invite_controller.go (1)
  • NewInviteReconciler (25-45)
internal/subroutine/invite/subroutine.go (2)
internal/config/config.go (1)
  • Config (11-41)
api/v1alpha1/invite_types.go (1)
  • Invite (23-37)
internal/subroutine/invite/subroutine_test.go (6)
internal/subroutine/mocks/mock_Client.go (2)
  • MockClient (22-24)
  • NewMockClient (835-845)
api/v1alpha1/invite_types.go (2)
  • Invite (23-37)
  • InviteSpec (9-11)
internal/subroutine/mocks/mock_Manager.go (1)
  • NewMockManager (827-837)
internal/subroutine/mocks/mock_Cluster.go (1)
  • NewMockCluster (509-519)
internal/subroutine/invite/subroutine.go (1)
  • New (44-66)
internal/config/config.go (2)
  • Config (11-41)
  • InviteConfig (3-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: pipe / testSource / test
  • GitHub Check: pipe / dockerBuild / docker
  • GitHub Check: pipe / lint / lint
🔇 Additional comments (2)
internal/controller/invite_controller.go (2)

47-49: LGTM!

The reconciliation delegation pattern is correctly implemented with proper cluster context enrichment.


51-62: LGTM!

The manager setup follows the standard controller-runtime pattern with proper configuration wiring for concurrency and debug settings. The method-level // coverage-ignore is appropriately scoped for this boilerplate code.

@aaronschweig aaronschweig enabled auto-merge (squash) October 13, 2025 08:59
@aaronschweig aaronschweig merged commit 0126a7d into main Oct 13, 2025
10 checks passed
@aaronschweig aaronschweig deleted the feat/invitation-controller branch October 13, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants