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.

Sign up for GitHub

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

Jump to bottom

Validate httproutes.gateway.networking.k8s.io #11150

Merged
merged 6 commits into from
Sep 25, 2023
Merged

Validate httproutes.gateway.networking.k8s.io #11150

merged 6 commits into from
Sep 25, 2023

Conversation

mikutas
Copy link
Contributor

@mikutas mikutas commented Jul 23, 2023

Fixes #11116

@mikutas mikutas marked this pull request as ready for review July 23, 2023 14:19
@mikutas mikutas requested a review from a team as a code owner July 23, 2023 14:19
adleong
adleong reviewed Jul 24, 2023
View reviewed changes
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

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

Hi @mikutas

I believe that this change will start sending admission requests for httproutes.gateway.networking.k8s.io to the policy-controller. However, the policy-controller needs to be updated in order be able to handle these requests and do the correct validation on them. These updates would need to happen inhttps://github.com/linkerd/linkerd2/blob/main/policy-controller/src/admission.rs

We should also add admission tests in the policy-test directory to verify that admission is working correctly for httproutes.gateway.networking.k8s.io

@mikutas mikutas marked this pull request as draft July 25, 2023 00:35
@hawkw
Copy link
Contributor

hawkw commented Jul 25, 2023

I believe that this change will start sending admission requests for httproutes.gateway.networking.k8s.io to the policy-controller. However, the policy-controller needs to be updated in order be able to handle these requests and do the correct validation on them. These updates would need to happen inhttps://github.com/linkerd/linkerd2/blob/main/policy-controller/src/admission.rs

And, it's worth noting that the validating logic for gateway.networking.k8s.io HTTPRoutes will need to be somewhat different from the existing validation logic for policy.linkerd.io HTTPRoutes. The validation for policy.linkerd.io routes simply rejects the route completely if it contains filters Linkerd doesn't support. If we also validate Gateway API routes, we will not be able to reject unsupported filters, because those filters might be used by other Gateway API implementations. In general, when we're validating the shared upstream version of the HTTPRoute resource, we'll have to be more careful to ensure we don't reject configurations that are unsupported by Linkerd but used by other Gateway implementations.

@mikutas mikutas force-pushed the 11116 branch 5 times, most recently from ed5d1e6 to 3c0119d Compare July 29, 2023 06:52
@mikutas
Add tests
ac4a7db 
Signed-off-by: Takumi Sue <[email protected]>
@mikutas
Add tests
7636ae1 
Signed-off-by: Takumi Sue <[email protected]>
@mikutas
Copy link
Contributor Author

mikutas commented Jul 29, 2023

tried to add validation and test but copied many codes...

@mikutas mikutas marked this pull request as ready for review July 29, 2023 11:15
@mikutas mikutas changed the title fix: validate httproutes.gateway.networking.k8s.io Validate httproutes.gateway.networking.k8s.io Jul 29, 2023
@hawkw hawkw requested review from hawkw and adleong July 30, 2023 02:28
alpeb
alpeb approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

LGTM 👍
I'm gonna push a small change to address my comment above re RBAC

charts/linkerd-control-plane/templates/destination-rbac.yaml Outdated
@@ -179,6 +179,11 @@ webhooks:
- meshtlsauthentications
- serverauthorizations
- servers
- operations: ["CREATE", "UPDATE"]
apiGroups: ["gateway.networking.k8s.io"]
apiVersions: ["v1alpha2", "v1beta1"]
Copy link
Member

Choose a reason for hiding this comment

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

Can you please update this array with a wildcard, like you did for policy.linkerd.io in the other PR?

@adleong adleong merged commit 74337f9 into linkerd:main Sep 25, 2023
40 checks passed
@mikutas mikutas deleted the 11116 branch September 25, 2023 23:59
olix0r added a commit that referenced this pull request Sep 28, 2023
@olix0r
edge-29.9.4
f4181a1 
This edge release makes Linkerd even better.

* Added a controlPlaneVersion override to the `linkerd-control-plane` Helm chart
  to support including SHA256 image digests in Linkerd manifests (thanks
  @cromulentbanana!) ([#11406])
* Improved `linkerd viz check` to attempt to validate that the Prometheus scrape
  interval will work well with the CLI and Web query parameters ([#11376])
* Fixed an issue where the destination controller would not update pod metadata
  for profile resolutions for a pod accessed via the host network (e.g.
  HostPort endpoints) ([#11334]).
* Added a validating webhook config for httproutes.gateway.networking.k8s.io
  resources (thanks @mikutas!) ([#11150])

[#11150]: #11150
[#11334]: #11334
[#11376]: #11376
[#11406]: #11406
@olix0r olix0r mentioned this pull request Sep 28, 2023
Merged
olix0r added a commit that referenced this pull request Sep 28, 2023
@olix0r
edge-29.9.4
5e367be 
This edge release makes Linkerd even better.

* Added a controlPlaneVersion override to the `linkerd-control-plane` Helm chart
  to support including SHA256 image digests in Linkerd manifests (thanks
  @cromulentbanana!) ([#11406])
* Improved `linkerd viz check` to attempt to validate that the Prometheus scrape
  interval will work well with the CLI and Web query parameters ([#11376])
* Fixed an issue where the destination controller would not update pod metadata
  for profile resolutions for a pod accessed via the host network (e.g.
  HostPort endpoints) ([#11334]).
* Added a validating webhook config for httproutes.gateway.networking.k8s.io
  resources (thanks @mikutas!) ([#11150])

[#11150]: #11150
[#11334]: #11334
[#11376]: #11376
[#11406]: #11406
olix0r added a commit that referenced this pull request Sep 29, 2023
@olix0r
edge-23.9.4 (#11441)
bc97b02 
This edge release makes Linkerd even better.

* Added a controlPlaneVersion override to the `linkerd-control-plane` Helm chart
  to support including SHA256 image digests in Linkerd manifests (thanks
  @cromulentbanana!) ([#11406])
* Improved `linkerd viz check` to attempt to validate that the Prometheus scrape
  interval will work well with the CLI and Web query parameters ([#11376])
* Improved CLI error handling to print differentiated error information when
  versioncheck.linkerd.io cannot be resolved (thanks @dtaskai) ([#11377])
* Fixed an issue where the destination controller would not update pod metadata
  for profile resolutions for a pod accessed via the host network (e.g.
  HostPort endpoints) ([#11334]).
* Added a validating webhook config for httproutes.gateway.networking.k8s.io
  resources (thanks @mikutas!) ([#11150])
* Introduced a new `multicluster check --timeout` flag to limit the time
  allowed for Kubernetes API calls (thanks @moki1202) ([#11420])

[#11150]: #11150
[#11334]: #11334
[#11376]: #11376
[#11377]: #11377
[#11406]: #11406
[#11420]: #11420
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adleong adleong adleong approved these changes

@alpeb alpeb alpeb approved these changes

@hawkw hawkw Awaiting requested review from hawkw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

policy-controller validating admission webhook doesn't validate gateway.networking.kubernetes.io HTTPRoutes
4 participants
@mikutas @hawkw @alpeb @adleong