Skip to content

SRVOCF-1081: List functions only from the current cluster #185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pmeida wants to merge 1 commit into
base: master
Choose a base branch
from
Open

SRVOCF-1081: List functions only from the current cluster #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 83 additions & 5 deletions backend/fakegithub/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"encoding/json"
"fmt"
"log"
"maps"
"net/http"
"slices"
"sort"
Expand All @@ -33,6 +34,7 @@ type repo struct {
Commits map[string]*commit
Refs map[string]string // "refs/heads/main" -> commit sha
Secrets map[string]string // name -> encrypted value
Variables map[string]string // name -> value
}

type treeEntry struct {
Expand Down Expand Up @@ -136,6 +138,11 @@ func (s *Server) routes() {
s.mux.HandleFunc("GET /repos/{owner}/{repo}/actions/secrets/public-key", s.handleGetPublicKey)
s.mux.HandleFunc("PUT /repos/{owner}/{repo}/actions/secrets/{name}", s.handlePutSecret)

// Actions variables
s.mux.HandleFunc("GET /repos/{owner}/{repo}/actions/variables/{name}", s.handleGetVariable)
s.mux.HandleFunc("POST /repos/{owner}/{repo}/actions/variables", s.handleCreateVariable)
s.mux.HandleFunc("PATCH /repos/{owner}/{repo}/actions/variables/{name}", s.handleUpdateVariable)

// Admin API (for test setup)
s.mux.HandleFunc("POST /_admin/seed", s.handleAdminSeed)
s.mux.HandleFunc("POST /_admin/reset", s.handleAdminReset)
Expand Down Expand Up @@ -205,6 +212,7 @@ func (s *Server) handleCreateRepo(w http.ResponseWriter, r *http.Request) {
Commits: make(map[string]*commit),
Refs: make(map[string]string),
Secrets: make(map[string]string),
Variables: make(map[string]string),
}

if body.AutoInit {
Expand Down Expand Up @@ -628,14 +636,82 @@ func (s *Server) handlePutSecret(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusCreated)
}

func (s *Server) handleGetVariable(w http.ResponseWriter, r *http.Request) {
s.mu.Lock()
defer s.mu.Unlock()

rp := s.getRepo(r)
if rp == nil {
writeError(w, http.StatusNotFound, "Not Found")
return
}

name := r.PathValue("name")
value, ok := rp.Variables[name]
if !ok {
writeError(w, http.StatusNotFound, "Not Found")
return
}
writeJSON(w, http.StatusOK, map[string]string{"name": name, "value": value})
}

func (s *Server) handleCreateVariable(w http.ResponseWriter, r *http.Request) {
s.mu.Lock()
defer s.mu.Unlock()

rp := s.getRepo(r)
if rp == nil {
writeError(w, http.StatusNotFound, "Not Found")
return
}

var body struct {
Name string `json:"name"`
Value string `json:"value"`
}
if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
writeError(w, http.StatusBadRequest, "invalid request body")
return
}
if _, exists := rp.Variables[body.Name]; exists {
writeError(w, http.StatusConflict, "variable already exists")
return
}
rp.Variables[body.Name] = body.Value
w.WriteHeader(http.StatusCreated)
}

func (s *Server) handleUpdateVariable(w http.ResponseWriter, r *http.Request) {
s.mu.Lock()
defer s.mu.Unlock()

rp := s.getRepo(r)
if rp == nil {
writeError(w, http.StatusNotFound, "Not Found")
return
}

name := r.PathValue("name")
var body struct {
Value string `json:"value"`
}
if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
writeError(w, http.StatusBadRequest, "invalid request body")
return
}
rp.Variables[name] = body.Value
w.WriteHeader(http.StatusNoContent)
}

// --- Admin API handlers ---

type seedRequest struct {
Owner string `json:"owner"`
Repo string `json:"repo"`
Branch string `json:"branch"`
Topics []string `json:"topics"`
Files []seedFile `json:"files"`
Owner string `json:"owner"`
Repo string `json:"repo"`
Branch string `json:"branch"`
Topics []string `json:"topics"`
Files []seedFile `json:"files"`
Variables map[string]string `json:"variables,omitempty"`
}

