fix: Look for secret in namespace of wandb CR

[danielpanzella]

• Refactor GetLicense to use the k8s client from the controller
• Use the namespace of the wandb resource for secret location
• Add tests for retreiving license from secret

Summary by CodeRabbit

• New Features

  • Introduced enhanced validation for execution environments—ensuring that only approved contexts are accepted for deployment.
  • Improved license retrieval process by integrating Kubernetes context and client information.

• Chores

  • Streamlined the license verification process with enhanced logging and context-based operations.

• Tests

  • Expanded test coverage to verify environment checks and license handling, including scenarios for missing license secrets and successful retrievals.

[coderabbitai]

Walkthrough

This pull request adds a validation in the Tiltfile to check that the Kubernetes context is among an allowed list. It also updates the license retrieval functionality across the codebase by modifying the signature of the GetLicense function to accept additional parameters (context and Kubernetes client), along with corresponding updates in its usage, logging, and secret retrieval logic. Furthermore, the test suites have been enhanced with a proper Kubernetes test environment setup to support these changes.

Changes

File(s)	Change Summary
Tiltfile	Added a conditional check to validate the Kubernetes context against an allowed list, printing a confirmation if valid or failing if not.
controllers/weightsandbiases_controller.go pkg/wandb/spec/utils/license.go pkg/wandb/spec/utils/license_test.go	Updated the GetLicense function signature to include context.Context and client.Client; modified internal logging and secret retrieval logic; adjusted tests to include new parameters and improved scenarios covering license secret retrieval.
pkg/wandb/spec/utils/utils_suite_test.go	Introduced a Kubernetes test environment setup with BeforeSuite and AfterSuite functions, including global variables for configuration, client, and environment, to properly initialize and tear down test environments.

Sequence Diagram(s)

sequenceDiagram
    participant T as Tiltfile Script
    participant K as k8s_context()
    participant S as Allowed Contexts
    participant O as Output (Print/Fail)
    T->>K: Retrieve current Kubernetes context
    T->>S: Get list of allowed contexts
    alt Context is allowed
        T->>O: Print confirmation message
    else Context not allowed
        T->>O: Call fail() with error message
    end

Loading

sequenceDiagram
    participant C as Controller (Reconcile)
    participant G as GetLicense()
    participant GS as getLicenseFromSecret()
    participant K as Kubernetes API
    C->>G: Call GetLicense(ctx, client, wandb, crdSpec, userInputSpec)
    alt License sourced via secret
        G->>GS: Request license from secret using k8sClient
        GS->>K: Query Kubernetes Secret
        K-->>GS: Return secret value (or empty)
        GS-->>G: Return retrieved license
    else License provided via values
        G->>G: Process license provided via spec values
    end
    G-->>C: Return license information

Loading

Possibly related PRs

• feat: Updated license.go file to include the feature for licenseSecret as well #54: Enhancements similar to the current changes with updates to the GetLicense function, including logging improvements and additional error handling.
• fix: Add Tilt configs for local development #53: Related modifications focusing on Tilt configurations, specifically the introduction of allowed contexts in the tilt-settings.sample.json file.

Suggested reviewers

• zacharyblasczyk

Poem

In the meadow of code I hop with glee,
Checking contexts as safe as can be.
Licenses now pass a refined new gate,
With secrets unveiled, oh ain't that great?
I thump my paws, a rabbit so true,
Celebrating changes fresh and new!
🐇✨

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ea28c0 and d9c9a40.

📒 Files selected for processing (2)

• pkg/wandb/spec/utils/license.go (3 hunks)
• pkg/wandb/spec/utils/license_test.go (3 hunks)

🧰 Additional context used

🧬 Code Definitions (1)

pkg/wandb/spec/utils/license_test.go (4)

pkg/wandb/spec/utils/license.go (1)

• GetLicense (14-49)

pkg/wandb/spec/utils/utils_suite_test.go (2)

• k8sClient (19-19)
• err (36-36)

pkg/wandb/spec/state/secrets/secrets_suite_test.go (2)

• k8sClient (25-25)
• err (43-43)

pkg/wandb/spec/spec.go (1)

• Spec (44-56)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: Test

🔇 Additional comments (10)

