address reviews

mjudeikis · mjudeikis · commit 89c2cbaaa003 · 2025-11-05T17:10:15.000+02:00
diff --git a/.github/workflows/image.yaml b/.github/workflows/image.yaml
@@ -22,13 +22,6 @@ jobs:
         go-version: v1.24.0
         check-latest: true
 
-    - name: Set up Node.js
-      uses: actions/setup-node@v4
-      with:
-        node-version: '20'
-        cache: 'npm'
-        cache-dependency-path: web/package-lock.json
-
     # We need this to remove local tags that are not semver so goreleaser doesn't get confused.
     - name: Delete non-semver tags
       run: 'git tag -d $(git tag -l | grep -v "^v")'
diff --git a/Dockerfile.konnector b/Dockerfile.konnector
@@ -1,4 +1,4 @@
-# Copyright 2022 The Kube Bind Authors.
+# Copyright 2025 The Kube Bind Authors.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
diff --git a/backend/auth/handler.go b/backend/auth/handler.go
@@ -184,45 +184,47 @@ func (ah *AuthHandler) HandleCallback(w http.ResponseWriter, r *http.Request) {
 		parsedRedirectURL.RawQuery = values.Encode()
 
 		http.Redirect(w, r, parsedRedirectURL.String(), http.StatusFound)
-	} else {
-		// UI flow - set cookie and redirect to UI
-		cookieName := ah.generateCookieName(authCode.ClusterID)
-		s := securecookie.New(ah.cookieSigningKey, ah.cookieEncryptionKey)
-		encoded, err := s.Encode(cookieName, sessionState)
-		if err != nil {
-			logger.Error(err, "failed to encode secure session cookie")
-			http.Error(w, "internal error", http.StatusInternalServerError)
-			return
-		}
+		return
+	}
 
-		secure := false
-		http.SetCookie(w, session.MakeCookie(r, cookieName, encoded, secure, 1*time.Hour))
+	// UI flow - set cookie and redirect to UI
+	cookieName := ah.generateCookieName(authCode.ClusterID)
+	s := securecookie.New(ah.cookieSigningKey, ah.cookieEncryptionKey)
+	encoded, err := s.Encode(cookieName, sessionState)
+	if err != nil {
+		logger.Error(err, "failed to encode secure session cookie")
+		http.Error(w, "internal error", http.StatusInternalServerError)
+		return
+	}
 
-		clientParams := &client.ClientParameters{
-			ClusterID:             authCode.ClusterID,
-			ClientSideRedirectURL: authCode.ClientSideRedirectURL,
-			RedirectURL:           authCode.RedirectURL,
-			SessionID:             authCode.SessionID,
-		}
-		url := clientParams.WithParams(authCode.RedirectURL)
+	secure := false
+	http.SetCookie(w, session.MakeCookie(r, cookieName, encoded, secure, 1*time.Hour))
 
-		http.Redirect(w, r, url, http.StatusFound)
+	clientParams := &client.ClientParameters{
+		ClusterID:             authCode.ClusterID,
+		ClientSideRedirectURL: authCode.ClientSideRedirectURL,
+		RedirectURL:           authCode.RedirectURL,
+		SessionID:             authCode.SessionID,
 	}
