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

Net 4414 remove anyuid openshift requirement #4152

Merged
merged 39 commits into from
Jul 9, 2024

Conversation

missylbytes
Copy link
Contributor

@missylbytes missylbytes commented Jun 26, 2024

Changes proposed in this PR

This PR solves two problems:

  • Adds the NET_BIND_SERVICE capability to the Consul Dataplane containers that are created. This work was done in another PR, but has been pulled into this one. Original PR
  • Removes the need on Openshift for using an SCC that allows anyuid. We now use the annotation provided by Openshift on the namespace to set the user and group IDs for our containers. This work was originally done in another PR, but has been pulled into this one and expanded to cover API gateways and use separate user/group IDs for the init + dataplane containers that do not conflict with application user/group IDs. Original PR

How I've tested this PR

Manual testing
Unit tests

How I expect reviewers to test this PR

Follow this gist to setup and install Consul onto Openshift

Checklist

.changelog/3813.txt Outdated Show resolved Hide resolved
@missylbytes missylbytes mentioned this pull request Jul 5, 2024
2 tasks
@missylbytes missylbytes marked this pull request as ready for review July 8, 2024 19:35
@sarahalsmiller
Copy link
Member

Tested on open shift and saw the fix working, after Nathan and Melissa walked me through it. Reviewing the code portion now

@@ -164,4 +164,4 @@ require (
sigs.k8s.io/yaml v1.3.0 // indirect
)

go 1.20
go 1.21
Copy link
Member

Choose a reason for hiding this comment

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

Does 1.22 break?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect it to. The go <version> directive here specifies the minimum Go version required to run the application, not necessarily the version that it will be built with since that's determined by what version of go is installed when you're building.

https://go.dev/ref/mod#go-mod-file-go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also un-bumped go version, should be done in a separate PR

Copy link
Member

@sarahalsmiller sarahalsmiller left a comment

Choose a reason for hiding this comment

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

Approved since I've tested it, but some non blocking questions

@missylbytes missylbytes enabled auto-merge (squash) July 9, 2024 18:33
@missylbytes missylbytes merged commit 2bf6e22 into main Jul 9, 2024
50 checks passed
@missylbytes missylbytes deleted the net-4414-remove-anyuid-openshift-requirement branch July 9, 2024 20:21
nathancoleman added a commit that referenced this pull request Jul 9, 2024
Co-authored-by: Curt Bushko <[email protected]>
Co-authored-by: Nathan Coleman <[email protected]>
nathancoleman added a commit that referenced this pull request Jul 9, 2024
Co-authored-by: Curt Bushko <[email protected]>
Co-authored-by: Nathan Coleman <[email protected]>
sarahalsmiller pushed a commit that referenced this pull request Jul 9, 2024
…/1.5.x (#4161)

Net 4414 remove anyuid openshift requirement (#4152)

Co-authored-by: Melisa Griffin <[email protected]>
Co-authored-by: Curt Bushko <[email protected]>
Co-authored-by: Nathan Coleman <[email protected]>
sarahalsmiller added a commit that referenced this pull request Jul 10, 2024
…/1.4.x (#4160)

* Net 4414 remove anyuid openshift requirement (#4152)

Co-authored-by: Curt Bushko <[email protected]>
Co-authored-by: Nathan Coleman <[email protected]>

* fix missing field int est struct

---------

Co-authored-by: Melisa Griffin <[email protected]>
Co-authored-by: Curt Bushko <[email protected]>
Co-authored-by: Nathan Coleman <[email protected]>
Co-authored-by: Sarah Alsmiller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants