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

Add plugincontainer module #84

Merged
merged 23 commits into from
Aug 29, 2023
Merged

Add plugincontainer module #84

merged 23 commits into from
Aug 29, 2023

Conversation

tomhjp
Copy link
Contributor

@tomhjp tomhjp commented Aug 18, 2023

Follows up on the new interfaces added in hashicorp/go-plugin#270.

Apologies for the size, although a lot of it is generated/license code. Happy to take feedback on splitting this up into multiple PRs if preferred. Maybe the example could be added separately for instance, but it does make for quite a nice end to end integration test of the feature.

tomhjp added 3 commits August 21, 2023 17:50
* Also updates go-plugin to latest from main
* Adds some tests
* Tweaks docs
@tomhjp
Copy link
Contributor Author

tomhjp commented Aug 23, 2023

I think this is in a decent state to review now. I'm planning to tag go-plugin and pull that tag in once there's rough consensus on this PR, just in case there are any last minute API changes required in go-plugin.

@tomhjp tomhjp marked this pull request as ready for review August 23, 2023 20:08
@tomhjp
Copy link
Contributor Author

tomhjp commented Aug 23, 2023

Note: Looks like the CI gives a green tick even if the build and tests fail: https://github.com/hashicorp/go-secure-stdlib/actions/runs/5956014581/job/16155887975. I'll look into fixing this in a separate PR tomorrow.

Verified

This commit was signed with the committer’s verified signature.
zorenkonte Rnz Brngn (zxreign)
fairclothjm
fairclothjm previously approved these changes Aug 24, 2023
Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

LGTM!

tomhjp added 4 commits August 24, 2023 18:14

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@tomhjp
Copy link
Contributor Author

tomhjp commented Aug 24, 2023

Sorry for the commit noise, I accidentally pushed a merge with my CI branch, and chose to revert the merge commit rather than rebase so that it's easier to see changes since last review.

@tomhjp
Copy link
Contributor Author

tomhjp commented Aug 24, 2023

I think this is relatively stable now, but I'm yet to finish the Vault changes leveraging this PR, so I might discover some more requirements. There also seems to be a problem with the tests failing in CI, but they work on my machine, so that may take a bit of time to unpick. (Failing CI link with #85 merged in)

tomhjp added 4 commits August 25, 2023 15:29
Set Follow: true for container logs, without which there was an inadvertent race condition
that normally succeeded on my laptop but failed on CI. The plugin had to have already printed
its protocol negotiation line on stdout by the time we started reading logs or else the
logging function would get to the end of the logs at that time and return, causing go-plugin
to pick up an empty string and say 'negotiation failed'. This was a pig to track down.

Also adds some extra debug/trace logs to shed light on the plugin lifecycle.

Finally, disable cgo when building the plugin for tests, so that it successfully runs in the
static distroless image where libc is not available.
Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

Approving from a repo coordinator perspective, thanks for keeping me in the loop 👍

Copy link

@briankassouf briankassouf left a comment

Choose a reason for hiding this comment

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

This looks fantastic!

@tomhjp
Copy link
Contributor Author

tomhjp commented Aug 29, 2023

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants