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 merging user and default bootstrap configurations #1791

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Aug 16, 2023

Related issue: #1685

Since this is a breaking change, please consider including the following notification, or something similar, in the release note to remind the users to update their EnvoyProxy configuration.

Important The Booststrap field of EnvoyProxy API has been changed from a string to a struct. Please ensure you adjust your EnvoyProxy configurations accordingly. Here is an example:

Before:

apiVersion: config.gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
  name: custom-proxy-config
  namespace: envoy-gateway-system
spec:
  bootstrap: |
    admin:
      access_log: // truncated for brevity

After:

apiVersion: config.gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
  name: custom-proxy-config
  namespace: envoy-gateway-system
spec:
  bootstrap:
    type: Replace
    value: |
      admin:
        access_log: // truncated for brevity

@zhaohuabing zhaohuabing requested a review from a team as a code owner August 16, 2023 09:58
@zhaohuabing zhaohuabing marked this pull request as draft August 16, 2023 09:59
@zhaohuabing zhaohuabing marked this pull request as ready for review August 16, 2023 11:14
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #1791 (46a9e04) into main (23652af) will decrease coverage by 0.08%.
The diff coverage is 59.74%.

@@            Coverage Diff             @@
##             main    #1791      +/-   ##
==========================================
- Coverage   65.15%   65.07%   -0.08%     
==========================================
  Files          84       86       +2     
  Lines       12230    12281      +51     
==========================================
+ Hits         7968     7992      +24     
- Misses       3754     3775      +21     
- Partials      508      514       +6     
Files Changed Coverage Δ
internal/xds/bootstrap/util.go 0.00% <0.00%> (ø)
api/config/v1alpha1/validation/validate.go 74.73% <53.84%> (-2.19%) ⬇️
...ternal/infrastructure/kubernetes/proxy/resource.go 91.39% <62.50%> (-1.66%) ⬇️
internal/cmd/egctl/translate.go 83.07% <63.63%> (-0.36%) ⬇️
internal/utils/yaml/yaml.go 75.00% <75.00%> (ø)

@arkodg
Copy link
Contributor

arkodg commented Aug 16, 2023

thanks for picking this up @zhaohuabing, can you split up API and implementation into 2 PRs, which may help with faster reviews, thanks !

@zhaohuabing
Copy link
Member Author

thanks for picking this up @zhaohuabing, can you split up API and implementation into 2 PRs, which may help with faster reviews, thanks !

@arkodg Since this is a breaking change, it won't pass the build if just changing the API.

@arkodg
Copy link
Contributor

arkodg commented Aug 17, 2023

thanks for picking this up @zhaohuabing, can you split up API and implementation into 2 PRs, which may help with faster reviews, thanks !

@arkodg Since this is a breaking change, it won't pass the build if just changing the API.

ah yah, sure lets stick with this for now

@zhaohuabing zhaohuabing force-pushed the merge-bootstrap branch 7 times, most recently from 708f1f0 to 18a5e3e Compare August 17, 2023 03:07
@zhaohuabing
Copy link
Member Author

@arkodg Should I add a notice in the release note so that users won't be surprised by this breaking change? If yes, do you know where to add this note?

@arkodg arkodg added the release-note Indicates a required release note label Aug 17, 2023
@arkodg
Copy link
Contributor

arkodg commented Aug 17, 2023

@arkodg Should I add a notice in the release note so that users won't be surprised by this breaking change? If yes, do you know where to add this note?

added the release note label, hopefully this reminds the release manager to add this note, we can work on proactively adding release notes too, maybe in the PR description that the release manager can copy into the release notes

// MergeYAML merges two yaml files. The second yaml file will override the first one if the same key exists.
// This method can add or override a value within a map, or add a new value to a list.
// Please note that this method can't override a value within a list.
func MergeYAML(base, override string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use https://github.com/evanphx/json-patch instead, i.e. convert the YAMLs into JSON using sigs.k8s.io/yaml and then use jsonpatch.MergePatch and convert it back to YAML ?

Copy link
Member Author

@zhaohuabing zhaohuabing Aug 18, 2023

Choose a reason for hiding this comment

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

jsonpatch.MergePatch can't merge two JSON strings, and there are no other methods to merge JSON strings either.

See https://github.com/evanphx/json-patch#create-and-apply-a-merge-patch

@arkodg
Copy link
Contributor

arkodg commented Aug 17, 2023

can we also update the user docs https://gateway.envoyproxy.io/latest/user/customize-envoyproxy.html#customize-envoyproxy-bootstrap-config in this PR or as a follow up, thanks !

@zhaohuabing zhaohuabing force-pushed the merge-bootstrap branch 2 times, most recently from 07ba10a to acc7ec1 Compare August 18, 2023 04:19
zirain
zirain previously approved these changes Aug 20, 2023
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.

LGTM, thanks @zhaohuabing !

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
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 !

@arkodg arkodg merged commit c5c675f into envoyproxy:main Aug 21, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Indicates a required release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants