Validate networking annotations on RevisionTemplateSpec

[linkvt]

Fixes #13053

Proposed Changes

• Add validation in RevisionTemplateSpec.Validate() to reject unknown networking.knative.dev/* annotations early at Service creation time
• Reuse existing networking.ValidateAnnotations() from the networking package for consistency with SKS validation
• Add test cases for invalid and valid networking annotations on RevisionTemplate

Background

When users mistakenly place networking.knative.dev/visibility: cluster-local as an annotation on spec.template.metadata.annotations (instead of as a label on the Service's metadata.labels), the annotation propagates through Revision → PodAutoscaler → ServerlessService, where SKS validation rejects it. This caused services to silently get stuck in "Unknown" state with no clear error message.

With this fix, users now get an immediate validation error:

validation failed: invalid key name "networking.knative.dev/visibility": spec.template.metadata.annotations

Example of broken resource (click to expand)

apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  name: my-service
spec:
  template:
    metadata:
      annotations:
        # WRONG: This causes the service to get stuck!
        # visibility should be a LABEL on the Service metadata, not here
        networking.knative.dev/visibility: cluster-local
    spec:
      containers:
        - image: gcr.io/knative-samples/helloworld-go

Correct usage:

apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  name: my-service
  labels:
    networking.knative.dev/visibility: cluster-local  # Correct!
spec:
  template:
    spec:
      containers:
        - image: gcr.io/knative-samples/helloworld-go

Release Note

Services with invalid networking.knative.dev/* annotations on the revision template now fail immediately with a clear error instead of getting stuck.

[knative-prow[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: linkvt Once this PR has been reviewed and has the lgtm label, please assign dprotaso for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• pkg/apis/OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 80.10%. Comparing base (5fbd94e) to head (8bf896f). :warning: Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #16296   +/-   ##
=======================================
  Coverage   80.10%   80.10%           
=======================================
  Files         215      215           
  Lines       13332    13333    +1     
=======================================
+ Hits        10679    10680    +1     
  Misses       2294     2294           
  Partials      359      359           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[linkvt]

/retest

[skonto]

Hi @linkvt! Some thoughts:

• Should we keep https://github.com/knative/networking/blob/main/pkg/apis/networking/v1alpha1/serverlessservice_validation.go#L29?
• Do we need to fail earlier like at the Service validation call?
• Are there other annotations that could cause similar issues beyond the networking ones?

[linkvt]

Hi @skonto , thanks for the review!

1. I would say we could keep the validation there as
   • we ensure the SKS is always created with valid annotations and avoid bad annotations e.g. even when it is created by other resources - even if unlikely
   • it doesn't do any damage keeping it
   • the LSP of my IDE didn't show me references in the vendor/ directory so I actually didn't see it when looking for references until now 😅
2. We're actually validating this within the Service validation call, here is the call chain:
   1. Service Validation: https://github.com/knative/serving/blob/4e32e7c78222f42c9a86ea623cc93ccbc0365798/pkg/apis/serving/v1/service_validation.go#L27
   2. Call of Service Spec validation: https://github.com/knative/serving/blob/4e32e7c78222f42c9a86ea623cc93ccbc0365798/pkg/apis/serving/v1/service_validation.go#L38
   3. Call of Configuration Spec validation: https://github.com/knative/serving/blob/4e32e7c78222f42c9a86ea623cc93ccbc0365798/pkg/apis/serving/v1/service_validation.go#L56
   4. Call of Revision Template validation: https://github.com/knative/serving/blob/4e32e7c78222f42c9a86ea623cc93ccbc0365798/pkg/apis/serving/v1/configuration_validation.go#L59
   5. Revision Template validation: https://github.com/knative/serving/blob/4e32e7c78222f42c9a86ea623cc93ccbc0365798/pkg/apis/serving/v1/revision_validation.go#L65
3. I'm not aware of other annotations but I'm kinda new to the project. Considering that the issue is 3 years old with no recent updates and no upvotes I think it's OK to fix only the problem described here and focus on other bugs afterwards 🙂

Let me know what you think, thanks!

[skonto]

We're actually validating this within the Service validation call, here is the call chain:

Yes, my thought was move this up in the chain to fail earlier along with any other check that makes sense. It is ok as it is the revision template that has the faulty annotation at he end of the day.

[skonto]

/lgtm

/hold for @dsimansk