pkg/wandb/spec/utils/license.go (5)

14-15: Function signature improvements enhance testability and dependency management

The updated function signature with explicit dependencies (context, k8sClient, wandb object) follows good dependency injection principles. Context-based logging with FromContext ensures consistent and traceable logging throughout the call chain.

---

38-38: Proper namespace usage for secret retrieval

Getting the namespace directly from the wandb object ensures that the secret is retrieved from the correct namespace associated with the resource, which aligns with Kubernetes best practices.

---

41-41: Context properly propagated to helper function

The context is correctly passed to the helper function, allowing for proper cancellation propagation and consistent logging through the call chain.

---

51-52: Helper function signature properly updated

The helper function signature has been updated to accept context and use context-based logging, maintaining consistency with the parent function.

---

55-55: Previously identified issue has been fixed

The call now properly uses the passed context (ctx) instead of context.Background(), ensuring that cancellation signals are properly propagated from the parent context.

pkg/wandb/spec/utils/license_test.go (5)

16-16: Improved test description clarity

Updating the description from "when the license is set" to "when the license is set via values" better reflects the specific test case and aligns with the function's behavior.

---

25-31: Proper test setup with WeightsAndBiases object

The test correctly creates a WeightsAndBiases object with appropriate metadata and passes it to the GetLicense function along with the context and Kubernetes client, ensuring that the function is tested with realistic parameters.

---

34-54: Comprehensive test for missing secret scenario

This new test case verifies that the function correctly handles the scenario when a license secret is specified but doesn't exist in Kubernetes, which is important for ensuring robust error handling.

---

55-87: Thorough test for secret retrieval functionality

This test properly creates a Kubernetes secret, verifies that GetLicense retrieves the license from it, and then cleans up by deleting the secret. This ensures the end-to-end secret retrieval functionality works as expected.

---

96-102: Updated existing test with new function signature

The existing test for when license is not set has been properly updated to use the new function signature, maintaining complete test coverage.

✨ Finishing Touches

• 📝 Generate Docstrings

---

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/wandb/spec/utils/license_test.go (1)

55-85: Added comprehensive test for license retrieval from secret.

This test creates a Kubernetes secret with license data, then verifies that the GetLicense function correctly retrieves it. The test properly handles secret creation and validation.

However, I don't see cleanup for the created secret resource after the test. Consider adding a cleanup step:

+ BeforeEach(func() {
+   // Delete any existing test secrets
+   secret := &corev1.Secret{}
+   err := k8sClient.DeleteAllOf(context.Background(), secret, 
+       client.InNamespace("default"), 
+       client.MatchingFields{"metadata.name": "test-secret-name"})
+   // Ignore not found errors
+   Expect(client.IgnoreNotFound(err)).NotTo(HaveOccurred())
+ })
+
+ AfterEach(func() {
+   // Clean up created resources
+   secret := &corev1.Secret{}
+   err := k8sClient.DeleteAllOf(context.Background(), secret, 
+       client.InNamespace("default"), 
+       client.MatchingFields{"metadata.name": "test-secret-name"})
+   Expect(client.IgnoreNotFound(err)).NotTo(HaveOccurred())
+ })

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a4bbf3 and 0ea28c0.

📒 Files selected for processing (5)

• Tiltfile (1 hunks)
• controllers/weightsandbiases_controller.go (1 hunks)
• pkg/wandb/spec/utils/license.go (3 hunks)
• pkg/wandb/spec/utils/license_test.go (3 hunks)
• pkg/wandb/spec/utils/utils_suite_test.go (1 hunks)

🧰 Additional context used

🧬 Code Definitions (1)

pkg/wandb/spec/utils/utils_suite_test.go (5)

pkg/wandb/spec/state/secrets/manager.go (1) (1)

• New (12-24)

pkg/wandb/spec/state/versioning.go (1) (1)

• New (20-34)

pkg/wandb/spec/state/secrets/secrets_suite_test.go (4) (4)

• testEnv (26-26)
• cfg (24-24)
• err (43-43)
• k8sClient (25-25)

main.go (1) (1)

• scheme (48-48)

pkg/wandb/spec/channel/deployer/deployer_test.go (1) (1)

• client (30-30)

⏰ Context from checks skipped due to timeout of 90000ms (2)

