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

docs: Drop sphinxcontrib-openapi fork, switch back to upstream #23118

Merged
merged 1 commit into from
Feb 4, 2023

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jan 16, 2023

Once upon a time, Cilium docs used the openapi Sphinx add-on to generate its API reference based on the code. And things were good.

One day, Dependabot raised a security alert, stating that Mistune v2.0.2 was vulnerable to catastrophic backtracking - this is a regex parsing thing. Mistune was a dependency to m2r, an add-on to parse Markdown in Sphinx, which in turn was a dependency to openapi.

The easy path would have been to update m2r to use the latest, fixed Mistune version; but m2r was incompatible with Mistune >= 2.0.0, and also it was no longer in development.

There was a fork, m2r2, which had little activity, and would avoid the security issue by very simply pinning the Mistune version to 0.8.4 (which would either fail to build Cilium's reference correctly, or bring some incompatibilities with other dependencies, at this point the narrator does not remember for sure).

There was a fork of the fork, sphinx-mdinclude. We could use that project to update openapi, except that it was not compatible with recent versions of docutils, and that this would cause openapi's test suite to fail to pass.

... So we ended up forking the openapi repository to update the dependency to sphinx-mdinclude locally, and this is what we've been using since last summer. And things were good again.

But things are even better when they go upstream [citation needed]. We also filed the issue for docutils compatibility in sphinx-mdinclude. It was fixed (thanks!). We submitted a PR to have openapi switch to sphinx-mdinclude. It was adjusted (thanks!), merged, and a new tag was created.

Now at last, we can switch back to the upstream version of openapi!
And the build system lived happily ever after.

I did not run make -C Documentation update-requirements, because the resulting changes seemed to break the Netlify preview. I stuck to openapi and bumped sphinx-mdinclude to >= 0.5.2, as required by openapi.

@qmonnet qmonnet requested a review from a team as a code owner January 16, 2023 14:31
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 16, 2023
@qmonnet qmonnet added release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 16, 2023
@qmonnet qmonnet added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Jan 16, 2023
@qmonnet qmonnet marked this pull request as draft January 16, 2023 14:33
@qmonnet
Copy link
Member Author

qmonnet commented Jan 16, 2023

Netlify preview breaks on the change :/.

@maintainer-s-little-helper

This comment was marked as outdated.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jan 16, 2023
@qmonnet qmonnet closed this Jan 16, 2023
@qmonnet qmonnet reopened this Jan 16, 2023
@maintainer-s-little-helper

This comment was marked as outdated.

@qmonnet qmonnet marked this pull request as ready for review January 16, 2023 15:53
@qmonnet qmonnet marked this pull request as draft January 16, 2023 15:53
@maintainer-s-little-helper

This comment was marked as outdated.

@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jan 16, 2023
@qmonnet qmonnet marked this pull request as ready for review January 16, 2023 16:03
@qmonnet qmonnet closed this Jan 16, 2023
@qmonnet qmonnet reopened this Jan 16, 2023
@qmonnet
Copy link
Member Author

qmonnet commented Jan 16, 2023

Struggling to find the latest Netlify preview (I can't log in), but I think the latest build is here, working fine.

@joestringer
Copy link
Member

Do we need a fresh version of the docs-builder image with this? I see that Travis is failing to build docs due to a dependency conflict.

Once upon a time, Cilium docs used the openapi Sphinx add-on to generate
its API reference based on the code. And things were good.

One day, Dependabot raised a security alert, stating that Mistune v2.0.2
was vulnerable to catastrophic backtracking [0] - this is a regex
parsing thing. Mistune was a dependency to m2r, an add-on to parse
Markdown in Sphinx, which in turn was a dependency to openapi.

The easy path would have been to update m2r to use the latest, fixed
Mistune version; but m2r was incompatible with Mistune >= 2.0.0, and
also it was no longer in development.

There was a fork, m2r2, which had little activity, and would avoid the
security issue by very simply pinning the Mistune version to 0.8.4
(which would either fail to build Cilium's reference correctly, or bring
some incompatibilities with other dependencies, at this point the
narrator does not remember for sure).

There was a fork of the fork, sphinx-mdinclude. We could use that
project to update openapi, except that it was not compatible with recent
versions of docutils, and that this would cause openapi's test suite to
fail to pass.

... So we ended up forking the openapi repository to update the
dependency to sphinx-mdinclude locally, and this is what we've been
using since last summer. And things were good again.

But things are even better when they go upstream [citation needed]. We
also filed the issue for docutils compatibility in sphinx-mdinclude [1].
It was fixed (thanks!). We submitted a PR to have openapi switch to
sphinx-mdinclude [2]. It was adjusted (thanks!), merged, and a new tag
was created.

Now at last, we can switch back to the upstream version of openapi!
[And the build system lived happily ever after.]

[0]: GHSA-fw3v-x4f2-v673
[1]: omnilib/sphinx-mdinclude#8
[2]: sphinx-contrib/openapi#127

I did _not_ run `make -C Documentation update-requirements`, because the
resulting changes seemed to break the Netlify preview [3]. I stuck to
openapi and bumped sphinx-mdinclude to >= 0.5.2, as required by openapi.

[3] https://app.netlify.com/sites/docs-cilium-io/deploys/63c55fcc5531c6000838b87c

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
Copy link
Member Author

qmonnet commented Jan 17, 2023

Do we need a fresh version of the docs-builder image with this?

Yes please. It shouldn't be strictly necessary for the docs to keep building, but it would allow us to delete the cilium/openapi fork. I warned the folks who had started to rely on it, they're switching back to upstream as well.

I see that Travis is failing to build docs due to a dependency conflict.

Good catch, thanks! I don't understand why I failed to observe this either locally or in the CI workflow for docs or on the Netlify build 🤔. I bumped sphinx-mdinclude in requirements.txt, let's see how it goes now.

@qmonnet qmonnet added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Jan 17, 2023
@joestringer joestringer merged commit e4889d7 into cilium:master Feb 4, 2023
@joestringer
Copy link
Member

I've triggered a new docs-builder job with these changes, tag 2023-02-04.

@qmonnet qmonnet deleted the pr/openapi-upstream branch February 6, 2023 14:23
@qmonnet
Copy link
Member Author

qmonnet commented Feb 6, 2023

Thanks, I'll send a PR to update the image.

I see that the 1.12 branch is also using the fork, so I'll tag the current PR for backport there. I think it should be safe to backport, although I haven't tested.
Branches v1.11 and older did not switch to the openapi fork.

@pchaigno pchaigno mentioned this pull request Feb 12, 2023
29 tasks
@pchaigno pchaigno added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Feb 12, 2023
@aanm aanm added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Feb 13, 2023
@pchaigno pchaigno mentioned this pull request Feb 15, 2023
15 tasks
@pchaigno pchaigno added backport-pending/1.12 backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed needs-backport/1.12 labels Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants