feat(cli): service to service access#146
Conversation
📝 WalkthroughWalkthroughThis PR implements service-to-service access control and cross-service URL injection. Changes include: introducing a "service" resource type with "invoke" action to the access schema; extending ServiceIntent with access mappings; adding validation for service access definitions; implementing lazy port allocation and dynamic environment variable injection for service URL discovery in simulation; and adding service accessor collection with URL environment variable merging in Terraform deployment. Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cli/internal/simulation/simulation.go(2 hunks)cli/pkg/schema/access.go(1 hunks)cli/pkg/schema/schema_test.go(1 hunks)cli/pkg/schema/service.go(1 hunks)cli/pkg/schema/validate.go(1 hunks)engines/terraform/resource_handler.go(3 hunks)engines/terraform/suga.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
cli/internal/simulation/simulation.go (2)
cli/internal/netx/net.go (2)
ReservedPort(52-52)GetNextPort(72-93)cli/internal/simulation/service/service.go (1)
ServiceEvent(42-47)
engines/terraform/resource_handler.go (4)
engines/terraform/deployment.go (1)
TerraformDeployment(18-32)cli/pkg/schema/schema.go (1)
Application(13-23)cli/pkg/schema/access.go (2)
ExpandActions(31-43)Service(10-10)engines/terraform/suga.go (1)
SugaServiceVariables(7-15)
cli/pkg/schema/validate.go (1)
cli/pkg/schema/access.go (3)
ValidateActions(46-56)Service(10-10)GetValidActions(21-28)
cli/pkg/schema/schema_test.go (3)
cli/pkg/schema/schema.go (2)
Application(13-23)ApplicationFromYaml(97-111)cli/pkg/schema/service.go (3)
ServiceIntent(3-16)Container(38-41)Docker(44-48)cli/pkg/schema/errors.go (2)
FormatValidationErrors(227-248)GetSchemaValidationErrors(190-216)
🔇 Additional comments (2)
cli/pkg/schema/validate.go (1)
102-122: Service access validation looks solid.The implementation correctly validates both action validity and accessor service existence, following the established pattern from bucket/database validation. Error messages are clear and the logic handles edge cases properly.
cli/pkg/schema/schema_test.go (1)
600-725: Excellent test coverage.The four test functions comprehensively cover valid configurations, invalid actions, non-existent accessors, and YAML parsing. The target-based access model is well-tested with both simple and complex cross-service scenarios.
| envVarName := fmt.Sprintf("%s_URL", strings.ToUpper(targetServiceName)) | ||
| intentCopy.Env[envVarName] = fmt.Sprintf("http://localhost:%d", targetPort) | ||
| } |
There was a problem hiding this comment.
Match production-safe env key naming
Simulation still emits PAYMENTS-API_URL, while Terraform (after fixing) will emit PAYMENTS_API_URL. Services would read different env vars locally vs. deployed. Please apply the same hyphen-to-underscore normalization here so developers see identical keys.
Apply this diff:
- envVarName := fmt.Sprintf("%s_URL", strings.ToUpper(targetServiceName))
+ safeName := strings.ToUpper(strings.ReplaceAll(targetServiceName, "-", "_"))
+ envVarName := fmt.Sprintf("%s_URL", safeName)📝 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.
| envVarName := fmt.Sprintf("%s_URL", strings.ToUpper(targetServiceName)) | |
| intentCopy.Env[envVarName] = fmt.Sprintf("http://localhost:%d", targetPort) | |
| } | |
| safeName := strings.ToUpper(strings.ReplaceAll(targetServiceName, "-", "_")) | |
| envVarName := fmt.Sprintf("%s_URL", safeName) | |
| intentCopy.Env[envVarName] = fmt.Sprintf("http://localhost:%d", targetPort) | |
| } |
🤖 Prompt for AI Agents
In cli/internal/simulation/simulation.go around lines 330 to 332, the env var
key is built with strings.ToUpper(targetServiceName) producing names like
PAYMENTS-API_URL; change the normalization to replace hyphens with underscores
before uppercasing so keys become PAYMENTS_API_URL (e.g. use
strings.ReplaceAll(targetServiceName, "-", "_") wrapped with strings.ToUpper
when constructing the fmt.Sprintf pattern) and keep the existing "_URL" suffix.
| envVarName := fmt.Sprintf("%s_URL", strings.ToUpper(targetServiceName)) | ||
| httpEndpoint := targetResource.Get(jsii.String("suga.http_endpoint")) | ||
| serviceEnvs[accessorServiceName] = append(serviceEnvs[accessorServiceName], map[string]interface{}{ | ||
| envVarName: httpEndpoint, | ||
| }) |
There was a problem hiding this comment.
Sanitize service URL env keys
Service names like payments-api now emit PAYMENTS-API_URL, which many runtimes (e.g. AWS Lambda/ECS) reject because env keys must use only letters, digits, and underscores. Please normalize the name (replace - with _) before uppercasing so deployments don’t fail for valid service names, and mirror the same transformation in simulation to keep parity.
Apply this diff:
- envVarName := fmt.Sprintf("%s_URL", strings.ToUpper(targetServiceName))
+ safeName := strings.ToUpper(strings.ReplaceAll(targetServiceName, "-", "_"))
+ envVarName := fmt.Sprintf("%s_URL", safeName)📝 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.
| envVarName := fmt.Sprintf("%s_URL", strings.ToUpper(targetServiceName)) | |
| httpEndpoint := targetResource.Get(jsii.String("suga.http_endpoint")) | |
| serviceEnvs[accessorServiceName] = append(serviceEnvs[accessorServiceName], map[string]interface{}{ | |
| envVarName: httpEndpoint, | |
| }) | |
| safeName := strings.ToUpper(strings.ReplaceAll(targetServiceName, "-", "_")) | |
| envVarName := fmt.Sprintf("%s_URL", safeName) | |
| httpEndpoint := targetResource.Get(jsii.String("suga.http_endpoint")) | |
| serviceEnvs[accessorServiceName] = append(serviceEnvs[accessorServiceName], map[string]interface{}{ | |
| envVarName: httpEndpoint, | |
| }) |
🤖 Prompt for AI Agents
In engines/terraform/resource_handler.go around lines 165 to 169, the code
constructs env var names by uppercasing the service name but does not sanitize
characters like hyphens, producing invalid keys such as PAYMENTS-API_URL;
replace non-alphanumeric characters (at least '-' -> '_') in targetServiceName
before uppercasing and appending _URL, e.g. normalizeName :=
strings.ToUpper(strings.ReplaceAll(targetServiceName, "-", "_")), use that for
envVarName, and ensure the identical normalization is applied in the simulation
codepath so runtime and simulation generate the same sanitized env keys.
davemooreuws
changed the title
| if err != nil { | ||
| return nil, err | ||
| } | ||
| envVarName := fmt.Sprintf("%s_URL", strings.ToUpper(targetServiceName)) |
There was a problem hiding this comment.
wonder if these names should be prefixed with SUGA
|
happy to review and test after the rabbit feedback has been resolved to avoid testing something stale. |
|
Closing as not planned. |
3 participants
A part of https://linear.app/nitric/issue/NIT-455/suga-feedback-01