Add validation to ensure no duplicate TargetRefs in policies

[bjee19]

Proposed changes

Validate that there are no duplicate TargetRefs in the UpstreamSettingsPolicy and ObservabilityPolicy. Additionally, since this change strengthens the validation rules on the ObservabilityPolicy API, we are bumping the ObservabilityPolicy API version from v1alpha1 to v1alpha2.

Problem: It's possible to create an ObservabilityPolicy or UpstreamSettingsPolicy with duplicate targetRefs. This is not a valid configuration and should be prevented.

Solution: Add CEL validation to prevent this.

Testing: Manually deployed both the UpstreamSettingsPolicy and ObservabilityPolicy and verified that duplicate targetRefs are invalid.

Closes #2923

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.

Add validation to ObservabilityPolicy and UpstreamSettingsPolicy APIs to ensure no duplicate targetRefs are allowed. Also, bumped the ObservabilityPolicy API version from `v1alpha1` to `v1alpha2`.

[bjee19]

Is there any specific testing I should do when bumping the version of the ObservabilityPolicy?

I verified applying the v1alpha1 version policy still works, and shows this warning:

Warning: The 'v1alpha1' version of ObservabilityPolicy API is deprecated, please migrate to 'v1alpha2'..

In addition the CEL validation is not present in the v1alpha1 version.

[bjee19]

For visibility, here is the commit which @salonichf5 wrote which bumped the version in the past but was reverted. I based my work off of that.

Should I reopen this task to fully remove v1alpha1 in 2.3.0?

[salonichf5]

Is there any specific testing I should do when bumping the version of the ObservabilityPolicy?

I verified applying the v1alpha1 version policy still works, and shows this warning:

Warning: The 'v1alpha1' version of ObservabilityPolicy API is deprecated, please migrate to 'v1alpha2'..

In addition the CEL validation is not present in the v1alpha1 version.

There are 4 cases you should test

• We can create a policy with v1alpha1 version
• We can create a policy with v1alpha2 version
• We can upgrade the policy from v1alpha1 --> v1alpha2 version.

Let me know if this assumption is correct @sjberman

• I remember not being able to downgrade from v1alpha2 to v1alpha1 and this is the desired behavior.

[salonichf5]

For visibility, here is the commit which @salonichf5 wrote which bumped the version in the past but was reverted. I based my work off of that.

Should I reopen this task to fully remove v1alpha1 in 2.3.0?

Yes we need a story to track deprecation of the old version.

[ADubhlaoich]

Doc changes LGTM!

[bjee19]

We can create a policy with v1alpha1 version

Works, displays Warning message, and when using kubectl get on the policy it shows apiVersion: gateway.nginx.org/v1alpha2 which was the same result from when we tried to change it last time.

We can create a policy with v1alpha2 version

Works, CEL validation is enabled.

We can upgrade a policy from v1alpha1 to v1alpha2

I tested this by simply changing the apiVersion on the policy to v1alpha2 and applying the change through kubectl apply -f. This works, not sure if there is some other more formal way I should be testing upgrading through.

I remember not being able to downgrade from v1alpha2 to v1alpha1 and this is the desired behavior.

This lines up with my results. Can't really downgrade because the apiversion shown on a policy created with v1alpha1 is shown as v1alpha2

[bjee19]

Re-opened #2767 to track the removal of the v1alpha1 API version.

[salonichf5]

We can create a policy with v1alpha1 version

Works, displays Warning message, and when using kubectl get on the policy it shows apiVersion: gateway.nginx.org/v1alpha2 which was the same result from when we tried to change it last time.

We can create a policy with v1alpha2 version

Works, CEL validation is enabled.

We can upgrade a policy from v1alpha1 to v1alpha2

I tested this by simply changing the apiVersion on the policy to v1alpha2 and applying the change through kubectl apply -f. This works, not sure if there is some other more formal way I should be testing upgrading through.

I remember not being able to downgrade from v1alpha2 to v1alpha1 and this is the desired behavior.

This lines up with my results. Can't really downgrade because the apiversion shown on a policy created with v1alpha1 is shown as v1alpha2

Looks right !

[salonichf5]

question: Why is only name considered for UpstreamSettings Policy and not a combination of name and kind. Assuming their parent Ref is a gateway?

// TargetRefs must be _distinct_. This means that the multi-part key defined by `kind` and `name` must
	// be unique across all targetRef entries in the ObservabilityPolicy.

[bjee19]

good question, I did it this way because in the UpstreamSettingsPolicy, the Kind must be of type "service" and also has constraints on the group.

I originally had combination of name-kind-group, but realized since we have constraints and already have validation on these other fields, we can just do name for USP and combination of name and kind for the observability policy

[salonichf5]

You might need to open this PR against main branch instead of your fork for the Plus Build fails

[salonichf5]

Just had a small question otherwise looks good to me

[kate-osborn]

@bjee19 since this is a breaking change, I added the change label to the PR.

[sjberman]

I feel like ObservabilityPolicy could just import these from v1alpha1, no?

[kate-osborn]

Yep, we can leave this file in v1alpha1

[bjee19]

I like the idea of keeping the package shared types different and since I also saw that there were distinct shared_types.go files in the gateway api repo https://github.com/kubernetes-sigs/gateway-api/tree/main/apis , I think it would be good to do.

However I do see how the alpha packages (only 2 of them) share the same shared_types.go while the beta and v1 packages have their own shared_types.go files (even though there is a lot of overlap).

So I would also be fine with sharing the types within the larger version (alpha,beta,v1) and creating a new file perhaps if a type changes within (like alpha1->alpha2) with only the specific changes necessary. While creating different shared_types.go files for the larger version/stage changes (not sure what this is called).

[kate-osborn]

My opinion is that we shouldn't create a new shared_types file until there's a reason to (e.g. a new shared type is added to v1alpha2 or a field changes).

[bjee19]

Sounds good yea I'll change it

[kate-osborn]

@bjee19 even though there are no spec/status field changes to ObservabilityPolicy, now that we moved the API to v1alpha2, we need to update the controller code to process v1alpha2 ObservabilityPolicies instead of v1alpha1.

[bjee19]

we need to update the controller code to process v1alpha2 ObservabilityPolicies instead of v1alpha1.

Following doing this, I tested applying a v1alpha1 Observability policy following the tracing guide here: https://docs.nginx.com/nginx-gateway-fabric/how-to/monitoring/tracing/, and found that it successfully updated NGINX conf. Tested the v1alpha2 and that successfully updated NGINX conf too.

[bjee19]

Does anyone know if there is a better way to update the controller code to use the new version of a CRD? I manually did it and there isn't really any verification that I caught all the places besides me just double checking and running the tests.

[bjee19]

how is this naming change? I followed what we do above with the gatewayv1alpha3 and gatewayv1beta1 style formatting.

Should we change other places in the code base that still use ngfAPI on a need to basis?

[kate-osborn]

Name change looks good to me.

Should we change other places in the code base that still use ngfAPI on a need to basis?

Yep 🙂

[kate-osborn]

Does anyone know if there is a better way to update the controller code to use the new version of a CRD? I manually did it and there isn't really any verification that I caught all the places besides me just double checking and running the tests.

Don't think there's a quicker way. You can double check that you caught everything by looking at the code references of the v1alpha1 ObservabilityPolicy types. There shouldn't be any in the internal directory.

[bjee19]

You can double check that you caught everything by looking at the code references of the v1alpha1 ObservabilityPolicy types. There shouldn't be any in the internal directory.

Yep, thanks for pointing that out, I already did that on my double check so we're good i guess 🤞