type seedFile struct {
Expand Down Expand Up @@ -669,11 +745,13 @@ func (s *Server) handleAdminSeed(w http.ResponseWriter, r *http.Request) {
Commits: make(map[string]*commit),
Refs: make(map[string]string),
Secrets: make(map[string]string),
Variables: make(map[string]string),
}

for _, f := range req.Files {
rp.Files[f.Path] = f.Content
}
maps.Copy(rp.Variables, req.Variables)
buildGitObjects(rp)

key := req.Owner + "/" + req.Repo
Expand Down
26 changes: 26 additions & 0 deletions backend/fakegithub/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,32 @@ var _ = Describe("FakeGitHub Server", func() {
})
})

Describe("StoreVariable + GetVariable", func() {
It("stores a variable and reads it back", func() {
err := cl.StoreVariable(context.Background(), "testuser", "test-func", "CLUSTER_API_URL", "https://api.my-cluster.example.com:6443")
Expect(err).NotTo(HaveOccurred())

value, err := cl.GetVariable(context.Background(), "testuser", "test-func", "CLUSTER_API_URL")
Expect(err).NotTo(HaveOccurred())
Expect(value).To(Equal("https://api.my-cluster.example.com:6443"))
})

It("returns empty string when variable does not exist", func() {
value, err := cl.GetVariable(context.Background(), "testuser", "test-func", "NO_SUCH_VAR")
Expect(err).NotTo(HaveOccurred())
Expect(value).To(BeEmpty())
})

It("overwrites a variable when stored again", func() {
Expect(cl.StoreVariable(context.Background(), "testuser", "test-func", "CLUSTER_API_URL", "first")).To(Succeed())
Expect(cl.StoreVariable(context.Background(), "testuser", "test-func", "CLUSTER_API_URL", "second")).To(Succeed())

value, err := cl.GetVariable(context.Background(), "testuser", "test-func", "CLUSTER_API_URL")
Expect(err).NotTo(HaveOccurred())
Expect(value).To(Equal("second"))
})
})

