feat: k8s auth token instead of kube conffig #600
Conversation
WalkthroughThe pull request introduces a comprehensive enhancement to Kubernetes client authentication in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
pkg/k8s/utils.go (1)
43-45: Improve error specificity ingetClusterConfigWithTokenCurrently, when any of
opts.clusterHost,opts.authToken, oropts.certis empty, the function returns a genericErrEmptyClusterHostOrAuthTokenOrCerterror. To enhance clarity, consider specifying which parameter is missing in the error message.Apply this diff to improve error messages:
func getClusterConfigWithToken(opts *ClientOptions) (*rest.Config, error) { - if opts.clusterHost == "" || opts.authToken == "" || opts.cert == "" { - return nil, ErrEmptyClusterHostOrAuthTokenOrCert + var missingFields []string + if opts.clusterHost == "" { + missingFields = append(missingFields, "clusterHost") + } + if opts.authToken == "" { + missingFields = append(missingFields, "authToken") + } + if opts.cert == "" { + missingFields = append(missingFields, "cert") + } + if len(missingFields) > 0 { + return nil, fmt.Errorf("missing required fields: %v", missingFields) }scripts/auth_token.sh (2)
1-6: Add prerequisite checks and documentationThe script needs better documentation and validation of prerequisites.
Add:
#!/bin/bash +set -euo pipefail + # This script generates a bearer token for the knuu service account. # It is used to authenticate the knuu service account to the Kubernetes API server. # The token is used to test things manually. # It requries ~/.kube/config to be set up. + +# Verify prerequisites +command -v kubectl >/dev/null 2>&1 || { echo "kubectl is required but not installed. Aborting." >&2; exit 1; } + +# Verify current context +CURRENT_CONTEXT=$(kubectl config current-context) +echo "Using Kubernetes context: $CURRENT_CONTEXT" +read -p "Continue? (y/N) " -n 1 -r +echo +if [[ ! $REPLY =~ ^[Yy]$ ]]; then + exit 1 +fi
61-62: Improve example usage documentationThe example command should be more comprehensive.
Add more detailed examples and common use cases:
# Example how to run the tests using the auth token: -# . ./scripts/auth_token.sh && go test -v ./e2e/basic/ +# Generate token and run tests: +# source ./scripts/auth_token.sh && go test -v ./e2e/basic/ +# +# Export variables for later use: +# source ./scripts/auth_token.sh +# export | grep K8S_ # verify exported variables +# +# Use with kubectl: +# source ./scripts/auth_token.sh +# kubectl --token="$K8S_AUTH_TOKEN" --server="$K8S_HOST" get podspkg/k8s/clinet_options.go (2)
5-11: Add field documentation and consider adding getter methods.The struct fields are correctly made private, but consider:
- Adding documentation for each field to explain their purpose and requirements
- Implementing getter methods if these values need to be accessed after initialization
Example documentation:
type ClientOptions struct { + // clusterDomain is the domain name of the Kubernetes cluster clusterDomain string + // clusterHost is the host address of the Kubernetes API server clusterHost string + // authToken is the authentication token for API server access authToken string + // cert is the certificate data for TLS verification cert string + // clusterConfig is the raw rest.Config if direct configuration is needed clusterConfig *rest.Config }
5-11: Document security requirements for auth tokens.Since this code handles authentication tokens, please add documentation about:
- Required token format and minimum length
- Token security requirements (e.g., entropy, expiration)
- Safe token handling practices
Example documentation:
// ClientOptions holds Kubernetes client configuration. // Security Notice: // - Auth tokens must be kept secure and not logged // - Tokens should be rotated regularly // - Minimum token length: 32 characters // - Token should have sufficient entropy type ClientOptions struct { // ... existing fields ... }Also applies to: 21-27
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e/basic/suite_setup_test.go(2 hunks)e2e/netshaper/suite_setup_test.go(2 hunks)e2e/sidecars/suite_setup_test.go(2 hunks)e2e/suite.go(3 hunks)e2e/system/suite_setup_test.go(2 hunks)e2e/tshark/tshark_test.go(2 hunks)pkg/k8s/clinet_options.go(1 hunks)pkg/k8s/errors.go(1 hunks)pkg/k8s/k8s.go(6 hunks)pkg/k8s/namespace.go(2 hunks)pkg/k8s/pod.go(3 hunks)pkg/k8s/utils.go(1 hunks)scripts/auth_token.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (./e2e/netshaper, 60m)
- GitHub Check: test (./e2e/basic, 15m)
🔇 Additional comments (18)
e2e/netshaper/suite_setup_test.go (3)
24-27: Good practice in grouping variable declarationsGrouping the variable declarations for
ctxandloggerenhances readability and maintains clean code structure.
29-30: Proper error handling after Kubernetes client creationThe error from
k8s.NewClientis correctly handled usings.Require().NoError(err), ensuring the test suite fails if the client cannot be created.
34-35: Enhanced configurability with new optionsPassing
K8sClientandLoggertoknuu.Newimproves the configurability and observability of theKnuuinstance.e2e/basic/suite_setup_test.go (1)
38-38: LGTM!The changes correctly integrate the new K8s auth token functionality and logger configuration.
Also applies to: 49-49
e2e/system/suite_setup_test.go (1)
Line range hint
39-50: LGTM! Good error messages.The changes correctly integrate the new K8s auth token functionality. The descriptive error messages in
s.Require().NoError()calls will help with debugging.e2e/sidecars/suite_setup_test.go (1)
37-43: LGTM!The changes correctly integrate the new K8s auth token functionality with clean error handling.
e2e/tshark/tshark_test.go (3)
16-16: LGTM: Import for e2e package added.The import aligns with the new e2e.Suite integration.
36-40: LGTM: Clean variable block initialization.Good practice to group related variable declarations in a single var block.
42-42: LGTM: Updated k8s client initialization with auth token support.The change correctly integrates the new authentication method via
K8sDefaultOptions.pkg/k8s/k8s.go (4)
12-12: LGTM: Added rest package import.Required for handling cluster configuration.
44-44: LGTM: Added clusterConfig field to Client struct.Good design to store the cluster configuration for reuse, avoiding repeated config retrieval.
Line range hint
57-82: LGTM: Enhanced NewClient implementation.The changes correctly:
- Use getAppliedOptions for configuration
- Set custom QPS and Burst
- Pass cluster config to NewClientCustom
Line range hint
94-110: LGTM: Updated NewClientCustom implementation.The changes properly integrate cluster configuration while maintaining existing functionality.
pkg/k8s/errors.go (1)
150-151: LGTM: Added comprehensive error handling for cluster configuration.The new error variables provide clear messages for configuration-related issues:
ErrEmptyClusterHostOrAuthTokenOrCertfor missing credentialsErrClusterConfigNotSetfor missing configurationpkg/k8s/pod.go (2)
213-214: LGTM: Added cluster config validation in RunCommandInPod.Good defensive programming to check for cluster config before creating the SPDY executor.
Also applies to: 216-216
285-286: LGTM: Added cluster config validation in PortForwardPod.Consistent error handling with RunCommandInPod for missing cluster config.
Also applies to: 296-296
pkg/k8s/clinet_options.go (2)
1-3: LGTM! Clean package structure.The package name and import are appropriate for Kubernetes client configuration.
35-43:⚠️ Potential issueFix undefined constant and consider function visibility.
- The
defaultClusterDomainconstant is undefined in this file- Consider if this function should be exported if it's needed by other packages
Let's verify the constant definition:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/suite.go (1)
27-29: Add documentation for the new environment variable constants.Consider adding documentation comments to explain the purpose and expected values of these environment variables.
+// envK8sHost is the environment variable name for the Kubernetes API server host envK8sHost = "K8S_HOST" +// envK8sCACert is the environment variable name for the Kubernetes CA certificate envK8sCACert = "K8S_CA_CERT" +// envK8sAuthToken is the environment variable name for the Kubernetes authentication token envK8sAuthToken = "K8S_AUTH_TOKEN"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/suite.go(4 hunks)pkg/k8s/namespace.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/k8s/namespace.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (./e2e/netshaper, 60m)
🔇 Additional comments (1)
e2e/suite.go (1)
103-116: Implementation looks good and includes proper validation!The method properly validates all required environment variables and provides a clear fallback mechanism to the default kubeconfig. The use of constants and descriptive logging enhances maintainability.
Let's verify that this method is consistently used across the codebase:
✅ Verification successful
K8sDefaultOptions is consistently used across the codebase
All e2e test files properly initialize the k8s client using
s.K8sDefaultOptions(), and there are no direct environment variable accesses outside of the Suite implementation. The implementation is being used as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of K8sDefaultOptions across test files # Find all test setup files that might need K8s client initialization echo "Checking test files for K8s client initialization..." rg -l "k8s\.NewClient" --type go # Check for any direct usage of these environment variables outside of K8sDefaultOptions echo -e "\nChecking for direct environment variable usage..." rg -l "K8S_(HOST|CA_CERT|AUTH_TOKEN)" --type go | grep -v "suite.go"Length of output: 600
Script:
#!/bin/bash # Check how k8s.NewClient is being used in e2e tests echo "Checking k8s.NewClient usage in e2e tests..." rg -B 3 -A 3 "k8s\.NewClient" e2e/ # Check if test files are using the Suite type and its methods echo -e "\nChecking Suite usage in test files..." rg -B 2 -A 2 "type \*Suite" e2e/Length of output: 2389
Closes #319
Summary by CodeRabbit
New Features
Bug Fixes
Chores