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

Update ObservabilityPolicy API to have minimum one target ref #2753

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

salonichf5
Copy link
Contributor

@salonichf5 salonichf5 commented Nov 7, 2024

Proposed changes

Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the
following format:

Problem: User wants ObservabilityPolicy CRD should require a minimum of 1 TargetRef.

Solution: Add validation to have minimum one target ref for the ObservabilityPolicy CRDs.

Testing:

Tested manually using the example. Installed observabilityPolicy with on branch main, a target ref and ensuring everything works as expected.

Tested with specifying no target refs

kubectl apply -f - <<EOF
apiVersion: gateway.nginx.org/v1alpha1
kind: ObservabilityPolicy
metadata:
  name: coffee
spec:
  targetRefs: []
  tracing:
    strategy: ratio
    ratio: 75
    spanAttributes:
    - key: coffee-key
      value: coffee-value
EOF
The ObservabilityPolicy "coffee" is invalid:
* spec.targetRefs: Invalid value: 0: spec.targetRefs in body should have at least 1 items
* spec.targetRefs: Invalid value: "array": TargetRef Kind must be: HTTPRoute or GRPCRoute

Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.

Closes #2524

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.

ObservabilityPolicy API requires minimum one target ref specified when creating a policy.

@salonichf5 salonichf5 requested review from a team as code owners November 7, 2024 07:11
@github-actions github-actions bot added bug Something isn't working documentation Improvements or additions to documentation labels Nov 7, 2024
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.86%. Comparing base (cafbd5c) to head (1ba62a1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2753   +/-   ##
=======================================
  Coverage   89.86%   89.86%           
=======================================
  Files         107      107           
  Lines       10997    10997           
  Branches       50       50           
=======================================
  Hits         9883     9883           
  Misses       1054     1054           
  Partials       60       60           

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


🚨 Try these New Features:

@salonichf5
Copy link
Contributor Author

Should we mention in the release-note about the deprecation of v1alpha2 .

Currently, the user can use both versions of ObservabilityPolicy but the stored version is v1alpha2 . How do we deprecate it after 3 weeks?

Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

Doc update LGTM!

@sjberman
Copy link
Contributor

If you install an old version of the policy, can you still view it properly when running kubectl get or describe? And everything still works as expected? Same with the new version?

Also, please update all ObservabilityPolicy examples to use the new version.

We should mention this in the release notes. After three releases we'll remove the v1alpha1 version, so let's create an issue targeted for the 2.3 milestone for this.

@salonichf5
Copy link
Contributor Author

salonichf5 commented Nov 11, 2024

Created a new issue to remove version v1alpha1

@github-actions github-actions bot added the tests Pull requests that update tests label Nov 14, 2024
@salonichf5
Copy link
Contributor Author

If you install an old version of the policy, can you still view it properly when running kubectl get or describe? And everything still works as expected? Same with the new version?

Also, please update all ObservabilityPolicy examples to use the new version.

We should mention this in the release notes. After three releases we'll remove the v1alpha1 version, so let's create an issue targeted for the 2.3 milestone for this.

Yes, when I create a policy with CRDs on main, the policy is set to v1alpha1 version. When I apply the newly defined CRDs on my branch, the policy gets updated to v1alpha2

We can still create a policy with version v1alpha1 but it automatically gets upgraded to v1alpha2 since that is the stored version.

@sjberman
Copy link
Contributor

Let's update the release note to include what the actual change is (relating to the targetRef minimum)

@github-actions github-actions bot removed the tests Pull requests that update tests label Nov 19, 2024
@salonichf5 salonichf5 force-pushed the bug/observability-ref branch from 8834253 to 81d550f Compare November 19, 2024 17:28
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Nov 19, 2024
@salonichf5 salonichf5 force-pushed the bug/observability-ref branch 2 times, most recently from 8e66d66 to 193be55 Compare November 21, 2024 19:06
@salonichf5 salonichf5 enabled auto-merge (squash) November 21, 2024 19:08
@salonichf5 salonichf5 force-pushed the bug/observability-ref branch from 193be55 to 1ba62a1 Compare November 22, 2024 18:48
@salonichf5 salonichf5 merged commit d54377f into nginxinc:main Nov 22, 2024
40 checks passed
salonichf5 added a commit to salonichf5/nginx-gateway-fabric that referenced this pull request Dec 5, 2024
…nc#2753)

Update Observability API to have one minimum one target ref

Problem: User wants ObservabilityPolicy CRD should require a minimum of 1 TargetRef.

Solution: Add validation to have minimum one target ref for the ObservabilityPolicy CRDs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release-notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add validation to ObservabilityPolicy to require TargetRefs
4 participants