From 58cc275eb8365461876a0b08fbb7d03c866aa67f Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 1 Nov 2023 15:16:14 -0700 Subject: [PATCH 1/4] cri: add ability to emit deprecation warnings Signed-off-by: Samuel Karp --- pkg/cri/config/config.go | 23 +++++++++++++---------- pkg/cri/config/config_test.go | 10 +++++++++- pkg/cri/cri.go | 25 ++++++++++++++++++------- 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/pkg/cri/config/config.go b/pkg/cri/config/config.go index 038ad19f5..26cecdd01 100644 --- a/pkg/cri/config/config.go +++ b/pkg/cri/config/config.go @@ -24,6 +24,8 @@ import ( "time" "github.com/containerd/log" + + "github.com/containerd/containerd/v2/pkg/deprecation" ) type SandboxControllerMode string @@ -365,22 +367,23 @@ const ( ) // ValidatePluginConfig validates the given plugin configuration. -func ValidatePluginConfig(ctx context.Context, c *PluginConfig) error { +func ValidatePluginConfig(ctx context.Context, c *PluginConfig) ([]deprecation.Warning, error) { + var warnings []deprecation.Warning if c.ContainerdConfig.Runtimes == nil { c.ContainerdConfig.Runtimes = make(map[string]Runtime) } // Validation for default_runtime_name if c.ContainerdConfig.DefaultRuntimeName == "" { - return errors.New("`default_runtime_name` is empty") + return warnings, errors.New("`default_runtime_name` is empty") } if _, ok := c.ContainerdConfig.Runtimes[c.ContainerdConfig.DefaultRuntimeName]; !ok { - return fmt.Errorf("no corresponding runtime configured in `containerd.runtimes` for `containerd` `default_runtime_name = \"%s\"", c.ContainerdConfig.DefaultRuntimeName) + return warnings, fmt.Errorf("no corresponding runtime configured in `containerd.runtimes` for `containerd` `default_runtime_name = \"%s\"", c.ContainerdConfig.DefaultRuntimeName) } for k, r := range c.ContainerdConfig.Runtimes { if !r.PrivilegedWithoutHostDevices && r.PrivilegedWithoutHostDevicesAllDevicesAllowed { - return errors.New("`privileged_without_host_devices_all_devices_allowed` requires `privileged_without_host_devices` to be enabled") + return warnings, errors.New("`privileged_without_host_devices_all_devices_allowed` requires `privileged_without_host_devices` to be enabled") } // If empty, use default podSandbox mode if len(r.Sandboxer) == 0 { @@ -392,7 +395,7 @@ func ValidatePluginConfig(ctx context.Context, c *PluginConfig) error { useConfigPath := c.Registry.ConfigPath != "" if len(c.Registry.Mirrors) > 0 { if useConfigPath { - return errors.New("`mirrors` cannot be set when `config_path` is provided") + return warnings, errors.New("`mirrors` cannot be set when `config_path` is provided") } log.G(ctx).Warning("`mirrors` is deprecated, please use `config_path` instead") } @@ -406,7 +409,7 @@ func ValidatePluginConfig(ctx context.Context, c *PluginConfig) error { auth := auth u, err := url.Parse(endpoint) if err != nil { - return fmt.Errorf("failed to parse registry url %q from `registry.auths`: %w", endpoint, err) + return warnings, fmt.Errorf("failed to parse registry url %q from `registry.auths`: %w", endpoint, err) } if u.Scheme != "" { // Do not include the scheme in the new registry config. @@ -422,22 +425,22 @@ func ValidatePluginConfig(ctx context.Context, c *PluginConfig) error { // Validation for stream_idle_timeout if c.StreamIdleTimeout != "" { if _, err := time.ParseDuration(c.StreamIdleTimeout); err != nil { - return fmt.Errorf("invalid stream idle timeout: %w", err) + return warnings, fmt.Errorf("invalid stream idle timeout: %w", err) } } // Validation for image_pull_progress_timeout if c.ImagePullProgressTimeout != "" { if _, err := time.ParseDuration(c.ImagePullProgressTimeout); err != nil { - return fmt.Errorf("invalid image pull progress timeout: %w", err) + return warnings, fmt.Errorf("invalid image pull progress timeout: %w", err) } } // Validation for drain_exec_sync_io_timeout if c.DrainExecSyncIOTimeout != "" { if _, err := time.ParseDuration(c.DrainExecSyncIOTimeout); err != nil { - return fmt.Errorf("invalid `drain_exec_sync_io_timeout`: %w", err) + return warnings, fmt.Errorf("invalid `drain_exec_sync_io_timeout`: %w", err) } } - return nil + return warnings, nil } diff --git a/pkg/cri/config/config_test.go b/pkg/cri/config/config_test.go index 5c021857e..c7086bcf6 100644 --- a/pkg/cri/config/config_test.go +++ b/pkg/cri/config/config_test.go @@ -21,6 +21,8 @@ import ( "testing" "github.com/stretchr/testify/assert" + + "github.com/containerd/containerd/v2/pkg/deprecation" ) func TestValidateConfig(t *testing.T) { @@ -28,6 +30,7 @@ func TestValidateConfig(t *testing.T) { config *PluginConfig expectedErr string expected *PluginConfig + warnings []deprecation.Warning }{ "no default_runtime_name": { config: &PluginConfig{}, @@ -143,13 +146,18 @@ func TestValidateConfig(t *testing.T) { }, } { t.Run(desc, func(t *testing.T) { - err := ValidatePluginConfig(context.Background(), test.config) + w, err := ValidatePluginConfig(context.Background(), test.config) if test.expectedErr != "" { assert.Contains(t, err.Error(), test.expectedErr) } else { assert.NoError(t, err) assert.Equal(t, test.expected, test.config) } + if len(test.warnings) > 0 { + assert.ElementsMatch(t, test.warnings, w) + } else { + assert.Len(t, w, 0) + } }) } } diff --git a/pkg/cri/cri.go b/pkg/cri/cri.go index c64d7b8dc..e85547e68 100644 --- a/pkg/cri/cri.go +++ b/pkg/cri/cri.go @@ -21,20 +21,21 @@ import ( "fmt" "path/filepath" - containerd "github.com/containerd/containerd/v2/client" - "github.com/containerd/containerd/v2/pkg/cri/nri" - "github.com/containerd/containerd/v2/pkg/cri/server" - nriservice "github.com/containerd/containerd/v2/pkg/nri" - "github.com/containerd/containerd/v2/platforms" - "github.com/containerd/containerd/v2/plugins" "github.com/containerd/log" "github.com/containerd/plugin" "github.com/containerd/plugin/registry" imagespec "github.com/opencontainers/image-spec/specs-go/v1" "k8s.io/klog/v2" + containerd "github.com/containerd/containerd/v2/client" criconfig "github.com/containerd/containerd/v2/pkg/cri/config" "github.com/containerd/containerd/v2/pkg/cri/constants" + "github.com/containerd/containerd/v2/pkg/cri/nri" + "github.com/containerd/containerd/v2/pkg/cri/server" + nriservice "github.com/containerd/containerd/v2/pkg/nri" + "github.com/containerd/containerd/v2/platforms" + "github.com/containerd/containerd/v2/plugins" + "github.com/containerd/containerd/v2/services/warning" ) // Register CRI service plugin @@ -48,6 +49,7 @@ func init() { plugins.EventPlugin, plugins.ServicePlugin, plugins.NRIApiPlugin, + plugins.WarningPlugin, }, InitFn: initCRIService, }) @@ -58,8 +60,17 @@ func initCRIService(ic *plugin.InitContext) (interface{}, error) { ic.Meta.Exports = map[string]string{"CRIVersion": constants.CRIVersion} ctx := ic.Context pluginConfig := ic.Config.(*criconfig.PluginConfig) - if err := criconfig.ValidatePluginConfig(ctx, pluginConfig); err != nil { + if warnings, err := criconfig.ValidatePluginConfig(ctx, pluginConfig); err != nil { return nil, fmt.Errorf("invalid plugin config: %w", err) + } else if len(warnings) > 0 { + ws, err := ic.GetSingle(plugins.WarningPlugin) + if err != nil { + return nil, err + } + warn := ws.(warning.Service) + for _, w := range warnings { + warn.Emit(ctx, w) + } } c := criconfig.Config{ From d7cb25d770cc6b600fa51fbdd725936109e93fb7 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 1 Nov 2023 17:20:24 -0700 Subject: [PATCH 2/4] cri: add deprecation warning for mirrors Signed-off-by: Samuel Karp --- pkg/cri/config/config.go | 1 + pkg/cri/config/config_test.go | 31 +++++++++++++++++++++++++++++++ pkg/deprecation/deprecation.go | 4 ++++ 3 files changed, 36 insertions(+) diff --git a/pkg/cri/config/config.go b/pkg/cri/config/config.go index 26cecdd01..9dbb64f3d 100644 --- a/pkg/cri/config/config.go +++ b/pkg/cri/config/config.go @@ -397,6 +397,7 @@ func ValidatePluginConfig(ctx context.Context, c *PluginConfig) ([]deprecation.W if useConfigPath { return warnings, errors.New("`mirrors` cannot be set when `config_path` is provided") } + warnings = append(warnings, deprecation.CRIRegistryMirrors) log.G(ctx).Warning("`mirrors` is deprecated, please use `config_path` instead") } diff --git a/pkg/cri/config/config_test.go b/pkg/cri/config/config_test.go index c7086bcf6..502f8b2c5 100644 --- a/pkg/cri/config/config_test.go +++ b/pkg/cri/config/config_test.go @@ -115,6 +115,37 @@ func TestValidateConfig(t *testing.T) { }, expectedErr: "`mirrors` cannot be set when `config_path` is provided", }, + "deprecated mirrors": { + config: &PluginConfig{ + ContainerdConfig: ContainerdConfig{ + DefaultRuntimeName: RuntimeDefault, + Runtimes: map[string]Runtime{ + RuntimeDefault: {}, + }, + }, + Registry: Registry{ + Mirrors: map[string]Mirror{ + "example.com": {}, + }, + }, + }, + expected: &PluginConfig{ + ContainerdConfig: ContainerdConfig{ + DefaultRuntimeName: RuntimeDefault, + Runtimes: map[string]Runtime{ + RuntimeDefault: { + Sandboxer: string(ModePodSandbox), + }, + }, + }, + Registry: Registry{ + Mirrors: map[string]Mirror{ + "example.com": {}, + }, + }, + }, + warnings: []deprecation.Warning{deprecation.CRIRegistryMirrors}, + }, "privileged_without_host_devices_all_devices_allowed without privileged_without_host_devices": { config: &PluginConfig{ ContainerdConfig: ContainerdConfig{ diff --git a/pkg/deprecation/deprecation.go b/pkg/deprecation/deprecation.go index 49fdae884..0f66d2817 100644 --- a/pkg/deprecation/deprecation.go +++ b/pkg/deprecation/deprecation.go @@ -25,12 +25,16 @@ const ( PullSchema1Image Warning = Prefix + "pull-schema-1-image" // GoPluginLibrary is a warning for the use of dynamic library Go plugins GoPluginLibrary Warning = Prefix + "go-plugin-library" + // CRIRegistryMirrors is a warning for the use of the `mirrors` property + CRIRegistryMirrors Warning = Prefix + "cri-registry-mirrors" ) var messages = map[Warning]string{ PullSchema1Image: "Schema 1 images are deprecated since containerd v1.7 and removed in containerd v2.0. " + `Since containerd v1.7.8, schema 1 images are identified by the "io.containerd.image/converted-docker-schema1" label.`, GoPluginLibrary: "Dynamically-linked Go plugins as containerd runtimes are deprecated since containerd v2.0 and removed in containerd v2.1.", + CRIRegistryMirrors: "The `mirrors` property of `[plugins.\"io.containerd.grpc.v1.cri\".registry]` is deprecated since containerd v1.5 and will be removed in containerd v2.0." + + "Use `config_path` instead.", } // Valid checks whether a given Warning is valid From 35924bccc0173af1b8900f6c203155645e76395e Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 1 Nov 2023 17:25:40 -0700 Subject: [PATCH 3/4] cri: add deprecation warning for auths Signed-off-by: Samuel Karp --- pkg/cri/config/config.go | 3 ++- pkg/cri/config/config_test.go | 1 + pkg/deprecation/deprecation.go | 4 ++++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/cri/config/config.go b/pkg/cri/config/config.go index 9dbb64f3d..82c0f7c97 100644 --- a/pkg/cri/config/config.go +++ b/pkg/cri/config/config.go @@ -420,7 +420,8 @@ func ValidatePluginConfig(ctx context.Context, c *PluginConfig) ([]deprecation.W config.Auth = &auth c.Registry.Configs[endpoint] = config } - log.G(ctx).Warning("`auths` is deprecated, please use `configs` instead") + warnings = append(warnings, deprecation.CRIRegistryAuths) + log.G(ctx).Warning("`auths` is deprecated, please use `ImagePullSecrets` instead") } // Validation for stream_idle_timeout diff --git a/pkg/cri/config/config_test.go b/pkg/cri/config/config_test.go index 502f8b2c5..2d7fe1837 100644 --- a/pkg/cri/config/config_test.go +++ b/pkg/cri/config/config_test.go @@ -81,6 +81,7 @@ func TestValidateConfig(t *testing.T) { }, }, }, + warnings: []deprecation.Warning{deprecation.CRIRegistryAuths}, }, "invalid stream_idle_timeout": { config: &PluginConfig{ diff --git a/pkg/deprecation/deprecation.go b/pkg/deprecation/deprecation.go index 0f66d2817..0d5b8b38a 100644 --- a/pkg/deprecation/deprecation.go +++ b/pkg/deprecation/deprecation.go @@ -27,6 +27,8 @@ const ( GoPluginLibrary Warning = Prefix + "go-plugin-library" // CRIRegistryMirrors is a warning for the use of the `mirrors` property CRIRegistryMirrors Warning = Prefix + "cri-registry-mirrors" + // CRIRegistryAuths is a warning for the use of the `auths` property + CRIRegistryAuths Warning = Prefix + "cri-registry-auths" ) var messages = map[Warning]string{ @@ -35,6 +37,8 @@ var messages = map[Warning]string{ GoPluginLibrary: "Dynamically-linked Go plugins as containerd runtimes are deprecated since containerd v2.0 and removed in containerd v2.1.", CRIRegistryMirrors: "The `mirrors` property of `[plugins.\"io.containerd.grpc.v1.cri\".registry]` is deprecated since containerd v1.5 and will be removed in containerd v2.0." + "Use `config_path` instead.", + CRIRegistryAuths: "The `auths` property of `[plugins.\"io.containerd.grpc.v1.cri\".registry]` is deprecated since containerd v1.3 and will be removed in containerd v2.0." + + "Use `ImagePullSecrets` instead.", } // Valid checks whether a given Warning is valid From a596d09ec9c5b919b03e2c49c2521b0150a98006 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 1 Nov 2023 17:34:03 -0700 Subject: [PATCH 4/4] cri: add deprecation warning for configs Signed-off-by: Samuel Karp --- pkg/cri/config/config.go | 5 +++++ pkg/cri/config/config_test.go | 39 ++++++++++++++++++++++++++++++++++ pkg/deprecation/deprecation.go | 4 ++++ 3 files changed, 48 insertions(+) diff --git a/pkg/cri/config/config.go b/pkg/cri/config/config.go index 82c0f7c97..7570f5d6a 100644 --- a/pkg/cri/config/config.go +++ b/pkg/cri/config/config.go @@ -401,6 +401,11 @@ func ValidatePluginConfig(ctx context.Context, c *PluginConfig) ([]deprecation.W log.G(ctx).Warning("`mirrors` is deprecated, please use `config_path` instead") } + if len(c.Registry.Configs) != 0 { + warnings = append(warnings, deprecation.CRIRegistryConfigs) + log.G(ctx).Warning("`configs` is deprecated, please use `config_path` instead") + } + // Validation for deprecated auths options and mapping it to configs. if len(c.Registry.Auths) != 0 { if c.Registry.Configs == nil { diff --git a/pkg/cri/config/config_test.go b/pkg/cri/config/config_test.go index 2d7fe1837..d442dda1f 100644 --- a/pkg/cri/config/config_test.go +++ b/pkg/cri/config/config_test.go @@ -147,6 +147,45 @@ func TestValidateConfig(t *testing.T) { }, warnings: []deprecation.Warning{deprecation.CRIRegistryMirrors}, }, + "deprecated configs": { + config: &PluginConfig{ + ContainerdConfig: ContainerdConfig{ + DefaultRuntimeName: RuntimeDefault, + Runtimes: map[string]Runtime{ + RuntimeDefault: {}, + }, + }, + Registry: Registry{ + Configs: map[string]RegistryConfig{ + "gcr.io": { + Auth: &AuthConfig{ + Username: "test", + }, + }, + }, + }, + }, + expected: &PluginConfig{ + ContainerdConfig: ContainerdConfig{ + DefaultRuntimeName: RuntimeDefault, + Runtimes: map[string]Runtime{ + RuntimeDefault: { + Sandboxer: string(ModePodSandbox), + }, + }, + }, + Registry: Registry{ + Configs: map[string]RegistryConfig{ + "gcr.io": { + Auth: &AuthConfig{ + Username: "test", + }, + }, + }, + }, + }, + warnings: []deprecation.Warning{deprecation.CRIRegistryConfigs}, + }, "privileged_without_host_devices_all_devices_allowed without privileged_without_host_devices": { config: &PluginConfig{ ContainerdConfig: ContainerdConfig{ diff --git a/pkg/deprecation/deprecation.go b/pkg/deprecation/deprecation.go index 0d5b8b38a..9b85e2d0b 100644 --- a/pkg/deprecation/deprecation.go +++ b/pkg/deprecation/deprecation.go @@ -29,6 +29,8 @@ const ( CRIRegistryMirrors Warning = Prefix + "cri-registry-mirrors" // CRIRegistryAuths is a warning for the use of the `auths` property CRIRegistryAuths Warning = Prefix + "cri-registry-auths" + // CRIRegistryConfigs is a warning for the use of the `configs` property + CRIRegistryConfigs Warning = Prefix + "cri-registry-configs" ) var messages = map[Warning]string{ @@ -39,6 +41,8 @@ var messages = map[Warning]string{ "Use `config_path` instead.", CRIRegistryAuths: "The `auths` property of `[plugins.\"io.containerd.grpc.v1.cri\".registry]` is deprecated since containerd v1.3 and will be removed in containerd v2.0." + "Use `ImagePullSecrets` instead.", + CRIRegistryConfigs: "The `configs` property of `[plugins.\"io.containerd.grpc.v1.cri\".registry]` is deprecated since containerd v1.5 and will be removed in containerd v2.0." + + "Use `config_path` instead.", } // Valid checks whether a given Warning is valid