Skip to content
This repository was archived by the owner on Jun 11, 2025. It is now read-only.

fix: cleans up service binding dns records, when synced without#363

Merged
nxtcoder17 merged 1 commit into
release-v1.0.7from
fix/service-binding-dns-deletion
Aug 29, 2024
Merged

fix: cleans up service binding dns records, when synced without#363
nxtcoder17 merged 1 commit into
release-v1.0.7from
fix/service-binding-dns-deletion

Conversation

@nxtcoder17
Copy link
Copy Markdown
Member

@nxtcoder17 nxtcoder17 commented Aug 29, 2024

Resolves kloudlite/kloudlite#284

Summary by Sourcery

Fix the cleanup of DNS records for service bindings when the hostname is missing or the service IP is nil, and enhance logging in the DNS resolver for better debugging.

Bug Fixes:

  • Fix the cleanup of DNS records for service bindings when the hostname is not specified or the service IP is nil.

Enhancements:

  • Add debug logging for invalid DNS queries and local domain handling in the DNS resolver.
  • Improve logging for failed service binding lookups in the DNS resolver.

@nxtcoder17 nxtcoder17 self-assigned this Aug 29, 2024
@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented Aug 29, 2024

Reviewer's Guide by Sourcery

This pull request addresses the cleanup of service binding DNS records when synced without certain conditions. It modifies the behavior of service binding updates, improves DNS query handling, and adjusts logging configuration.

File-Level Changes

Change Details Files
Modify service binding update logic to handle empty hostnames and service IPs
  • Add a filter for account name and service binding spec hostname
  • Implement deletion of service binding when hostname or service IP is empty
  • Simplify upsert operation by using the filter directly
apps/console/internal/domain/service-binding.go
Enhance DNS server logging and error handling
  • Add debug logging for invalid DNS queries
  • Add debug logging for local domain queries
  • Improve error logging when failing to find service binding
apps/console/internal/app/dns-server.go
Update logging configuration in main application
  • Remove environment variable based debug log configuration
  • Use command-line flag for debug logging instead
apps/console/main.go

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Hey @nxtcoder17 - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

}

if svcb.Spec.ServiceIP == nil {
if svcb.Spec.ServiceIP == nil || svcb.Spec.Hostname == "" {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: Consider adding a comment explaining the rationale for deleting the service binding.

It would be helpful to clarify why we're deleting the service binding when either ServiceIP or Hostname is empty. This would improve code maintainability and make the intention clearer for future developers.

// Delete service binding if ServiceIP or Hostname is empty, as these are required fields
if svcb.Spec.ServiceIP == nil || svcb.Spec.Hostname == "" {
	if err := d.serviceBindingRepo.DeleteOne(ctx, filter); err != nil {
		if !errors.Is(err, repos.ErrNoDocuments) {
			return err
		}
	}
	return nil
}

if _, err := d.serviceBindingRepo.Upsert(ctx, filter, &entities.ServiceBinding{
	ServiceBinding: *svcb,
	AccountName:    ctx.AccountName,
	ClusterName:    opts.ClusterName,

@nxtcoder17 nxtcoder17 force-pushed the fix/service-binding-dns-deletion branch from a9a65e8 to f11f65d Compare August 29, 2024 10:19
@nxtcoder17 nxtcoder17 merged commit be8df5e into release-v1.0.7 Aug 29, 2024
@nxtcoder17 nxtcoder17 deleted the fix/service-binding-dns-deletion branch August 29, 2024 10:31
abdheshnayak pushed a commit that referenced this pull request Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API] console API must be able to differentiate b/w reserved and unreserved service bindings, and delete DNS records for unreserved ones

1 participant