chore(*) delete CLI flag '--bootstrap-version' #2965

lobkovilya · 2021-10-18T13:17:21Z

Summary

Based on envoyproxy/envoy#16456, since v4 is very far away we decided to delete a bootstrap version field from Kuma.

Full changelog

delete from kuma-dp flags
delete from BootstrapRequest
delete from kuma-cp config

Issues resolved

N/A

Documentation

Link to the website documentation PR

Testing

Unit tests
E2E tests
Manual testing on Universal
Manual testing on Kubernetes

Backwards compatibility

Add backport-to-stable label if the code is backwards compatible. Otherwise, list breaking changes.

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

codecov-commenter · 2021-10-18T13:35:06Z

Codecov Report

Merging #2965 (0584249) into master (c586219) will increase coverage by 0.01%.
The diff coverage is 61.36%.

@@            Coverage Diff             @@
##           master    #2965      +/-   ##
==========================================
+ Coverage   52.37%   52.39%   +0.01%     
==========================================
  Files         912      912              
  Lines       52522    52493      -29     
==========================================
- Hits        27510    27502       -8     
+ Misses      22817    22798      -19     
+ Partials     2195     2193       -2

Impacted Files	Coverage Δ
pkg/config/app/kuma-dp/config.go	`63.15% <ø> (ø)`
...pp/kuma-dp/pkg/dataplane/envoy/remote_bootstrap.go	`60.95% <40.00%> (-0.37%)`	⬇️
pkg/xds/bootstrap/generator.go	`62.94% <63.63%> (-0.88%)`	⬇️
pkg/xds/bootstrap/handler.go	`41.81% <66.66%> (ø)`
app/kuma-dp/cmd/run.go	`71.14% <100.00%> (+0.39%)`	⬆️
app/kuma-dp/pkg/dataplane/envoy/envoy.go	`79.82% <100.00%> (-0.52%)`	⬇️
pkg/config/xds/bootstrap/config.go	`56.25% <100.00%> (-0.51%)`	⬇️
pkg/core/resources/manager/cache.go	`83.11% <0.00%> (-5.20%)`	⬇️
api/observability/v1/mads.pb.go	`35.56% <0.00%> (+1.03%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c586219...0584249. Read the comment docs.

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

michaelbeaumont · 2021-10-18T15:50:56Z

app/kuma-dp/cmd/run.go

@@ -226,7 +226,8 @@ func newRunCmd(opts kuma_cmd.RunCmdOpts, rootCtx *RootContext) *cobra.Command {
 	cmd.PersistentFlags().StringVar(&cfg.ControlPlane.CaCertFile, "ca-cert-file", cfg.ControlPlane.CaCertFile, "Path to CA cert by which connection to the Control Plane will be verified if HTTPS is used")
 	cmd.PersistentFlags().StringVar(&cfg.DataplaneRuntime.BinaryPath, "binary-path", cfg.DataplaneRuntime.BinaryPath, "Binary path of Envoy executable")
 	cmd.PersistentFlags().Uint32Var(&cfg.DataplaneRuntime.Concurrency, "concurrency", cfg.DataplaneRuntime.Concurrency, "Number of Envoy worker threads")
-	cmd.PersistentFlags().StringVar(&cfg.Dataplane.BootstrapVersion, "bootstrap-version", cfg.Dataplane.BootstrapVersion, "Bootstrap version (and API version) of xDS config. If empty, default version defined in Kuma CP will be used. (ex. '2', '3')")
+	cmd.PersistentFlags().StringVar(&bootstrapVersion, "bootstrap-version", "", "Bootstrap version (and API version) of xDS config. If empty, default version defined in Kuma CP will be used. (ex. '2', '3')")
+	_ = cmd.PersistentFlags().MarkDeprecated("bootstrap-version", "Envoy API v3 is used by default")


This message makes it seem like it still does something IMO since a deprecated command could just be discouraged. I think changing "by default" to "and can not be changed. This flag is not respected." or something similar would help. Might even be worth throwing an error if it's used?

I'd recommend against throwing an error, since that would immediately break existing deployments that use the flag.

I agree, we can't throw an error here. Updated the message, now it looks like:

Flag --bootstrap-version has been deprecated, Envoy API v3 is used and can not be changed

yeah indeed, although if somebody uses the flag and sets something different from the default, just ignoring it may not be much better since presumably something will break for them that's even harder to diagnose. In the end a proper deprecate -> remove process would be best but I think there wasn't really time here.

michaelbeaumont · 2021-10-18T15:52:45Z

pkg/config/xds/bootstrap/config.go

 	return nil
 }

 func DefaultBootstrapServerConfig() *BootstrapServerConfig {
 	return &BootstrapServerConfig{
-		APIVersion: envoy_common.APIV3,
-		Params:     DefaultBootstrapParamsConfig(),
+		//APIVersion: envoy_common.APIV3,


Might be better just to leave this out, otherwise it might get out of sync one day.

yep, that was unintentionally, thanks!

jpeach · 2021-10-18T23:50:09Z

pkg/xds/bootstrap/types/bootstrap_request.go

-	DNSPort          uint32           `json:"dnsPort,omitempty"`
-	EmptyDNSPort     uint32           `json:"emptyDnsPort,omitempty"`
+	DNSPort         uint32            `json:"dnsPort,omitempty"`
+	EmptyDNSPort    uint32            `json:"emptyDnsPort,omitempty"`


I don't think that this is a compatible change. If the DP is upgraded before the CP is, then the CP would not receive the BootstrapVersion field when it unmarshals the request JSON. That would be OK, but the field would be empty, so in the bootstrap generator the check in (*bootstrapGenerator)configForParameters() would fail and the DP would not be able to bootstrap.

To get across the upgrade, it would be safer for the DP to always send this field with a fixed value, then we can ignore it on the CP side and remove the DP code in a later release.

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

jpeach

This looks good to me. We should file an issue to follow up and remove remnants of the bootstrap in a future release (and we can ref the issue number from the TODO comments).

@lobkovilya Did you manually confirm that this works with a Envoy 1.18 DP?

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

lobkovilya · 2021-10-21T14:41:36Z

@jpeach done #2986

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> (cherry picked from commit 85b4c67) Co-authored-by: Ilya Lobkov <ilya.lobkov@konghq.com>

chore(*) delete CLI flag '--bootstrap-version'

1eb9bce

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

lobkovilya added the backport-to-stable label Oct 18, 2021

lobkovilya requested a review from a team as a code owner October 18, 2021 13:17

chore(*) fix integration

87a1d86

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

lobkovilya mentioned this pull request Oct 18, 2021

chore(*) update Envoy to 1.20 #2967

Merged

6 tasks

michaelbeaumont reviewed Oct 18, 2021

View reviewed changes

jpeach reviewed Oct 18, 2021

View reviewed changes

lobkovilya added 2 commits October 19, 2021 10:00

chore(*) review

755e585

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

chore(*) fix test

0584249

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

jpeach approved these changes Oct 21, 2021

View reviewed changes

chore(*) add an issue ref to TODOs

e2a63e0

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

lobkovilya merged commit 85b4c67 into master Oct 21, 2021

lobkovilya deleted the chore/delete-bootstrap-version branch October 21, 2021 15:25

mergify bot mentioned this pull request Oct 21, 2021

chore(*) delete CLI flag '--bootstrap-version' (backport #2965) #2987

Merged

lobkovilya added a commit that referenced this pull request Oct 21, 2021

chore(*) delete CLI flag '--bootstrap-version' (#2965) (#2987)

96c4782

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com> (cherry picked from commit 85b4c67) Co-authored-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(*) delete CLI flag '--bootstrap-version' #2965

chore(*) delete CLI flag '--bootstrap-version' #2965

lobkovilya commented Oct 18, 2021

codecov-commenter commented Oct 18, 2021 •

edited

michaelbeaumont Oct 18, 2021 •

edited

jpeach Oct 18, 2021

lobkovilya Oct 19, 2021

michaelbeaumont Oct 20, 2021

michaelbeaumont Oct 18, 2021 •

edited

lobkovilya Oct 19, 2021

jpeach Oct 18, 2021

jpeach left a comment

lobkovilya commented Oct 21, 2021

chore(*) delete CLI flag '--bootstrap-version' #2965

chore(*) delete CLI flag '--bootstrap-version' #2965

Conversation

lobkovilya commented Oct 18, 2021

Summary

Full changelog

Issues resolved

Documentation

Testing

Backwards compatibility

codecov-commenter commented Oct 18, 2021 • edited

Codecov Report

michaelbeaumont Oct 18, 2021 • edited

Choose a reason for hiding this comment

jpeach Oct 18, 2021

Choose a reason for hiding this comment

lobkovilya Oct 19, 2021

Choose a reason for hiding this comment

michaelbeaumont Oct 20, 2021

Choose a reason for hiding this comment

michaelbeaumont Oct 18, 2021 • edited

Choose a reason for hiding this comment

lobkovilya Oct 19, 2021

Choose a reason for hiding this comment

jpeach Oct 18, 2021

Choose a reason for hiding this comment

jpeach left a comment

Choose a reason for hiding this comment

lobkovilya commented Oct 21, 2021

codecov-commenter commented Oct 18, 2021 •

edited

michaelbeaumont Oct 18, 2021 •

edited

michaelbeaumont Oct 18, 2021 •

edited