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 GatewayClass status based on EnvoyProxy validation #1107

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Mar 7, 2023

  • Refactor EnvoyProxy's Validate API
  • If a GatewayClass references a EnvoyProxy resource, ensure that that the GatewayClass status condition is based off the EnvoyProxy validation result

Relates to #31

@arkodg arkodg requested a review from a team as a code owner March 7, 2023 00:30
@arkodg arkodg marked this pull request as draft March 7, 2023 00:30
@arkodg
Copy link
Contributor Author

arkodg commented Mar 7, 2023

@Xunzhuo, Im unable to generate the manifests for the webhook by adding the kubebuilder annotations to the file and running make manifests, has something changed with the helm stuff ?

@Xunzhuo
Copy link
Member

Xunzhuo commented Mar 7, 2023

Nothing changes except the output path in make manifests.

Can you tell where,what annotations you add? I will have a try too

@arkodg

@zirain
Copy link
Contributor

zirain commented Mar 24, 2023

kindly ping @arkodg , are you still working on this?

@arkodg
Copy link
Contributor Author

arkodg commented Mar 24, 2023

kindly ping @arkodg , are you still working on this?

yes 😅 will resume next week

* Implement controller-runtime Webhook interface for EnvoyProxy

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg
Copy link
Contributor Author

arkodg commented Apr 5, 2023

Below logs highlight that the the Accepted condition is set to False with a reason of InvalidParameters when validation fails on the referenced EnvoyProxy resource

🐳 ~/go-workspace/src/github.com/envoyproxy/gateway$ kubectl get envoyproxy/ep-config -n envoy-gateway-system -o yaml
apiVersion: config.gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"config.gateway.envoyproxy.io/v1alpha1","kind":"EnvoyProxy","metadata":{"annotations":{},"name":"ep-config","namespace":"envoy-gateway-system"},"spec":{"bootstrap":"blah","logging":{"level":{"system":"debug"}}}}
  creationTimestamp: "2023-04-05T21:58:31Z"
  generation: 2
  name: ep-config
  namespace: envoy-gateway-system
  resourceVersion: "5009"
  uid: ee973c95-a96f-49f3-90ec-233349dbe1bc
spec:
  bootstrap: blah
  logging:
    level:
      system: debug
🐳 ~/go-workspace/src/github.com/envoyproxy/gateway$ kubectl get gc/eg -o yaml
apiVersion: gateway.networking.k8s.io/v1beta1
kind: GatewayClass
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"gateway.networking.k8s.io/v1beta1","kind":"GatewayClass","metadata":{"annotations":{},"name":"eg"},"spec":{"controllerName":"gateway.envoyproxy.io/gatewayclass-controller","parametersRef":{"group":"config.gateway.envoyproxy.io","kind":"EnvoyProxy","name":"ep-config","namespace":"envoy-gateway-system"}}}
  creationTimestamp: "2023-04-05T21:54:21Z"
  finalizers:
  - gateway-exists-finalizer.gateway.networking.k8s.io
  generation: 2
  name: eg
  resourceVersion: "31384"
  uid: 0db8dcfd-b3f7-4cff-8c82-acedc388b78f
spec:
  controllerName: gateway.envoyproxy.io/gatewayclass-controller
  parametersRef:
    group: config.gateway.envoyproxy.io
    kind: EnvoyProxy
    name: ep-config
    namespace: envoy-gateway-system
status:
  conditions:
  - lastTransitionTime: "2023-04-05T22:10:54Z"
    message: 'Invalid parametersRef: invalid gatewayclass eg: invalid envoyproxy:
      unable to unmarshal user bootstrap: proto: syntax error (line 1:1): unexpected
      token "blah"'
    observedGeneration: 1
    reason: InvalidParameters
    status: "False"
    type: Accepted
    ```

@arkodg arkodg changed the title Add validation webhook for EnvoyProxy resource Update GatewayClass status based on EnvoyProxy validation Apr 5, 2023
@arkodg arkodg force-pushed the webhook branch 2 times, most recently from 4fcf245 to 1fb7dae Compare April 5, 2023 22:23
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #1107 (14da7f5) into main (d53b561) will decrease coverage by 3.22%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main    #1107      +/-   ##
==========================================
- Coverage   63.95%   60.73%   -3.22%     
==========================================
  Files          80       84       +4     
  Lines       10010    10542     +532     
==========================================
+ Hits         6402     6403       +1     
- Misses       3184     3715     +531     
  Partials      424      424              
Impacted Files Coverage Δ
internal/provider/kubernetes/controller.go 50.38% <0.00%> (+0.22%) ⬆️
api/config/v1alpha1/validate.go 75.58% <100.00%> (ø)
internal/xds/server/runner/runner.go 30.09% <100.00%> (ø)

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg marked this pull request as ready for review April 5, 2023 22:28
@arkodg arkodg added this to the 0.4.0-rc.1 milestone Apr 6, 2023
@arkodg arkodg added the priority/high Label used to express the "high" priority level label Apr 6, 2023
@zirain
Copy link
Contributor

zirain commented Apr 6, 2023

what's the expected behavior when apply a wrong EnvoyProxy? EG use last correct EnvoyProxy?
what will happen when EG control plane restarts after a wrong EnvoyProxy applied?

@arkodg
Copy link
Contributor Author

arkodg commented Apr 6, 2023

what's the expected behavior when apply a wrong EnvoyProxy? EG use last correct EnvoyProxy?
what will happen when EG control plane restarts after a wrong EnvoyProxy applied?

thanks for raising this, even w/o this PR we were failing fast - removing the GatewayClass from being accepted if EnvoyProxy was invalid, causing entire deletion of data plane. Imo this should be discussed in a separate issue if we want to handle this more gracefully

Copy link
Contributor

@zirain zirain left a comment

Choose a reason for hiding this comment

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

Overall LGTM, can you open a ticket for the discussion?

@arkodg arkodg merged commit 9867c6a into envoyproxy:main Apr 6, 2023
@arkodg arkodg deleted the webhook branch April 6, 2023 16:24
arkodg added a commit to arkodg/gateway that referenced this pull request Apr 6, 2023
Due to lack of Merge ordering between envoyproxy#1257
and envoyproxy#1107

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg mentioned this pull request Apr 6, 2023
zirain pushed a commit that referenced this pull request Apr 7, 2023
Due to lack of Merge ordering between #1257
and #1107

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high Label used to express the "high" priority level
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants