deprecate support to Envoy SDS v2 API #4228

MarcosDY · 2023-06-06T14:07:31Z

Deprecate SDS v2 API, disabling it as default, but add a configurable to reactivate it.

Which issue this PR fixes
fixes #4204

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

rturner3 · 2023-06-06T15:51:18Z

cmd/spire-agent/cli/run/run.go

@@ -496,6 +497,10 @@ func NewAgentConfig(c *Config, logOptions []log.Option, allowUnknownConfig bool)
 	ac.JoinToken = c.Agent.JoinToken
 	ac.DataDir = c.Agent.DataDir
 	ac.DefaultSVIDName = c.Agent.SDS.DefaultSVIDName
+	ac.EnableEnvoySDSv2 = c.Agent.SDS.EnableSDSv2
+	if ac.EnableEnvoySDSv2 {
+		logger.Warn("The support to Envoy SDS v2 API has been deprecated in SPIRE and is no longer supported by Envoy. Enabling SDS v2 is not recommended and this config setting will be removed in a future version.")


Suggested change

logger.Warn("The support to Envoy SDS v2 API has been deprecated in SPIRE and is no longer supported by Envoy. Enabling SDS v2 is not recommended and this config setting will be removed in a future version.")

logger.Warn("The Envoy SDS v2 API is now deprecated in SPIRE and is no longer supported by Envoy. It is recommended that users of the SDS v2 API migrate to the SDS v3 API. The SDS v2 API and this config setting will be removed in a future version.")

Just a few wording suggestions:

has been -> is to make it clear that the current status of the SDS v2 API is that it's deprecated

Rather than saying using the SDS v2 API is not recommended (also implied by saying it's deprecated), point users currently dependent on the SDS v2 API to the supported successor, the SDS v3 API

Clarify that the SDS v2 API itself will also be removed, not just the config. As written, users might interpret that the API will still be available, but only this config field will be removed.

rturner3 · 2023-06-06T15:58:00Z

cmd/spire-agent/cli/run/run.go

@@ -104,6 +104,7 @@ type sdsConfig struct {
 	DefaultBundleName           string `hcl:"default_bundle_name"`
 	DefaultAllBundlesName       string `hcl:"default_all_bundles_name"`
 	DisableSPIFFECertValidation bool   `hcl:"disable_spiffe_cert_validation"`
+	EnableSDSv2                 bool   `hcl:"enable_sds_v2"`


Suggested change

EnableSDSv2 bool `hcl:"enable_sds_v2"`

EnableDeprecatedv2API bool `hcl:"enable_deprecated_v2_api"`

I think it would be good to include the term deprecated into the configurable name to clarify the intent behind the configurable.

We can probably omit the term sds in the name of the configurable since the field is already inside the sds configuration section

rturner3 · 2023-06-06T16:14:57Z

pkg/agent/endpoints/sdsv2/handler.go

@@ -26,6 +26,10 @@ import (
 	"google.golang.org/protobuf/types/known/anypb"
 )

+const (
+	deprecatedAPIErrorMsg = "support to Envoy SDS v2 API has been deprecated in SPIRE and is no longer supported by Envoy. Please refer to: https://www.envoyproxy.io/docs/envoy/latest/api/api_supported_versions. It can be enabled using the config setting 'enable_sds_v2': https://github.com/spiffe/spire/blob/main/doc/spire_agent.md#sds-configuration. Please note that enabling SDS v2 is not recommended and this config setting will be removed in a future version."


Suggested change

deprecatedAPIErrorMsg = "support to Envoy SDS v2 API has been deprecated in SPIRE and is no longer supported by Envoy. Please refer to: https://www.envoyproxy.io/docs/envoy/latest/api/api_supported_versions. It can be enabled using the config setting 'enable_sds_v2': https://github.com/spiffe/spire/blob/main/doc/spire_agent.md#sds-configuration. Please note that enabling SDS v2 is not recommended and this config setting will be removed in a future version."

deprecatedAPIErrorMsg = "The Envoy SDS v2 API is now deprecated in SPIRE and is no longer supported by Envoy. Please refer to: https://www.envoyproxy.io/docs/envoy/latest/api/api_supported_versions. The SDS v2 API can be enabled using the config setting 'enable_deprecated_v2_api': https://github.com/spiffe/spire/blob/main/doc/spire_agent.md#sds-configuration. It is recommended that users of the SDS v2 API migrate to the SDS v3 API. The SDS v2 API and this config setting will be removed in a future version."

it is an error, so I'll change the message start from The to the

rturner3 · 2023-06-06T16:17:57Z

pkg/agent/endpoints/sdsv2/handler.go

@@ -204,6 +213,10 @@ func (h *Handler) DeltaSecrets(discovery_v2.SecretDiscoveryService_DeltaSecretsS
 }


Shall we also return the same Unavailable code for the DeltaSecrets method when the SDS v2 API is not enabled to be consistent across all the methods in the SDS v2 API?

rturner3 · 2023-06-06T16:18:13Z

pkg/agent/endpoints/sdsv2/handler_test.go

@@ -204,6 +206,24 @@ func (s *HandlerSuite) TestStreamSecretsStreamAllSecrets() {
 	s.requireSecrets(resp, tdValidationContext, fedValidationContext, workloadTLSCertificate1)
 }

+func (s *HandlerSuite) TestAPIIsNoEnabled() {


Suggested change

func (s *HandlerSuite) TestAPIIsNoEnabled() {

func (s *HandlerSuite) TestAPIIsNotEnabled() {

rturner3 · 2023-06-06T16:19:04Z

test/integration/suites/envoy-sds-v2/conf/upstream-agent/agent.conf

@@ -6,6 +6,9 @@ agent {
    socket_path ="/opt/shared/agent.sock"
    trust_bundle_path = "/opt/spire/conf/agent/bootstrap.crt"
    trust_domain = "domain.test"
+    sds = {
+        enable_sds_v2 = true


Suggested change

enable_sds_v2 = true

enable_deprecated_sds_v2_api = true

rturner3 · 2023-06-06T16:20:17Z

doc/spire_agent.md

-| `disable_spiffe_cert_validation` | Disable Envoy SDS custom validation                                                              | false   |
+| Configuration                    | Description                                                                                                                         | Default |
+|----------------------------------|-------------------------------------------------------------------------------------------------------------------------------------|---------|
+| `enable_sds_v2`                  | Enable SDS v2 API. Please note that enabling SDS v2 is not recommended and this config setting will be removed in a future version. | false   |


Suggested change

| `enable_sds_v2` | Enable SDS v2 API. Please note that enabling SDS v2 is not recommended and this config setting will be removed in a future version. | false |

| `enable_deprecated_v2_api` | Enable deprecated SDS v2 API. It is recommended that users of the SDS v2 API migrate to the SDS v3 API. The SDS v2 API and this config setting will be removed in a future version. | false |

rturner3 · 2023-06-06T16:23:26Z

cmd/spire-agent/cli/run/run_test.go

@@ -244,6 +244,18 @@ func TestMergeInput(t *testing.T) {
 				require.Equal(t, "foo", c.Agent.SDS.DefaultAllBundlesName)
 			},
 		},
+		{
+			msg: "enable_sds_v2 should be configurable by file",


Suggested change

msg: "enable_sds_v2 should be configurable by file",

msg: "enable_deprecated_v2_api should be configurable by file",

rturner3 · 2023-06-06T16:23:48Z

conf/agent/agent_full.conf

@@ -72,6 +72,10 @@ agent {

    # sds: Optional SDS configuration section.
    # sds = {
+    #     # enable_sds_v2: Enable SDS v2 API. Please note that enabling SDS v2 is not 
+    #     # recommended and this config setting will be removed in a future version. Default: false
+    #     # enable_sds_v2 = false


Suggested change

# # enable_sds_v2 = false

# # enable_deprecated_v2_api = false

rturner3 · 2023-06-06T16:25:55Z

conf/agent/agent_full.conf

+    #     # enable_sds_v2: Enable SDS v2 API. Please note that enabling SDS v2 is not 
+    #     # recommended and this config setting will be removed in a future version. Default: false


Suggested change

# # enable_sds_v2: Enable SDS v2 API. Please note that enabling SDS v2 is not

# # recommended and this config setting will be removed in a future version. Default: false

# # enable_deprecated_v2_api: Enable deprecated SDS v2 API. It is recommended that

# # users of the SDS v2 API migrate to the SDS v3 API.

# # The SDS v2 API and this config setting will be removed in a future version.

# # Default: false

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

* deprecate support to Envoy SDS v2 API Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Neniel <11655196+Neniel@users.noreply.github.com>

deprecate support to Envoy SDS v2 API

27cb330

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

MarcosDY requested review from evan2645, amartinezfayo, azdagron and rturner3 as code owners June 6, 2023 14:07

MarcosDY added this to the 1.7.0 milestone Jun 6, 2023

rturner3 reviewed Jun 6, 2023

View reviewed changes

MarcosDY added 2 commits June 6, 2023 14:41

PR changes

42a3cd8

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

Merge branch 'main' into deprecate-sds-v2

87d7896

rturner3 approved these changes Jun 6, 2023

View reviewed changes

MarcosDY merged commit 9d1dc74 into spiffe:main Jun 6, 2023

MarcosDY deleted the deprecate-sds-v2 branch June 6, 2023 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate support to Envoy SDS v2 API #4228

deprecate support to Envoy SDS v2 API #4228

MarcosDY commented Jun 6, 2023

rturner3 Jun 6, 2023

rturner3 Jun 6, 2023

rturner3 Jun 6, 2023

MarcosDY Jun 6, 2023

rturner3 Jun 6, 2023

rturner3 Jun 6, 2023

rturner3 Jun 6, 2023

rturner3 Jun 6, 2023

rturner3 Jun 6, 2023

rturner3 Jun 6, 2023

rturner3 Jun 6, 2023

	logger.Warn("The support to Envoy SDS v2 API has been deprecated in SPIRE and is no longer supported by Envoy. Enabling SDS v2 is not recommended and this config setting will be removed in a future version.")
	logger.Warn("The Envoy SDS v2 API is now deprecated in SPIRE and is no longer supported by Envoy. It is recommended that users of the SDS v2 API migrate to the SDS v3 API. The SDS v2 API and this config setting will be removed in a future version.")

	EnableSDSv2 bool `hcl:"enable_sds_v2"`
	EnableDeprecatedv2API bool `hcl:"enable_deprecated_v2_api"`

		@@ -204,6 +213,10 @@ func (h *Handler) DeltaSecrets(discovery_v2.SecretDiscoveryService_DeltaSecretsS
		}

	func (s *HandlerSuite) TestAPIIsNoEnabled() {
	func (s *HandlerSuite) TestAPIIsNotEnabled() {

	\| `enable_sds_v2` \| Enable SDS v2 API. Please note that enabling SDS v2 is not recommended and this config setting will be removed in a future version. \| false \|
	\| `enable_deprecated_v2_api` \| Enable deprecated SDS v2 API. It is recommended that users of the SDS v2 API migrate to the SDS v3 API. The SDS v2 API and this config setting will be removed in a future version. \| false \|

	msg: "enable_sds_v2 should be configurable by file",
	msg: "enable_deprecated_v2_api should be configurable by file",

	# # enable_sds_v2 = false
	# # enable_deprecated_v2_api = false

		# # enable_sds_v2: Enable SDS v2 API. Please note that enabling SDS v2 is not
		# # recommended and this config setting will be removed in a future version. Default: false

-    #     # enable_sds_v2: Enable SDS v2 API. Please note that enabling SDS v2 is not
-    #     # recommended and this config setting will be removed in a future version. Default: false
+    #     # enable_deprecated_v2_api: Enable deprecated SDS v2 API. It is recommended that
+    #     # users of the SDS v2 API migrate to the SDS v3 API.
+    #     # The SDS v2 API and this config setting will be removed in a future version.
+    #     # Default: false

deprecate support to Envoy SDS v2 API #4228

deprecate support to Envoy SDS v2 API #4228

Conversation

MarcosDY commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment