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

Adds Envoy Bootstrap Config #206

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Adds Envoy Bootstrap Config #206

merged 1 commit into from
Aug 23, 2022

Conversation

danehans
Copy link
Contributor

Adds support for managing Envoy's bootstrap config through a ConfigMap.

Fixes: #201

Signed-off-by: danehans [email protected]

@danehans danehans requested a review from a team as a code owner August 17, 2022 18:13
@danehans danehans added the provider/kubernetes Issues related to the Kubernetes provider label Aug 17, 2022
@danehans danehans added this to the 0.2.0-rc1 milestone Aug 17, 2022
address:
socket_address:
address: {{ .XdsServerAddress }}
port_value: 18000
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 templated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#212 tracks this requirement.

@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2022

Codecov Report

Merging #206 (06447e3) into main (2be093c) will decrease coverage by 0.36%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
- Coverage   63.60%   63.23%   -0.37%     
==========================================
  Files          27       27              
  Lines        2349     2358       +9     
==========================================
- Hits         1494     1491       -3     
- Misses        768      776       +8     
- Partials       87       91       +4     
Impacted Files Coverage Δ
internal/infrastructure/kubernetes/deployment.go 82.38% <53.84%> (-6.95%) ⬇️

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

Copy link
Member

@Alice-Lilith Alice-Lilith left a comment

Choose a reason for hiding this comment

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

bootstrap looks good to me

Copy link
Contributor

@LukeShu LukeShu left a comment

Choose a reason for hiding this comment

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

Some nits that you can choose to ignore. On the whole, lgtm.

// envoyHTTPPort is the container port number of Envoy's HTTP endpoint.
envoyHTTPPort = int32(8080)
// envoyHTTPSPort is the container port number of Envoy's HTTPS endpoint.
envoyHTTPSPort = int32(8443)
)

var bootstrapTmpl = template.Must(template.New(envoyCfgFileName).Parse(`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider putting this in a separate bootstrap.yaml file and saying

//go:embed bootstrap.yaml
const string bootstrapTmplStr

var bootstrapTmpl = template.Must(template.New(envoyCfgFileName).Parse(bootstrapTmplStr))

so that the file is easier to edit, with editors recognizing it as yaml.

Comment on lines +119 to +123
buf := new(bytes.Buffer)
if err := bootstrapTmpl.Execute(buf, b.parameters); err != nil {
return fmt.Errorf("failed to render bootstrap config: %v", err)
}
b.rendered = buf.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

If we never need access it as a []byte or mutate it (other than append), then it'd be more efficient to use a strings.Builder instead of a bytes.Buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danehans danehans merged commit 9c34a8c into envoyproxy:main Aug 23, 2022
@danehans danehans deleted the issue_201 branch August 23, 2022 15:56
@danehans
Copy link
Contributor Author

@LukeShu since we're getting close to the RC1 release, I created issues for most of your feedback and #224 to use a const for the Envoy service name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider/kubernetes Issues related to the Kubernetes provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Bootstrap Config to Envoy Deployment
5 participants