Preserve external controller state during reconciliation

[ciarams87]

Proposed changes

Problem: NGINX Gateway Fabric's provisioner removes all annotations during service reconciliation that aren't explicitly managed by NGF. This causes conflicts with external controllers that need to add operational annotations.

Solution: Implemented a preservation pattern for both service annotations by merging Service annotations instead of overwriting.

Note: This necessitates the use of a new internal annotation gateway.nginx.org/internal-managed-annotation-keys to track the user managed annotation keys

Testing: Unit testing, manual testing covering the following scenarios:

• External annotation added and preserved throughout all scenarios, and remains removed when deleted
• User added annotations using both Gateway infrastructure field and patches are preserved, take precedence, and are deleted from the Service when removed from the Gateway or NginxProxy

Closes #4012
Closes #4175

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto main
• I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

Preserve external controller annotations.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.07%. Comparing base (3562cc0) to head (0e6da21).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4182      +/-   ##
==========================================
+ Coverage   86.04%   86.07%   +0.03%     
==========================================
  Files         131      131              
  Lines       14142    14167      +25     
  Branches       35       35              
==========================================
+ Hits        12168    12194      +26     
+ Misses       1772     1771       -1     
  Partials      202      202              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sjberman]

@ciarams87 Do we really need a new API field for preserving annotations? Going off of this comment, could we essentially ignore or automatically preserve any existing fields set in the metadata.annotations/labels? And only add what we need, instead of overwriting?

Maybe this requires a CreateOrPatch instead of CreateOrUpdate that we currently use?

[ciarams87]

@ciarams87 Do we really need a new API field for preserving annotations? Going off of this comment, could we essentially ignore or automatically preserve any existing fields set in the metadata.annotations/labels? And only add what we need, instead of overwriting?

Maybe this requires a CreateOrPatch instead of CreateOrUpdate that we currently use?

@sjberman You're right, I was over complicating it.

I'll update the implementation

[bjee19]

Does there still exist a way to remove annotations if we start with all existing annotations then overwrite with NGF-managed annotations?

[ciarams87]

Does there still exist a way to remove annotations if we start with all existing annotations then overwrite with NGF-managed annotations?

@bjee19 Great question - you're right - there's a limitation: if a user sets an annotation via Gateway.Spec.Infrastructure.Annotations or NginxProxy patches and later removes it from the Gateway spec, it won't be automatically removed from the Service. It can still be removed by manually deleting it, but it won't be cleaned up automatically.

The proper fix is to migrate to Server-Side Apply, which tracks field ownership automatically. I think migrating to SSA is too large a change for a patch release since it affects how we reconcile all resources, so I think we should go with the approach and accept the limitation for now, but I'm going to create a follow up ticket to move to SSA instead. What do you think?

[sjberman]

@ciarams87 How large of a change do you think it really is? We'd technically be introducing a new bug, where a user removes some configuration, but then we don't actually delete it.

[ciarams87]

How large of a change do you think it really is? We'd technically be introducing a new bug, where a user removes some configuration, but then we don't actually delete it.

@sjberman Yeah that's fair - what I'll do is I'll see if I can just modify the service creation to use SSA first, and if so I'll open a ticket to investigate if we should do this for all resources later

This proved to be far too complex and requires tracking owners for managed fields - instead I'll keep the current approach but create a new "internal" annotation to track user configured annotations that should be removed from the service when they are no longer specified by the user