Skip to content

Fix backword-compatibility issue of non-versioned config file#5359

Merged
estesp merged 1 commit intocontainerd:masterfrom
ktock:v1cfg
Apr 15, 2021
Merged

Fix backword-compatibility issue of non-versioned config file#5359
estesp merged 1 commit intocontainerd:masterfrom
ktock:v1cfg

Conversation

@ktock
Copy link
Copy Markdown
Member

@ktock ktock commented Apr 14, 2021

Related: #5335

According to the doc about config.toml of containerd:

If no version number is specified inside the config file then it is assumed to be a version 1 config and parsed as such.

However, it's not true recently.

# cat <<EOF > /tmp/config.toml
[plugins]
  [plugins.overlayfs]
    root_path = "/dummy"
EOF
# containerd --config=/tmp/config.toml
containerd: failed to load TOML from /tmp/config.toml: invalid plugin key URI "overlayfs" expect io.containerd.x.vx

This will break the backward-compatibility in some environment (e.g. stargz-snapshotter project CI).
This commit fixes this issue.

cc @dims @AkihiroSuda @mikebrow @kzys

@ktock
Copy link
Copy Markdown
Member Author

ktock commented Apr 14, 2021

Please tell me if we should fix the doc (not the validation implementation).

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 14, 2021

Build succeeded.

@AkihiroSuda AkihiroSuda added this to the 1.5 milestone Apr 14, 2021
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Apr 14, 2021

Can we have a test?

@ktock
Copy link
Copy Markdown
Member Author

ktock commented Apr 14, 2021

Can we have a test?

@AkihiroSuda Thanks for the review.
Added a test.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 14, 2021

Build succeeded.

dims
dims approved these changes Apr 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

@ktock May i recommend fixing the config.toml in the broken CI job to be bumped to version 2 as well? :)

@ktock
Copy link
Copy Markdown
Member Author

ktock commented Apr 14, 2021

May i recommend fixing the config.toml in the broken CI job to be bumped to version 2 as well? :)

@dims We've already switched to v2 config by containerd/stargz-snapshotter#302 (just after we found the issue) ! 😄

@dims
Copy link
Copy Markdown
Member

dims commented Apr 14, 2021

@ktock Excellent Kohei!

mikebrow
mikebrow approved these changes Apr 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM
just a nit to add a comment on the Version setting

Comment thread services/server/config/config_test.go Outdated
Copy link
Copy Markdown
Member

@mikebrow mikebrow Apr 14, 2021

Choose a reason for hiding this comment

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

this test will also pass even if platformAgnosticDefaultConfig() returns Version to 2

But it is a good test to ensure v1 will pass v2 validate by it returning nil..

hmm.. let's also add a comment where the default config Version is defined explaining why it should not be changed... I'll suggest a comment above

Comment thread cmd/containerd/command/config.go Outdated
Copy link
Copy Markdown
Member

@mikebrow mikebrow Apr 14, 2021

Choose a reason for hiding this comment

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

suggest adding a comment here something like

// see: https://github.com/containerd/containerd/blob/5c6ea7fdc1247939edaddb1eba62a94527418687/RELEASES.md#daemon-configuration  
// this version MUST remain set to 1 until either there exists a means to override / configure the default at the containerd cli .. or when 
// version 1 is no longer supported

Copy link
Copy Markdown
Member Author

@ktock ktock Apr 15, 2021

Choose a reason for hiding this comment

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

Thanks for the review.
Added the suggested comment.

@ktock
Fix backword-compatibility issue of non-versioned config file
fdb76f5 
According to the doc about `config.toml` of containerd:

```
If no version number is specified inside the config file then it is assumed to
be a version 1 config and parsed as such.
```

However, it's not true recently.
This will break the backward-compatibility in some environment.
This commit fixes this issue.

Signed-off-by: Kohei Tokunaga <[email protected]>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 15, 2021

Build succeeded.

@ktock
Copy link
Copy Markdown
Member Author

ktock commented Apr 15, 2021

All green.

estesp
estesp approved these changes Apr 15, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 88b09e6 into containerd:master Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dims dims dims approved these changes

@estesp estesp estesp approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees

No one assigned

Labels

kind/bug priority/P0

Projects

None yet

Milestone

1.5

Development

Successfully merging this pull request may close these issues.

5 participants

@ktock @AkihiroSuda @dims @estesp @mikebrow