Skip to content
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.

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

Fix/same secrets #23

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Fix/same secrets #23

merged 4 commits into from
Nov 29, 2023

Conversation

hoptical
Copy link
Collaborator

@hoptical hoptical commented Nov 28, 2023

The operator would face panic when a s3userclaim with already existing secrets was created.
This PR:

  • Fixes the subrecociler behavior while facing an error on setting controller reference in ensuring secrets to prevent panics.
  • Creates validation webhooks on create/update events that deny requests with existing secrets.
  • Updates the e2e test phase

Summary by CodeRabbit

  • Chores

    • Updated .gitignore to exclude .vscode directory and clarify ignored items.
  • New Features

    • Implemented secret name validation for S3UserClaim creation and updates.
  • Bug Fixes

    • Improved error handling in secret provisioning to ensure proper requeuing.
  • Documentation

    • Standardized error messages for consistency in user communication.

@hoptical hoptical added the bug Something isn't working label Nov 28, 2023
@hoptical hoptical requested a review from therealak12 November 28, 2023 14:37
@hoptical hoptical self-assigned this Nov 28, 2023
Copy link
Contributor

coderabbitai bot commented Nov 28, 2023

Walkthrough

The changes involve updates to a codebase, likely for a Kubernetes-related project. A .gitignore file has been updated to exclude certain directories, and Go code has been modified to enhance validation for secret names within Kubernetes. Error handling in a provisioning function has been improved, and error messages have been refined for consistency and to introduce a new error condition.

Changes

File Path Change Summary
.gitignore Added .vscode to ignored list and provided explanatory comments.
api/.../s3userclaim_webhook.go Updated S3UserClaim validation methods and added validateSecrets function for secret name validation.
internal/.../provisioner.go Modified error handling to requeue on failure when setting controller reference.
pkg/consts/consts.go Changed S3UserRefNotFoundErrMessage for consistency and added SecretExistsErrMessage.

Poem

🐇✨
In the burrow of code, changes abound,
Secrets checked, errors sound.
.gitignore grows, with comments it's crowned,
A rabbit's work, in git, is now found. 🌟
🐇✨


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 5052540 and bbf439f.
Files ignored due to filter (3)
  • testing/e2e/04-validation-webhook.yaml
  • testing/e2e/create-s3user-with-existing-secret.yaml
  • testing/e2e/update-s3user-with-existsing-secret.yaml
Files selected for processing (4)
  • .gitignore (1 hunks)
  • api/v1alpha1/s3userclaim_webhook.go (3 hunks)
  • internal/controllers/s3userclaim/provisioner.go (1 hunks)
  • pkg/consts/consts.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • .gitignore
Additional comments: 7
api/v1alpha1/s3userclaim_webhook.go (4)
  • 242-259: The implementation of validateSecrets correctly handles context timeouts and errors. It's good to see that it differentiates between a secret not being found (which is acceptable) and other errors (which are appended to the error list). Also, when a secret is found, it correctly appends a forbidden error, indicating that the secret already exists.

  • 71-79: The update to the ValidateCreate method correctly includes the new secret validation logic, ensuring that secrets do not already exist when creating an S3UserClaim.

  • 100-116: The update to the ValidateUpdate method correctly includes the new secret validation logic, ensuring that updated secrets do not already exist in the cluster. It also correctly checks if the secret names have changed before validating them.

  • 238-239: The unchanged validateQuota function correctly handles context timeouts and errors, which is crucial for the reliability of the quota validation logic.

internal/controllers/s3userclaim/provisioner.go (1)
  • 327-334: The error handling in the ensureSecret function is correctly implemented to log errors and requeue the reconciliation process when setting the controller reference or updating the secret fails. This change improves the robustness of the operator by preventing it from crashing and allowing it to retry the operation.
pkg/consts/consts.go (2)
  • 28-28: The change in the error message "S3UserRefNotFoundErrMessage" to start with a lowercase letter is noted and appears to be a stylistic choice for consistency with other error messages.

  • 30-30: The addition of "SecretExistsErrMessage" is appropriate for the new validation logic that checks for existing secrets.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between bbf439f and 5a960a4.
Files ignored due to filter (1)
  • testing/e2e/04-validation-webhook.yaml
Files selected for processing (2)
  • api/v1alpha1/s3userclaim_webhook.go (3 hunks)
  • pkg/consts/consts.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • pkg/consts/consts.go
Additional comments: 3
api/v1alpha1/s3userclaim_webhook.go (3)
  • 74-75: The addition of secret name validation in the ValidateCreate method is correctly implemented and aligns with the summary provided.

  • 104-112: The update validation logic correctly checks for changes in secret names and validates them if necessary.

  • 242-258: The implementation of the validateSecrets function correctly checks for the existence of secrets and appends appropriate errors to the error list.

@hoptical hoptical merged commit 2aac745 into main Nov 29, 2023
3 checks passed
@hoptical hoptical deleted the fix/same-secrets branch November 29, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants