update deletion logic + adjust unit tests for this

- delete RRSet if challengeKey is the only record, or there are no records
- remove the record if theres more records
- do nothing if challengeKey is not existing in the RRSet

Author: Niklas Burchhardt

internal/resolver/resolver.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/http"
 	"os"
+	"slices"
 	"strings"
 
 	"github.com/cert-manager/cert-manager/pkg/acme/webhook"
@@ -97,7 +98,7 @@ func (s *stackitDnsProviderResolver) CleanUp(ch *v1alpha1.ChallengeRequest) erro
 		return s.handleErrorDuringInitialization(err)
 	}
 
-	return s.handleRRSetCleanup(initResolverRes)
+	return s.handleRRSetCleanup(initResolverRes, ch.Key)
 }
 
 // Initialize will be called when the webhook first starts.
@@ -287,6 +288,7 @@ func (s *stackitDnsProviderResolver) handleErrorDuringInitialization(
 
 func (s *stackitDnsProviderResolver) handleRRSetCleanup(
 	initResolverRes *initResolverContextResult,
+	challengeKey string,
 ) error {
 	s.logger.Info("Cleaning up RRSet", zap.String("rrSetName", initResolverRes.rrSetName))
 
@@ -299,7 +301,27 @@ func (s *stackitDnsProviderResolver) handleRRSetCleanup(
 		return s.handleFetchRRSetError(err, initResolverRes.rrSetName)
 	}
 
-	return s.deleteRRSet(initResolverRes.rrSetRepository, rrSet, initResolverRes.rrSetName)
+	if rrSet == nil || rrSet.Records == nil || len(*rrSet.Records) == 0 {
+		return s.deleteRRSet(initResolverRes.rrSetRepository, rrSet, initResolverRes.rrSetName)
+	}
+
+	originalLen := len(*rrSet.Records)
+
+	*rrSet.Records = slices.DeleteFunc(*rrSet.Records, func(r stackitdnsclient.Record) bool {
+		return r.Content != nil && *r.Content == challengeKey
+	})
+
+	if len(*rrSet.Records) == originalLen {
+		s.logger.Info("Challenge key not found in RRSet records, nothing to clean up", zap.String("rrSetName", initResolverRes.rrSetName))
+
+		return nil
+	}
+
+	if len(*rrSet.Records) == 0 {
+		return s.deleteRRSet(initResolverRes.rrSetRepository, rrSet, initResolverRes.rrSetName)
+	}
+
+	return initResolverRes.rrSetRepository.UpdateRRSet(s.ctx, *rrSet)
 }
 
 func (s *stackitDnsProviderResolver) handleFetchRRSetError(err error, rrSetName string) error {

internal/resolver/resolver_test.go

@@ -27,6 +27,11 @@ var (
 	}
 )
 
+const (
+	targetKey = "delete-me"
+	keepKey   = "keep-me"
+)
+
 func TestName(t *testing.T) {
 	t.Parallel()
 
@@ -601,56 +606,89 @@ func (s *cleanSuite) TestFailFetchNoRRSet() {
 	s.NoError(err)
 }
 
-func (s *cleanSuite) TestFailDeleteNoRRSet() {
+func (s *cleanSuite) TestCleanUp_RemovesOnlyKey_DeletesRRSet() {
 	s.setupCommonMocks()
+
+	req := &v1alpha1.ChallengeRequest{
+		Config: configJson,
+		Key:    targetKey,
+	}
+
 	rrset := stackitdnsclient_new.RecordSet{
 		Id: toPtr("1234"),
+		Records: &[]stackitdnsclient_new.Record{
+			{Content: toPtr(targetKey)},
+		},
 	}
+
 	s.mockRRSetRepository.EXPECT().
 		FetchRRSetForZone(gomock.Any(), gomock.Any(), gomock.Any()).
 		Return(&rrset, nil)
+
+	// Because it was the only key, the slice becomes empty, so we expect a DeleteRRSet
 	s.mockRRSetRepository.EXPECT().
 		DeleteRRSet(gomock.Any(), *rrset.Id).
-		Return(repository.ErrRRSetNotFound)
+		Return(nil)
 
-	err := s.resolver.CleanUp(challengeRequest)
+	err := s.resolver.CleanUp(req)
 	s.NoError(err)
 }
 
-func (s *cleanSuite) TestFailDeleteRRSet() {
+func (s *cleanSuite) TestCleanUp_RemovesOneKey_UpdatesRRSet() {
 	s.setupCommonMocks()
+
+	req := &v1alpha1.ChallengeRequest{
+		Config: configJson,
+		Key:    targetKey,
+	}
+
 	rrset := stackitdnsclient_new.RecordSet{
 		Id: toPtr("1234"),
+		Records: &[]stackitdnsclient_new.Record{
+			{Content: toPtr(targetKey)},
+			{Content: toPtr(keepKey)},
+		},
 	}
+
 	s.mockRRSetRepository.EXPECT().
 		FetchRRSetForZone(gomock.Any(), gomock.Any(), gomock.Any()).
 		Return(&rrset, nil)
+
+	// Because one key remains, we expect an UpdateRRSet, NOT a DeleteRRSet
 	s.mockRRSetRepository.EXPECT().
-		DeleteRRSet(gomock.Any(), *rrset.Id).
-		Return(fmt.Errorf("error deleting rr set"))
+		UpdateRRSet(gomock.Any(), matchedBy(func(updated stackitdnsclient_new.RecordSet) bool {
+			return updated.Records != nil &&
+				len(*updated.Records) == 1 &&
+				*(*updated.Records)[0].Content == keepKey
+		})).
+		Return(nil)
 
-	err := s.resolver.CleanUp(challengeRequest)
-	s.Error(err)
-	s.Containsf(
-		err.Error(),
-		"error deleting rr set",
-		"error message should contain error from rrSetRepository",
-	)
+	err := s.resolver.CleanUp(req)
+	s.NoError(err)
 }
 
-func (s *cleanSuite) TestSuccessDeleteRRSet() {
+func (s *cleanSuite) TestCleanUp_KeyNotFound_DoesNothing() {
 	s.setupCommonMocks()
+
+	req := &v1alpha1.ChallengeRequest{
+		Config: configJson,
+		Key:    targetKey,
+	}
+
 	rrset := stackitdnsclient_new.RecordSet{
 		Id: toPtr("1234"),
+		Records: &[]stackitdnsclient_new.Record{
+			{Content: toPtr(keepKey)},
+		},
 	}
+
 	s.mockRRSetRepository.EXPECT().
 		FetchRRSetForZone(gomock.Any(), gomock.Any(), gomock.Any()).
 		Return(&rrset, nil)
-	s.mockRRSetRepository.EXPECT().
-		DeleteRRSet(gomock.Any(), *rrset.Id).
-		Return(nil)
 
-	err := s.resolver.CleanUp(challengeRequest)
+	// We do NOT expect DeleteRRSet or UpdateRRSet to be called.
+
+	err := s.resolver.CleanUp(req)
 	s.NoError(err)
 }