Describe("DeleteRepo", func() {
It("removes the repo so it is no longer listed", func() {
err := cl.DeleteRepo(context.Background(), "testuser", "test-func")
Expand Down
14 changes: 13 additions & 1 deletion backend/handler/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ import (
k8svalidation "k8s.io/apimachinery/pkg/util/validation"
)

const (
repoSecretKubeconfig = "KUBECONFIG"
repoVarClusterAPIURL = "CLUSTER_API_URL"
)

var (
validBranch = regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9._/-]*[a-zA-Z0-9])?$`)
validRuntimes = map[string]bool{"node": true, "python": true, "go": true, "quarkus": true}
Expand Down Expand Up @@ -135,13 +140,20 @@ func (h *Handlers) createFunction(ctx context.Context, req createRequest, pat, o
}
}()

if err := client.StoreSecret(ctx, req.Owner, req.Repo, "KUBECONFIG", kubeconfig); err != nil {
if err := client.StoreSecret(ctx, req.Owner, req.Repo, repoSecretKubeconfig, kubeconfig); err != nil {
if errors.Is(err, scm.ErrUnauthorized) {
return err
}
slog.Error("failed to store CI secret", "owner", req.Owner, "repo", req.Repo, "err", err)
return fmt.Errorf("%w: %w", errUpstream, fmt.Errorf("store secret: %w", err))
}
if err := client.StoreVariable(ctx, req.Owner, req.Repo, repoVarClusterAPIURL, h.externalAPIServerURL); err != nil {
if errors.Is(err, scm.ErrUnauthorized) {
return err
}
slog.Error("failed to store cluster variable", "owner", req.Owner, "repo", req.Repo, "err", err)
return fmt.Errorf("%w: %w", errUpstream, fmt.Errorf("store variable: %w", err))
}
if err := client.PushFiles(ctx, req.Owner, req.Repo, req.Branch, "Initialize Knative function project", files); err != nil {
if errors.Is(err, scm.ErrUnauthorized) {
return err
Expand Down
59 changes: 59 additions & 0 deletions backend/handler/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,24 @@ var _ = Describe("POST /api/v1/func/create", func() {
withClusterStub(&cluster.ClientStub{})
}, http.StatusBadGateway),

Entry("StoreVariable returns ErrUnauthorized", func() {
withSCMStub(&scm.ClientStub{
OnStoreVariable: func(ctx context.Context, owner, repo, name, value string) error {
return scm.ErrUnauthorized
},
})
withClusterStub(&cluster.ClientStub{})
}, http.StatusUnauthorized),

Entry("StoreVariable returns generic error", func() {
withSCMStub(&scm.ClientStub{
OnStoreVariable: func(ctx context.Context, owner, repo, name, value string) error {
return errors.New("github unavailable")
},
})
withClusterStub(&cluster.ClientStub{})
}, http.StatusBadGateway),

Entry("PushFiles returns ErrUnauthorized", func() {
withSCMStub(&scm.ClientStub{
OnPushFiles: func(ctx context.Context, owner, repo, branch, message string, files []scm.FileEntry) error {
Expand Down Expand Up @@ -348,6 +366,47 @@ var _ = Describe("POST /api/v1/func/create", func() {
Expect(calls).To(HaveKey("deleteRepo"))
})

It("rolls back cluster resources and repo when StoreVariable fails", func() {
calls := map[string]int{}
recordCall := func(key string) { calls[key]++ }

w := doCreate(func() {
withSCMStub(&scm.ClientStub{
OnStoreVariable: func(ctx context.Context, owner, repo, name, value string) error {
return errors.New("github down")
},
OnDeleteRepo: func(ctx context.Context, owner, repo string) error {
recordCall("deleteRepo")
return nil
},
})
withClusterStub(&cluster.ClientStub{
OnDeleteServiceAccount: func(ctx context.Context, namespace string) error {
recordCall("deleteServiceAccount")
return nil
},
OnDeleteRole: func(ctx context.Context, namespace string) error {
recordCall("deleteRole")
return nil
},
OnDeleteRoleBinding: func(ctx context.Context, namespace string) error {
recordCall("deleteRoleBinding")
return nil
},
OnDeleteImageBuilderBinding: func(ctx context.Context, namespace string) error {
recordCall("deleteImageBuilderBinding")
return nil
},
})
})
Expect(w.Code).To(Equal(http.StatusBadGateway))
Expect(calls).To(HaveKey("deleteServiceAccount"))
Expect(calls).To(HaveKey("deleteRole"))
Expect(calls).To(HaveKey("deleteRoleBinding"))
Expect(calls).To(HaveKey("deleteImageBuilderBinding"))
Expect(calls).To(HaveKey("deleteRepo"))
})

It("rolls back cluster resources and repo when PushFiles fails", func() {
calls := map[string]int{}
recordCall := func(key string) { calls[key]++ }
Expand Down
70 changes: 43 additions & 27 deletions backend/handler/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func (h *Handlers) HandleListFunctions(w http.ResponseWriter, r *http.Request) {
wg.Add(2)
go func() {
defer wg.Done()
repoFunctions, repoErr = listRepoFunctions(r.Context(), pat, namespace)
repoFunctions, repoErr = listRepoFunctions(r.Context(), pat, namespace, h.externalAPIServerURL)
}()
go func() {
defer wg.Done()
Expand Down Expand Up @@ -125,7 +125,7 @@ func (h *Handlers) HandleListFunctions(w http.ResponseWriter, r *http.Request) {
writeJSON(w, http.StatusOK, items)
}

func listRepoFunctions(ctx context.Context, pat, namespace string) ([]listItem, error) {
func listRepoFunctions(ctx context.Context, pat, namespace, clusterAPIURL string) ([]listItem, error) {
client := config.SCMRegistry.Client(scm.DefaultPlatform, pat)

repos, err := client.ListRepos(ctx)
Expand All @@ -134,55 +134,71 @@ func listRepoFunctions(ctx context.Context, pat, namespace string) ([]listItem,
}

items := make([]listItem, len(repos))
for i, repo := range repos {
items[i] = listItem{
Owner: repo.Owner,
RepoName: repo.Name,
RepoURL: repo.URL,
DefaultBranch: repo.DefaultBranch,
Source: sourceRepo,
}
}
excluded := make([]bool, len(repos))

g, gctx := errgroup.WithContext(ctx)
g.SetLimit(10)
for i, repo := range repos {
g.Go(func() error {
items[i] = listItem{
Owner: repo.Owner,
RepoName: repo.Name,
RepoURL: repo.URL,
DefaultBranch: repo.DefaultBranch,
Source: sourceRepo,
}

repoClusterURL, err := client.GetVariable(gctx, repo.Owner, repo.Name, repoVarClusterAPIURL)

@Cragsmann Cragsmann Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you reorder the goroutine to fetch func.yaml first, and only call GetVariable for repos that will survive filtering by namespace

@pmeida pmeida Sep 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Although the current approach always does both calls for every repo. There are downside to each suggestion:

  1. Cluster filtering is always active (unlike namespace filtering, which is optional). Deferring GetVariable past func.yaml means we pay the cost of fetching and parsing func.yaml for every repo before we can apply the primary filter of this PR. The cost of GetVariable should be lower then GetFileContent + parseFuncYaml.
  2. There's a correctness edge case: if func.yaml fails or is invalid, skipping GetVariable means cluster filtering can't run for that repo - a repo from a different cluster with a broken func.yaml would slip through instead of being excluded.
  3. The namespace early-return optimization only pays off when a namespace is provided and is selective. When no namespace is given (which is a common case), both orderings are equivalent in terms of API calls.

Anyway I evaluated the eficiency concerns and came up with the best solution.

Efficiency gains:
Repos from other clusters only pay for one API call (GetVariable) instead of two. In a mixed-cluster environment that can cut GitHub API usage by up to 50% per excluded repo.
On the CPU side: 2 fewer sequential loops over the repo list, and all filtering decisions are made inline during the goroutine pass so the data is touched once instead of three times.

@Cragsmann Cragsmann Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe we can do this in one loop only with Mutex? second loop in listRepoFunctions seems redundant. What about sorting by name at HandleListFunctions?

@pmeida pmeida Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Switching to Mutex+append would lose the original repo order (goroutines append in completion order), add lock contention with SetLimit(10), and save nothing measurable - the compaction loop is O(n) in-memory, so it is negligible.

Happy to add a name sort. Though the better UX would be by creation date so newly created functions surface at the top rather than appearing mid-list - but that needs CreatedAt exposed on the SCM client and FE changes to append on top of the list at creation time. So I think this is out of scope for this PR.

if err != nil {
slog.Warn("failed to read variable", "variable", repoVarClusterAPIURL, "repo", repo.Owner+"/"+repo.Name, "err", err)
// keep the repo on transient errors rather than hiding it
} else if repoClusterURL != clusterAPIURL {
// Filter out repos whose CLUSTER_API_URL variable does not match this cluster. The variable is written alongside
// the KUBECONFIG secret at repo creation, so a mismatch means the secret points to a different cluster
// (from where it was created) and deploying from this console would target the wrong cluster.
excluded[i] = true
return nil
}

content, err := client.GetFileContent(gctx, repo.Owner, repo.Name, repo.DefaultBranch, "func.yaml")
if err != nil {
slog.Warn("failed to read func.yaml", "repo", repo.Owner+"/"+repo.Name, "err", err)
items[i].Err = "failed to read func.yaml"
if namespace != "" {
excluded[i] = true
} else {
items[i].Err = "failed to read func.yaml"
}
return nil
}
name, namespace, runtime, parseErr := parseFuncYaml(content)
name, funcNamespace, runtime, parseErr := parseFuncYaml(content)
if parseErr != nil {
slog.Warn("failed to parse func.yaml", "repo", repo.Owner+"/"+repo.Name, "err", parseErr)
items[i].Err = "invalid func.yaml"
if namespace != "" {
excluded[i] = true
} else {
items[i].Err = "invalid func.yaml"
}
return nil
}
items[i].Name = name
items[i].Namespace = namespace
items[i].Namespace = funcNamespace
items[i].Runtime = runtime

if namespace != "" && funcNamespace != namespace {
excluded[i] = true
}
return nil
})
}
_ = g.Wait()

if namespace != "" {
items = filterByNamespace(items, namespace)
}

return items, nil
}

func filterByNamespace(items []listItem, namespace string) []listItem {
filtered := make([]listItem, 0, len(items))
for _, item := range items {
if item.Namespace == namespace {
filtered := items[:0]
for i, item := range items {
if !excluded[i] {
filtered = append(filtered, item)
}
}
return filtered
return filtered, nil
}

func (h *Handlers) listClusterFunctions(ctx context.Context, ocpToken, namespace string) ([]listItem, error) {
Expand Down
Loading