Skip to content

feat: set envoyproxy image to envoy-dev latest in main#712

Merged
arkodg merged 1 commit intoenvoyproxy:mainfrom
Xunzhuo:feat-custom-image
Nov 8, 2022
Merged

feat: set envoyproxy image to envoy-dev latest in main#712
arkodg merged 1 commit intoenvoyproxy:mainfrom
Xunzhuo:feat-custom-image

Conversation

@Xunzhuo
Copy link
Copy Markdown
Member

@Xunzhuo Xunzhuo commented Nov 8, 2022

Fixes: #672

Signed-off-by: bitliu bitliu@tencent.com

Copy link
Copy Markdown
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

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

[decision]: Do we expose the Envoy image version to users?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Since this is a user-facing API, I think it's better to s/in the Infra IR to deploy EnvoyProxy/for the managed Envoy proxies/
  • IMO this field does not belong in the Gateway struct` and I'm unsure if we want to expose the Envoy version to users. Making the version configurable can create a support nightmare of neverending EG<>Envoy combinations. IMO we release EG with the latest Envoy release and users upgrade EG if they need a newer version of Envoy. Thoughts @arkodg @LukeShu @skriss @youngnick @AliceProxy?
  • Add an empty line before +optional.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 to delaying this API change until users really need it. Post v0.3.0 an exercise we will need to do is

  • what level of customizations do we need to provide for
    • xds
    • EG platform/k8s deployment/service
    • Envoy platform/k8s deployment/service
  • how do we expose these customizations

Copy link
Copy Markdown
Member Author

@Xunzhuo Xunzhuo Nov 8, 2022

Choose a reason for hiding this comment

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

Thanks for comments @danehans @arkodg, this PR is WIP state, and the approach in this PR is what I want to have a discussion. I think you are right, API changes should be careful. But for resolving the linked issue, if we want to update the envoyproxy image in our release CI or by make targets, I think it should be updated in the configuration, otherwise we can only change the default value of envoy Image. It is a const, feel a bit weird to change it repeatly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it should be updated in the configuration...

If we expose the Envoy image here, then users can change the image. I don't think we should do this due to the concerns I stated in #712 (comment).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I understand that @danehans. But for resolving #672, how do we do it without exposing configuration? By just changing the default image const? We keep the const to the envoy-dev:latest in main. Everytime we release, we update the const to the appropriate envoy release version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We keep the const to the envoy-dev:latest in main. Everytime we release, we update the const to the appropriate envoy release version.

Yes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay, if so, this is just a easy-update. I have updated. PTAL @danehans @arkodg.

Signed-off-by: bitliu <bitliu@tencent.com>
@Xunzhuo Xunzhuo marked this pull request as ready for review November 8, 2022 17:56
@Xunzhuo Xunzhuo requested a review from a team as a code owner November 8, 2022 17:56
@Xunzhuo Xunzhuo changed the title feat: add proxyImage field to custom EnvoyProxy Image feat: set envoyproxy image to envoy-dev latest in main Nov 8, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 8, 2022

Codecov Report

Merging #712 (2b78ff8) into main (7db9a40) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #712   +/-   ##
=======================================
  Coverage   63.29%   63.29%           
=======================================
  Files          47       47           
  Lines        5806     5806           
=======================================
  Hits         3675     3675           
  Misses       1902     1902           
  Partials      229      229           
Impacted Files Coverage Δ
internal/ir/infra.go 67.41% <ø> (ø)

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

@arkodg arkodg merged commit 672a0cc into envoyproxy:main Nov 8, 2022
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 Support for Main to use envoy-dev Image

4 participants