Conversation
Small example returning hardcoded data using goa with a helm chart for deployment, based on Eric's proof of concept. Signed-off-by: Jordan Evans <jevans@linuxfoundation.org>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
07e2cc8 to
3c5f580
Compare
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
…hanges to kubernetes templates Signed-off-by: Andres Tobon <andrest2455@gmail.com>
emsearcy
left a comment
There was a problem hiding this comment.
great work on the codebase. some comments on resources
.github/workflows/mega-linter.yml
Outdated
| - name: MegaLinter | ||
| id: ml | ||
| # Use the Documentation flavor. | ||
| uses: oxsecurity/megalinter/flavors/documentation@5a91fb06c83d0e69fbd23756d47438aa723b4a5a # 8.7.0 |
There was a problem hiding this comment.
please use Go flavor. Hash will be the same.
You can also remove any references to Vale and proselint as those won't be in the Go flavor.
| # SPDX-License-Identifier: MIT | ||
| --- | ||
| apiVersion: traefik.io/v1alpha1 | ||
| kind: IngressRoute |
There was a problem hiding this comment.
Suggestion to use native Gateway API instead of Ingress Route CRD
There was a problem hiding this comment.
For this request, I am commenting a TODO in the code to make the change. I'll need to look more into it to get it configured properly, so for now I'll put it aside.
| match: >- | ||
| Host(`{{.Values.ingress.hostname}}`) | ||
| priority: 10 | ||
| # TODO: add heimdall middleware once it is working - currently it causes a 403 |
There was a problem hiding this comment.
Suggestion: create a flag for enabling/disabling via the values file, so that the chart can be used standalone or as part of the umbrella chart. Even the inclusion of the ingress-route (or gateway API http route) could/should be configurable IMO
| # Copyright The Linux Foundation and each contributor to LFX. | ||
| # SPDX-License-Identifier: MIT | ||
| --- | ||
| apiVersion: jetstream.nats.io/v1beta2 |
There was a problem hiding this comment.
again, I'd wrap this entire resource in a values conditional
| history: 20 | ||
| storage: file | ||
| maxValueSize: 10485760 # 10MB | ||
| maxBytes: 1073741824 # 1GB |
There was a problem hiding this comment.
I'd also suggest making these configurable with defaults (or file a backlog ticket for this)
…ading of mock project data via the POST project endpoint Signed-off-by: Andres Tobon <andrest2455@gmail.com>
…ble from values.yaml Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
| - kind: Rule | ||
| match: >- | ||
| Host(`{{.Values.ingress.hostname}}`) && | ||
| (Path(`/projects`) || PathPrefix(`/projects/`) || Path(`/livez`) || Path(`/readyz`)) |
There was a problem hiding this comment.
/livez and /readyz don't need to be routed to as they're used internally by Kubernetes.
There was a problem hiding this comment.
I'm not sure if it's for another reason, but whenever I removed the paths from here I was getting a 404. It worked when I re-added it. So we can address this later once we've figured that out.
There was a problem hiding this comment.
The Traefik Gateway API provider utilizes HTTPRoute instead of Traefik's custom IngressRoute. Here's a example of what this might look like as an HTTPRoute
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: lfx-v2-project-service
namespace: lfx
spec:
parentRefs:
# Name of the Gateway resource
- name: {{ .Values.ingress.gateway.name }}
# Note: using websecure not requires having SSL certificate setup and declaring 'certificateRefs' in the Gateway config
sectionName: {{ .Values.ingress.gateway.section | default 'web' }}
namespace: lfx
hostnames:
- '{{ .Values.ingress.hostname }}'
rules:
- match:
- path:
value: "/projects"
{{- if .Values.heimdall.enabled }}
filters:
- type: ExtensionRef
extensionRef:
group: traefik.io
kind: Middleware
name: heimdall
{{ - end }}
backendRefs:
- name: lfx-v2-project-service
namespace: lfx
port: 8080
I'd recommend this file be renamed to match the type as well: httproute.yaml
There was a problem hiding this comment.
Isn't a Gateway resource also needed? Please let me know once you made updates to https://github.com/linuxfoundation/lfx-v2-helm to support the Gateway API and then I can make the corresponding changes here.
There was a problem hiding this comment.
Yes, I have the Gateway change up here: https://github.com/linuxfoundation/lfx-v2-helm/pull/11/files. I'd suggest updating values with something like:
traefik:
ingressroute:
enabled: true
entrypoints:
- web
- websecure
gateway:
enabled: false
entrypoint: weband adding this example as an additional resource wrapped in a check for traefik.gateway.enabled.
There was a problem hiding this comment.
Thanks. I'll make note of this and then have the resource change implemented as part of a separate PR after this is merged.
…T principal parsing Signed-off-by: Andres Tobon <andrest2455@gmail.com>
| compression: true | ||
|
|
||
| # heimdall is the configuration for the heimdall middleware | ||
| heimdall: |
There was a problem hiding this comment.
I don't think this should be duplicating the heimdall config in lfx-v2-helm, just defining the required values needed:
heimdall:
enabled: true
url: http://heimdall:4456/
There was a problem hiding this comment.
I'll remove the heimdall config other than enabling the middleware and setting the URL. However, the LFX platform heimdall service from lfx-v2-helm is not working from my testing locally so I'm still using the PoC deployment of heimdall. I assume it will be working once https://linuxfoundation.atlassian.net/browse/LFXV2-13 is done? That's where I had gotten the configuration of, but it's true that I shouldn't have that in the individual service repo.
- Configure OpenFGA integration in Helm chart with conditional authorization - Add granular authorization rules for project operations (viewer/writer relations) - Enhance Makefile with integration and authorization test targets - Update README with OpenFGA configuration and authorization documentation Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andres Tobon <andrest2455@gmail.com>
- Use Chart.AppVersion for deployment image tag instead of hardcoded version - Make Heimdall middleware URL configurable via values.yaml - Add configurable resource policy for NATS KV bucket preservation - Update NATS URL to use lfx-platform-nats service - Remove health check paths from ingress route (handled separately) - Simplify Heimdall configuration and remove complex mechanisms - Add helm-templates target to Makefile for template debugging - Update README with correct NATS service URL Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andres Tobon <andrest2455@gmail.com>
cmd/project-api/service_endpoint.go
Outdated
| } | ||
|
|
||
| // Send the transaction to the NATS server for the data indexing. | ||
| err = s.natsConn.Publish(subject, transactionBytes) |
There was a problem hiding this comment.
Can we please run this in parallel? I mean, both (indexing and access) are independent. What do you think? We might extract them in a separate method and execute them in the go routine, as I see they are being used for create/update/delete.
There was a problem hiding this comment.
That's a good point. Both messages can be published in parallel and I can break up that code into its own function so its reused across all endpoints.
cmd/project-api/service_endpoint.go
Outdated
| keysLister, err := s.projectsKV.ListKeys(ctx) | ||
| if err != nil { | ||
| slog.ErrorContext(ctx, "error listing project keys from NATS KV store", errKey, err) | ||
| return nil, &projsvc.InternalServerError{ |
There was a problem hiding this comment.
Can we create a helper function for error response? what do you think?
There was a problem hiding this comment.
That seems like a good idea to me. I'll make a function that takes in the code and message, so with that I can reuse the function across all the endpoints.
| gracefulShutdownSeconds = 25 | ||
| ) | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
I think we can refactor the main function into small functions -- what do you think?
func parseFlagsAndEnv() (port, bind string, debug bool, lfxEnv constants.LFXEnvironment, natsURL string)
func setupHTTPServer(logger *slog.Logger, svc *ProjectsService) *http.Server
func setupNATS(ctx context.Context, logger *slog.Logger, svc *ProjectsService, wg *sync.WaitGroup) (*nats.Conn, error)
func gracefulShutdownHandler(httpServer *http.Server, natsConn *nats.Conn, wg *sync.WaitGroup, logger *slog.Logger)
There was a problem hiding this comment.
That's fine with me. I think that should make it easier to maintain since right now the main function is quite large.
cmd/project-api/service_endpoint.go
Outdated
| transaction := msg.ProjectTransaction{ | ||
| Action: msg.ActionCreated, | ||
| Headers: map[string]string{}, | ||
| // Headers: map[string]string{ |
There was a problem hiding this comment.
I think we will need this, as the indexer service needs to set the permission values and push the data to the open search. Below are logs from the indexer service. The end-to-end flow will not work.
{"time":"2025-07-22T17:49:04.541839+05:30","level":"ERROR","msg":"V2 header validation failed: missing authorization","component":"indexer","error":"authorization header is required for V2 transactions","transaction_id":"txn_project_created_1753186744541807000","validation_step":"authorization_header","has_auth_header":false}
{"time":"2025-07-22T17:49:04.541842+05:30","level":"ERROR","msg":"Transaction enrichment failed: header validation","component":"nats","request_id":"0af6b7dccf3dafcf","error":"authorization header is required for V2 transactions","transaction_id":"txn_project_created_1753186744541807000","step":"validate_headers"}
{"time":"2025-07-22T17:49:04.541852+05:30","level":"ERROR","msg":"Failed to enrich transaction","component":"nats","request_id":"0af6b7dccf3dafcf","error":"invalid transaction headers: authorization header is required for V2 transactions","transaction_id":"txn_project_created_1753186744541807000","step":"enrichment"}
There was a problem hiding this comment.
Yeah no problem. I'm not sure why it was commented, but I'll uncomment it and check your implementation to make sure I am passing the expected headers.
Major refactoring to improve code organization and maintainability: - Extract project endpoints into dedicated file (service_endpoint_project.go) - Move NATS infrastructure code into centralized package with proper interfaces - Add authorization middleware to capture auth headers in request context - Refactor main.go for better separation of concerns and graceful shutdown - Fix unsafe type assertions in context value extraction (prevent panics) - Restore health check endpoints (/livez, /readyz) to ingress route - Add comprehensive tests for authorization middleware - Update NATS message handling to use builder pattern Technical improvements: - Remove redundant slog.Default() calls after log initialization - Fix RequestIDMiddleware test to use correct context constant - Consolidate NATS KV store and messaging models - Improve error handling in NATS setup Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andres Tobon <andrest2455@gmail.com>
| # SPDX-License-Identifier: MIT | ||
| --- | ||
| apiVersion: traefik.io/v1alpha1 | ||
| kind: IngressRoute |
Changes addressed, and other changes will be added in later PRs.
Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-2
This PR adds the initial codebase for the project service. The service has a REST API for performing CRUD operations on projects and also has an open NATS connection that is for listening to message subjects for getting a project name by ID and getting a project ID by slug. There are still missing pieces to the implementation, such as the use of heimdall for access control, but those can be added in further PRs.
--
This pull request introduces several key updates to the repository, including the addition of workflows for CI/CD, configuration for linter and license checks, and updates to licensing and ownership files. These changes aim to improve code quality, enforce licensing standards, and streamline development processes.
CI/CD Workflows
License Header Checkworkflow to validate license headers in pull requests. (.github/workflows/license-header-check.yml)MegaLinterworkflow to ensure code quality and enforce linting rules across the repository. (.github/workflows/mega-linter.yml)Project API Buildworkflow for building, testing, and managing dependencies for the project API. (.github/workflows/project-api-build.yml)Licensing and Ownership
LICENSEfile with the MIT license for the repository. (LICENSE)LICENSE-docsfile for documentation under the Creative Commons Attribution 4.0 International License. (LICENSE-docs)CODEOWNERSfile to assign ownership of the repository to the@linuxfoundation/lfx-platformteam. (CODEOWNERS)Configuration and Dockerization
.yamllintconfiguration file to enforce YAML style guidelines, including ignoring specific directories and setting a line-length warning. (.yamllint)Dockerfileto build and run the project API service, including multi-stage builds with Chainguard images and cross-compilation support. (Dockerfile)