+	url := clientParams.WithParams(authCode.RedirectURL)
+
+	http.Redirect(w, r, url, http.StatusFound)
 }
 
 func (ah *AuthHandler) respondWithError(w http.ResponseWriter, clientType ClientType, message string, statusCode int) {
-	if clientType == ClientTypeCLI {
-		w.Header().Set("Content-Type", "application/json")
-		w.WriteHeader(statusCode)
-		err := json.NewEncoder(w).Encode(AuthResponse{
-			Success: false,
-			Error:   message,
-		})
-		if err != nil {
-			http.Error(w, "internal error", http.StatusInternalServerError)
-		}
-	} else {
+	if clientType != ClientTypeCLI {
 		http.Error(w, message, statusCode)
+		return
+	}
+
+	w.Header().Set("Content-Type", "application/json")
+	w.WriteHeader(statusCode)
+	err := json.NewEncoder(w).Encode(AuthResponse{
+		Success: false,
+		Error:   message,
+	})
+	if err != nil {
+		http.Error(w, "internal error", http.StatusInternalServerError)
 	}
 }
 
diff --git a/backend/auth/types.go b/backend/auth/types.go
@@ -43,6 +43,9 @@ type TokenResponse struct {
 	ClusterID string `json:"cluster_id,omitempty"`
 }
 
+// TODO: We should remove client_side_redirect_url.
+// https://github.com/kube-bind/kube-bind/issues/362
+
 type AuthorizeRequest struct {
 	RedirectURL           string     `json:"redirect_url" form:"redirect_url"`
 	ClientSideRedirectURL string     `json:"client_side_redirect_url" form:"client_side_redirect_url"`
diff --git a/backend/http/handler.go b/backend/http/handler.go
@@ -130,8 +130,8 @@ func (h *handler) AddRoutes(mux *mux.Router) error {
 	apiRouter.Handle("/ping", auth.RequireAuth(http.HandlerFunc(h.handlePing))).Methods(http.MethodGet)
 
 	switch {
+	// Development mode: proxy to frontend dev server
 	case strings.HasPrefix(h.frontend, "http://"):
-		// Development mode: proxy to frontend dev server
 		spaserver, err := spaserver.NewSPAReverseProxyServer(h.frontend)
 		if err != nil {
 			return err
diff --git a/backend/server.go b/backend/server.go
@@ -124,8 +124,7 @@ func NewServer(ctx context.Context, c *Config) (*Server, error) {
 	if err != nil {
 		return nil, fmt.Errorf("error setting up HTTP Handler: %w", err)
 	}
-	err = handler.AddRoutes(s.WebServer.Router)
-	if err != nil {
+	if err := handler.AddRoutes(s.WebServer.Router); err != nil {
 		return nil, fmt.Errorf("error adding routes to HTTP Server: %w", err)
 	}
 
diff --git a/cli/pkg/client/client.go b/cli/pkg/client/client.go
@@ -34,13 +34,13 @@ import (
 )
 
 type Client interface {
-	GetTemplates(ctx context.Context) (*kubebindv1alpha2.APIServiceExportTemplateList, error)
-	GetCollections(ctx context.Context) (*kubebindv1alpha2.CollectionList, error)
-	Bind(ctx context.Context, request *kubebindv1alpha2.BindableResourcesRequest) (*kubebindv1alpha2.BindingResourceResponse, error)
+	GetTemplates(context.Context) (*kubebindv1alpha2.APIServiceExportTemplateList, error)
+	GetCollections(context.Context) (*kubebindv1alpha2.CollectionList, error)
+	Bind(context.Context, *kubebindv1alpha2.BindableResourcesRequest) (*kubebindv1alpha2.BindingResourceResponse, error)
 
-	Get(urlStr string) (*http.Response, error)
-	Post(urlStr string, body io.Reader) (*http.Response, error)
-	Do(req *http.Request) (*http.Response, error)
+	Get(string) (*http.Response, error)
+	Post(string, io.Reader) (*http.Response, error)
+	Do(*http.Request) (*http.Response, error)
 }
 
 // authenticatedClient provides an HTTP client with automatic authentication
@@ -152,8 +152,8 @@ func (c *authenticatedClient) Bind(ctx context.Context, request *kubebindv1alpha
 }
 
 // Get performs an authenticated GET request
-func (c *authenticatedClient) Get(urlStr string) (*http.Response, error) {
-	req, err := http.NewRequest("GET", urlStr, nil)
+func (c *authenticatedClient) Get(url string) (*http.Response, error) {
+	req, err := http.NewRequest("GET", url, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -166,8 +166,8 @@ func (c *authenticatedClient) Get(urlStr string) (*http.Response, error) {
 }
 
 // Post performs an authenticated POST request
-func (c *authenticatedClient) Post(urlStr string, body io.Reader) (*http.Response, error) {
-	req, err := http.NewRequest("POST", urlStr, body)
+func (c *authenticatedClient) Post(url string, body io.Reader) (*http.Response, error) {
+	req, err := http.NewRequest("POST", url, body)
 	if err != nil {
 		return nil, err
 	}
diff --git a/cli/pkg/kubectl/base/browser.go b/cli/pkg/kubectl/base/browser.go
@@ -45,10 +45,5 @@ func isWindows() bool {
 
 // isMacOS checks if the current OS is macOS
 func isMacOS() bool {
-	cmd := exec.Command("uname")
-	output, err := cmd.Output()
-	if err != nil {
-		return false
-	}
-	return strings.TrimSpace(string(output)) == "Darwin"
+	return strings.Contains(strings.ToLower(exec.Command("uname").String()), "darwin")
 }
diff --git a/cli/pkg/kubectl/base/options.go b/cli/pkg/kubectl/base/options.go
@@ -40,10 +40,10 @@ type Options struct {
 	Kubeconfig string
 	// KubectlOverrides stores the extra client connection fields, such as context, user, etc.
 	KubectlOverrides *clientcmd.ConfigOverrides
-	// Server is the kube-bind server name to use (overrides kube-bind config current server)
-	Server string
-	// Cluster is the kube-bind cluster name to use (overrides kube-bind config current cluster)
-	Cluster string
+	// ServerName is the kube-bind server name to use (overrides kube-bind config current server)
+	ServerName string
+	// ClusterName is the kube-bind cluster name to use (overrides kube-bind config current cluster)
+	ClusterName string
 	// SkipInsecure skips TLS verification (for development)
 	SkipInsecure bool
 	// ConfigFile is the path to the kube-bind configuration file
@@ -87,8 +87,8 @@ func (o *Options) BindFlags(cmd *cobra.Command) {
 	clientcmd.BindOverrideFlags(o.KubectlOverrides, cmd.PersistentFlags(), kubectlConfigOverrideFlags)
 
 	// Add common kube-bind flags
-	cmd.Flags().StringVar(&o.Server, "server", o.Server, "The kube-bind server name to use (overrides kube-bind config current server)")
-	cmd.Flags().StringVar(&o.Cluster, "cluster", o.Cluster, "The kube-bind cluster name to use (overrides kube-bind config current cluster)")
+	cmd.Flags().StringVar(&o.ServerName, "server", o.ServerName, "The kube-bind server name to use (overrides kube-bind config current server)")
+	cmd.Flags().StringVar(&o.ClusterName, "cluster", o.ClusterName, "The kube-bind cluster name to use (overrides kube-bind config current cluster)")
 	cmd.Flags().BoolVar(&o.SkipInsecure, "insecure-skip-tls-verify", false, "Skip TLS certificate verification (not recommended)")
 	cmd.Flags().StringVar(&o.ConfigFile, "config-file", "", "Path to the kube-bind configuration file")
 }
@@ -123,16 +123,16 @@ func (o *Options) Complete(skipValidate bool) error {
 	}
 
 	switch {
-	case o.Server != "" && o.Cluster != "":
+	case o.ServerName != "" && o.ClusterName != "":
 		// Both server and cluster specified separately - do nothing
-	case o.Server != "" && strings.Contains(o.Server, "@"):
+	case o.ServerName != "" && strings.Contains(o.ServerName, "@"):
 		// Server specified in server@cluster format
-		parts := strings.SplitN(o.Server, "@", 2)
-		o.Server = parts[0]
-		o.Cluster = parts[1]
-	case o.Server != "" && o.Cluster == "":
+		parts := strings.SplitN(o.ServerName, "@", 2)
+		o.ServerName = parts[0]
+		o.ClusterName = parts[1]
+	case o.ServerName != "" && o.ClusterName == "":
 		// Only server specified - do nothing. Cluster is empty
-	case o.Server == "":
+	case o.ServerName == "":
 		// No server specified - do nothing. Will be resolved from config
 	}
 
@@ -141,7 +141,7 @@ func (o *Options) Complete(skipValidate bool) error {
 	// So if its empty here, we need to resolve it.
 	var s *config.Server
 	var c *config.Config
-	if o.Server == "" {
+	if o.ServerName == "" {
 		c, err = config.LoadConfigFromFile(o.ConfigFile)
 		if err != nil {
 			return fmt.Errorf("failed to load config: %w", err)
@@ -159,11 +159,11 @@ func (o *Options) Complete(skipValidate bool) error {
 		}
 
 		var exists bool
-		s, exists = c.Get(o.Server, o.Cluster)
+		s, exists = c.Get(o.ServerName, o.ClusterName)
 		if !exists && !skipValidate {
-			err := fmt.Errorf("server %q not found in config", o.Server)
-			if o.Cluster != "" {
-				err = fmt.Errorf("server %q with cluster %q not found in config", o.Server, o.Cluster)
+			err := fmt.Errorf("server %q not found in config", o.ServerName)
+			if o.ClusterName != "" {
+				err = fmt.Errorf("server %q with cluster %q not found in config", o.ServerName, o.ClusterName)
 			}
 			return err
 		}
@@ -174,8 +174,8 @@ func (o *Options) Complete(skipValidate bool) error {
 	}
 
 	if s != nil {
-		o.Server = s.URL
-		o.Cluster = s.Cluster
+		o.ServerName = s.URL
+		o.ClusterName = s.Cluster
 	}
 	o.server = s
 	o.config = c
@@ -185,14 +185,14 @@ func (o *Options) Complete(skipValidate bool) error {
 
 // Validate validates the configured options.
 func (o *Options) Validate() error {
-	if o.Server != "" && strings.Contains(o.Server, "@") && o.Cluster != "" {
+	if o.ServerName != "" && strings.Contains(o.ServerName, "@") && o.ClusterName != "" {
 		return fmt.Errorf("cannot specify both server in 'server@cluster' format and --cluster flag")
 	}
 
-	if o.Cluster != "" && o.Server == "" {
+	if o.ClusterName != "" && o.ServerName == "" {
 		return fmt.Errorf("cannot specify --cluster without --server")
 	}
-	if _, err := url.Parse(o.Server); err != nil {
+	if _, err := url.Parse(o.ServerName); err != nil {
 		return fmt.Errorf("invalid server URL: %w", err)
 	}
 
diff --git a/cli/pkg/kubectl/bind-apiservice/plugin/bind.go b/cli/pkg/kubectl/bind-apiservice/plugin/bind.go
@@ -82,6 +82,7 @@ func (b *BindAPIServiceOptions) AddCmdFlags(cmd *cobra.Command) {
 	b.Print.AddFlags(cmd)
 
 	cmd.Flags().StringVar(&b.Template, "template-name", b.Template, "A template name to use for binding")
+	cmd.Flags().StringVar(&b.Name, "name", b.Name, "The name of the BindableResourcesRequest to create")
 	cmd.Flags().StringVar(&b.remoteKubeconfigFile, "remote-kubeconfig", b.remoteKubeconfigFile, "A file path for a kubeconfig file to connect to the service provider cluster")
 	cmd.Flags().StringVar(&b.remoteKubeconfigNamespace, "remote-kubeconfig-namespace", b.remoteKubeconfigNamespace, "The namespace of the remote kubeconfig secret to read from")
 	cmd.Flags().StringVar(&b.remoteKubeconfigName, "remote-kubeconfig-name", b.remoteKubeconfigNamespace, "The name of the remote kubeconfig secret to read from")
diff --git a/cli/pkg/kubectl/bind-apiservice/plugin/binder.go b/cli/pkg/kubectl/bind-apiservice/plugin/binder.go
@@ -63,6 +63,7 @@ func NewBinder(config *rest.Config, opts *BinderOptions) *Binder {
 
 // TODO: bindFromFile and bindFromResponse can likely share a lot of code. This slow is bit repetitive
 // but keeps the two paths separate for clarity. But it needs love.
+// https://github.com/kube-bind/kube-bind/issues/360
 
 func (b *Binder) BindFromFile(ctx context.Context) ([]*kubebindv1alpha2.APIServiceBinding, error) {
 	// Ensure client side namespace exists
diff --git a/cli/pkg/kubectl/bind-collections/plugin/collections.go b/cli/pkg/kubectl/bind-collections/plugin/collections.go
@@ -63,7 +63,7 @@ func (o *CollectionsOptions) AddCmdFlags(cmd *cobra.Command) {
 func (o *CollectionsOptions) Complete(args []string) error {
 	// Set this before complete base settings as login accepts server URL as argument without flag.
 	if len(args) > 0 {
-		o.Options.Server = args[0]
+		o.Options.ServerName = args[0]
 	}
 
 	if err := o.Options.Complete(false); err != nil {
diff --git a/cli/pkg/kubectl/bind-login/plugin/login.go b/cli/pkg/kubectl/bind-login/plugin/login.go
@@ -19,7 +19,6 @@ package plugin
 import (
 	"context"
 	"crypto/rand"
-	"encoding/base64"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -96,7 +95,7 @@ func (o *LoginOptions) AddCmdFlags(cmd *cobra.Command) {
 // Complete completes the options
 func (o *LoginOptions) Complete(args []string) error {
 	if len(args) > 0 {
-		o.Options.Server = strings.TrimSuffix(args[0], "/")
+		o.Options.ServerName = strings.TrimSuffix(args[0], "/")
 	}
 	err := o.Options.Complete(true)
 	if err != nil {
@@ -118,17 +117,14 @@ func (o *LoginOptions) Run(ctx context.Context, authURLCh chan<- string) error {
 	config := o.Options.GetConfig()
 
 	// Generate a random session ID for cli session to verify callback requests.
-	sessionID, err := generateRandomString(32)
-	if err != nil {
-		return fmt.Errorf("failed to generate session ID: %w", err)
-	}
+	sessionID := rand.Text()
 
 	// Setup callback server with random port
 	tokenCh := make(chan *TokenResponse, 1)
 	errCh := make(chan error, 1)
 
 	// Get provider information
-	fmt.Fprintf(o.Streams.ErrOut, "Connecting to kube-bind server %s...\n", o.Options.Server)
+	fmt.Fprintf(o.Streams.ErrOut, "Connecting to kube-bind server %s...\n", o.Options.ServerName)
 	provider, err := o.getProvider(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to get provider information: %w", err)
@@ -182,7 +178,7 @@ func (o *LoginOptions) Run(ctx context.Context, authURLCh chan<- string) error {
 		token.ExpiresAt = time.Now().Add(time.Duration(token.ExpiresIn) * time.Second)
 	}
 
-	serverHost, err := url.Parse(o.Options.Server)
+	serverHost, err := url.Parse(o.Options.ServerName)
 	if err != nil {
 		return fmt.Errorf("failed to parse server URL: %w", err)
 	}
@@ -234,7 +230,7 @@ func (o *LoginOptions) Run(ctx context.Context, authURLCh chan<- string) error {
 }
 
 func (o *LoginOptions) getProvider(ctx context.Context) (*kubebindv1alpha2.BindingProvider, error) {
-	url, err := url.Parse(o.Options.Server)
+	url, err := url.Parse(o.Options.ServerName)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse server URL: %w", err)
 	}
@@ -293,8 +289,8 @@ func (o *LoginOptions) buildAuthURL(provider *kubebindv1alpha2.BindingProvider,
 	values.Set("redirect_url", redirectURL)
 	values.Set("session_id", sessionID)
 	values.Set("client_type", "cli")
-	if o.Cluster != "" {
-		values.Set("cluster_id", o.Cluster)
+	if o.ClusterName != "" {
+		values.Set("cluster_id", o.ClusterName)
 	}
 	u.RawQuery = values.Encode()
 
@@ -376,11 +372,3 @@ func (o *LoginOptions) startCallbackServerWithRandomPort(tokenCh chan<- *TokenRe
 
 	return server, callbackURL, nil
 }
-
-func generateRandomString(length int) (string, error) {
-	bytes := make([]byte, length)
-	if _, err := rand.Read(bytes); err != nil {
-		return "", err
-	}
-	return base64.URLEncoding.EncodeToString(bytes)[:length], nil
-}
diff --git a/cli/pkg/kubectl/bind/plugin/bind.go b/cli/pkg/kubectl/bind/plugin/bind.go
diff --git a/contrib/kcp/README.md b/contrib/kcp/README.md
diff --git a/contrib/kcp/test/e2e/kcp_test.go b/contrib/kcp/test/e2e/kcp_test.go

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-# Copyright 2022 The Kube Bind Authors.`
	`1`	`+# Copyright 2025 The Kube Bind Authors.`
`2`	`2`	`#`
`3`	`3`	`# Licensed under the Apache License, Version 2.0 (the "License");`
`4`	`4`	`# you may not use this file except in compliance with the License.`
Original file line number	Diff line number	Diff line change
`@@ -43,6 +43,9 @@ type TokenResponse struct {`
`43`	`43`	ClusterID string `json:"cluster_id,omitempty"`
`44`	`44`	`}`
`45`	`45`
	`46`	`+// TODO: We should remove client_side_redirect_url.`
	`47`	`+// https://github.com/kube-bind/kube-bind/issues/362`
	`48`	`+`
`46`	`49`	`type AuthorizeRequest struct {`
`47`	`50`	RedirectURL string `json:"redirect_url" form:"redirect_url"`
`48`	`51`	ClientSideRedirectURL string `json:"client_side_redirect_url" form:"client_side_redirect_url"`
Original file line number	Diff line number	Diff line change
`@@ -124,8 +124,7 @@ func NewServer(ctx context.Context, c Config) (Server, error) {`
`124`	`124`	`if err != nil {`
`125`	`125`	`return nil, fmt.Errorf("error setting up HTTP Handler: %w", err)`
`126`	`126`	`}`
`127`		`- err = handler.AddRoutes(s.WebServer.Router)`
`128`		`- if err != nil {`
	`127`	`+ if err := handler.AddRoutes(s.WebServer.Router); err != nil {`
`129`	`128`	`return nil, fmt.Errorf("error adding routes to HTTP Server: %w", err)`
`130`	`129`	`}`
`131`	`130`