• GitHub Check: Test
• GitHub Check: build-and-scan

🔇 Additional comments (16)

Tiltfile (1)

22-25: Good validation for Kubernetes context safety.

This adds a safeguard to prevent accidentally deploying to unintended environments by checking if the current context is in the allowed list. This is a valuable safety measure, especially in environments where developers might have multiple contexts configured.

controllers/weightsandbiases_controller.go (1)

143-143: Correctly updated function call to match new signature.

The GetLicense call has been updated to include the context, K8s client, and the wandb resource itself. This aligns with the changes in the implementation and ensures the license is retrieved from the correct namespace.

pkg/wandb/spec/utils/utils_suite_test.go (4)

3-16: Good addition of necessary imports for K8s testing.

These imports support the new Kubernetes testing environment setup, which is essential for properly testing the modified license functionality.

---

18-20: Properly declared global variables for K8s testing.

These variables establish the foundation for the Kubernetes test environment, client, and configuration - necessary components for testing Kubernetes-dependent functionality.

---

27-51: Well-structured test environment setup.

The BeforeSuite function properly initializes the test environment with appropriate error checking at each step. This ensures tests have a valid Kubernetes environment to interact with.

---

53-57: Clean test environment teardown.

The AfterSuite function ensures proper cleanup of test resources, which is important to prevent resource leaks during testing.

pkg/wandb/spec/utils/license.go (4)

14-15: Improved function signature with necessary context and client parameters.

The function now accepts the required parameters (context, K8s client, wandb object) to correctly retrieve license information, and uses a context-aware logger. This is more aligned with Kubernetes controller patterns.

---

38-38: Fixed namespace retrieval to use wandb resource namespace.

This is the core fix that addresses the PR objective - now getting the namespace directly from the wandb object rather than from spec values. This ensures the secret is always looked up in the correct namespace.

---

41-41: Updated secret retrieval with the passed client.

Now using the provided Kubernetes client instead of creating a new one, which is more efficient and aligned with controller patterns.

---

51-51: Updated helper function signature to use consistent parameter naming.

Changed parameter name from kubeClient to k8sClient for consistency with the calling function.

pkg/wandb/spec/utils/license_test.go (6)

4-12: Added required imports for enhanced license functionality.

The added imports are necessary to support the expanded functionality of the GetLicense function, which now requires a Kubernetes context, client, and type definitions for working with Kubernetes resources.

---

16-16: Context description updated for clarity.

Changing from "when the license is set" to "when the license is set via values" provides better specificity about the test scenario, clearly differentiating it from the new secret-based approach.

---

25-31: Updated test for license from values with new function signature.

The test now correctly uses the expanded function signature, passing the required context, Kubernetes client, and WeightsAndBiases object with namespace information to match the updated implementation.

---

34-54: Good addition of test for missing license secret.

This test case verifies the behavior when a license secret is referenced but doesn't exist in the cluster, ensuring the function gracefully returns an empty string rather than failing.

---

94-100: Updated test for no license with new function signature.

The test for when no license is set has been correctly updated to use the new function signature with context, client, and WeightsAndBiases object.

---

1-104:

Details

❌ Incorrect review comment

Missing k8sClient setup in test file.

The tests use a global k8sClient variable, but I don't see where it's defined or initialized in this file. Ensure that the test setup properly initializes the Kubernetes client, possibly in a BeforeSuite block or by importing from a test suite helper.

---

🏁 Script executed:

#!/bin/bash
# Search for k8sClient setup in test files
echo "Searching for k8sClient setup in test files..."
rg -A 5 "var\s+k8sClient" --type go

Length of output: 1245

---

Review Update: k8sClient Initialization Confirmed

After verifying the codebase, it’s clear that the global k8sClient is defined and initialized in the test suite file pkg/wandb/spec/utils/utils_suite_test.go (and similarly in other suite test files). There is no need to add any additional setup in this test file. Please ensure you're running the tests within the full suite context where the k8sClient is correctly established.

Likely an incorrect or invalid review comment.

[zacharyblasczyk]

This looks a little odd, but I don't think there is a better solution. Maybe there is a basedir?

[jsbroks]

This PR is included in version 1.19.3 🎉