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

feat: support mergepatch to envoyproxy/ratelimit deployment #2374

Merged
merged 7 commits into from
Feb 23, 2024

Conversation

Xunzhuo
Copy link
Member

@Xunzhuo Xunzhuo commented Dec 29, 2023

What type of PR is this?

feat(infra): support mergepatch to envoyproxy/ratelimit deployment

What this PR does / why we need it:

Add unsupported fields in deployment, for private customizations like:

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
  name: mergepatch
  namespace: envoy-gateway-system
spec:
  provider:
    type: Kubernetes
    kubernetes:
      envoyDeployment:
        patch:
          type: StrategicMerge
          value:
            spec:
              template:
                spec:
                  hostNetwork: true
                  dnsPolicy: ClusterFirstWithHostNet
---
apiVersion: gateway.networking.k8s.io/v1
kind: GatewayClass
metadata:
  name: eg
spec:
  controllerName: gateway.envoyproxy.io/gatewayclass-controller
  parametersRef:
    group: gateway.envoyproxy.io
    kind: EnvoyProxy
    name: mergepatch
    namespace: envoy-gateway-system

Which issue(s) this PR fixes:

Fixes #2346

@Xunzhuo Xunzhuo requested a review from a team as a code owner December 29, 2023 07:03
Copy link

codecov bot commented Dec 29, 2023

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 63.38%. Comparing base (4c79ef9) to head (393da43).
Report is 5 commits behind head on main.

Files Patch % Lines
api/v1alpha1/validation/envoyproxy_validate.go 80.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2374      +/-   ##
==========================================
- Coverage   63.38%   63.38%   -0.01%     
==========================================
  Files         119      119              
  Lines       19098    19485     +387     
==========================================
+ Hits        12106    12350     +244     
- Misses       6193     6333     +140     
- Partials      799      802       +3     

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

@Xunzhuo Xunzhuo force-pushed the feat-mergepatch branch 6 times, most recently from 0c59f94 to b727ef9 Compare December 29, 2023 07:22
@zirain
Copy link
Member

zirain commented Jan 5, 2024

/retest

@Xunzhuo
Copy link
Member Author

Xunzhuo commented Jan 5, 2024

/retest

api/v1alpha1/shared_types.go Outdated Show resolved Hide resolved
api/v1alpha1/shared_types.go Outdated Show resolved Hide resolved
@arkodg
Copy link
Contributor

arkodg commented Feb 9, 2024

as a follow up, we need to make sure that if a patch fails, we need to surface that to the EnvoyProxy's GatewayClass status

@Xunzhuo Xunzhuo requested a review from arkodg February 22, 2024 02:13
//
// By default, StrategicMerge is used as the patch type.
// +optional
Type MergeType `json:"type,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

should be a pointer since its optional

@arkodg
Copy link
Contributor

arkodg commented Feb 22, 2024

small nit, else LGTM !
lets raise a follow up to reuse the same structure for Service as well

arkodg
arkodg previously approved these changes Feb 23, 2024
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !
lets raise some follow up GH issues that can hopefully be tackled pre GA

  • to populate patch errors in GatewayClass status
  • to support patching for envoy.service as well

@arkodg arkodg requested review from a team February 23, 2024 03:41
@Xunzhuo Xunzhuo requested review from arkodg and shawnh2 February 23, 2024 03:42
arkodg
arkodg previously approved these changes Feb 23, 2024
Signed-off-by: bitliu <[email protected]>
Copy link
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for bringing this feature in

@shawnh2
Copy link
Contributor

shawnh2 commented Feb 23, 2024

/retest

@arkodg arkodg requested review from zirain and a team February 23, 2024 04:09
@arkodg arkodg merged commit b94211c into envoyproxy:main Feb 23, 2024
20 checks passed
bpdohall added a commit to gravitational/gateway that referenced this pull request Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to set k8s fields not defined in the EnvoyProxy provider config
4 participants