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

Implement Bootstrap API #1094

Merged
merged 3 commits into from
Mar 6, 2023
Merged

Implement Bootstrap API #1094

merged 3 commits into from
Mar 6, 2023

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Mar 3, 2023

Relates to #31

Relates to envoyproxy#31

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested a review from a team as a code owner March 3, 2023 00:59
@arkodg arkodg closed this Mar 3, 2023
@arkodg arkodg reopened this Mar 3, 2023
arkodg added 2 commits March 3, 2023 11:18
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested a review from zirain March 4, 2023 01:21
// The config should have been validated already
if infra.Proxy.Config != nil &&
infra.Proxy.Config.Spec.Bootstrap != nil {
cfg = *infra.Proxy.Config.Spec.Bootstrap
Copy link
Member

Choose a reason for hiding this comment

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

I think we should unmarshal it to bootstrap struct to validate it if the bootstrap user provided is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the validation will happen in the validation webhook, and the data should be valid by the time it reaches this component


// Set a custom bootstrap config into EnvoyProxy API and ensure the same
// value is set as the container arg.
bstrap := "blah"
Copy link
Member

Choose a reason for hiding this comment

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

This is obvious a invalid bootstrap config I think? There should be an error reported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as I mentioned in the prevous comment, the validation will happen earlier. Just used a dummy string to test in this component.

Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

LGTM

@zirain zirain merged commit d1ae47d into envoyproxy:main Mar 6, 2023
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.

3 participants