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 Redhat Openshift support #912

Merged
merged 6 commits into from
Apr 4, 2023
Merged

Add Redhat Openshift support #912

merged 6 commits into from
Apr 4, 2023

Conversation

hi-im-aren
Copy link
Member

@hi-im-aren hi-im-aren commented Dec 15, 2022

Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
Issues #795
License Apache 2.0

What's in this PR?

Add openshift support by:

  • adding finalizer rbac
  • changing default webhook port from protected 443 to 9443

Checklist

ToDo

@hi-im-aren hi-im-aren marked this pull request as ready for review February 7, 2023 07:30
@hi-im-aren hi-im-aren requested a review from a team as a code owner February 7, 2023 07:30
@dobrerazvan
Copy link

Yeah, those seem to be the changes required for rbacs to make the operator work (just tried them today). But there are some changes required to the kafka build to not use the root account otherwise the broker attached pvcs cannot be used (permission denied).

panyuenlau
panyuenlau previously approved these changes Feb 8, 2023
Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

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

lgtm - although I don't have access to OpenShift so can't really play with it myself

@hi-im-aren
Copy link
Member Author

Yeah, those seem to be the changes required for rbacs to make the operator work (just tried them today). But there are some changes required to the kafka build to not use the root account otherwise the broker attached pvcs cannot be used (permission denied).

Yeah I made those changes as well but forgot to create a PR for it in our kafka docker repo, I will add it in a sec.

@pregnor
Copy link
Member

pregnor commented Mar 2, 2023

Thanks for the reviews, I will take over this PR and fix the commented things.

@tiswanso
Copy link

tiswanso commented Mar 23, 2023

fyi, we're also going to need to set runAsUser and runAsGroup in the container security context to match the nonroot ID used in the docker-kafka build.

without that RHOS picks a random UID for this mapping and it screws up any filesystem access (owner doesn't map).

@pregnor
Copy link
Member

pregnor commented Mar 24, 2023

I force-with-lease-pushed the branch to clean up redundant commits I manually rebased, I will fix the comments in subsequent pushes, stay tuned.

@pregnor pregnor force-pushed the openshift-support branch 2 times, most recently from 1df5df1 to 3cb22dd Compare March 24, 2023 13:46
@pregnor
Copy link
Member

pregnor commented Mar 24, 2023

fyi, we're also going to need to set runAsUser and runAsGroup in the container security context to match the nonroot ID used in the docker-kafka build.

without that RHOS picks a random UID for this mapping and it screws up any filesystem access (owner doesn't map).

Fixed in c32648a.

Also I will highlight this in the docs, because it is KafkaCluster CR config details the user needs to pay attention to.

I will retest Koperator using banzaicloud/docker-kafka#29 and #912 on K8s, RHOS 4.10 and 4.11 and if everything seems to work fine I will rerequest the reviews so we can get these merged ASAP.

@pregnor pregnor force-pushed the openshift-support branch from c32648a to a62b459 Compare March 24, 2023 14:17
@pregnor pregnor marked this pull request as draft March 24, 2023 15:57
@tiswanso
Copy link

for the koperator Dockerfile, I think the distroless base image should change to gcr.io/distroless/static-debian11:nonroot

and possibly need to ensure the deployment of koperator sets the securitycontext:

  # capabilities:
  #   drop:
  #   - ALL
  # readOnlyRootFilesystem: true
  # runAsNonRoot: true

@pregnor
Copy link
Member

pregnor commented Mar 24, 2023

Converted to draft, because there is still stuff to figure out unfortunately, working on it.

@tiswanso
Copy link

I think this needs to change to webhook-server as well for the test instructions to work:

@pregnor pregnor force-pushed the openshift-support branch from 5410bdb to 8c7f653 Compare March 28, 2023 11:31
@pregnor
Copy link
Member

pregnor commented Mar 28, 2023

I think this needs to change to webhook-server as well for the test instructions to work:

I mean this will always be outdated if the user overconfigures it, but pushed an update in https://github.com/banzaicloud/koperator/compare/5410bdb1a28dbfa881301952ac7a350dad91f437..8c7f6536849a049fc95e8e92b93746776945b3f7.

(Also removed the commit for the user/group specification, because this way it works on RHOS 4.10 and 4.11 and it doesn't with those and we need to figure out the correct combination of configuration for those with the init containers as well which takes more time but we also want to release the basic RHOS support to move forward.)

@pregnor pregnor marked this pull request as ready for review March 28, 2023 11:36
@pregnor pregnor force-pushed the openshift-support branch from 8c7f653 to f7afeb0 Compare March 28, 2023 17:21
@pregnor
Copy link
Member

pregnor commented Apr 3, 2023

For the external listeners with envoy ingress I added the podSecurityContext propagation which is needed to run fine.
With this we should have covered all basic use-cases.

This last one will build on top of an API change that will be merged in #956 , I will update this PR once that is merged and the new API is tagged. That is what breaks the CI now.

@pregnor pregnor force-pushed the openshift-support branch from 89a8fba to ee1eb4d Compare April 3, 2023 23:15
pregnor
pregnor previously approved these changes Apr 3, 2023
@pregnor pregnor requested review from panyuenlau, tiswanso, amuraru, dobrerazvan and a team and removed request for amuraru, tiswanso and dobrerazvan April 3, 2023 23:18
Kuvesz
Kuvesz previously approved these changes Apr 4, 2023
hi-im-aren and others added 5 commits April 4, 2023 14:42
RHOS requires ports to be over 1024.
To decouple service ports from container ports.
So finalizer RBACs would be covered for create,
delete, patch, update.
Required for RHOS.

After changing the controller markers the
manifests were regenerated using
`make manifests`.
RHOS requires the propagation of the envoy config
podSecurityContext to set uid/gid.
@pregnor pregnor dismissed stale reviews from Kuvesz and themself via b5b1437 April 4, 2023 12:42
@pregnor pregnor force-pushed the openshift-support branch from ee1eb4d to b5b1437 Compare April 4, 2023 12:42
Required to be able to use the envoy
podSecurityContext.
@pregnor pregnor force-pushed the openshift-support branch from cb0cf90 to 99c3b7a Compare April 4, 2023 13:19
@pregnor
Copy link
Member

pregnor commented Apr 4, 2023

I added the new Koperator API version containing the envoy podSecurityContext, this is the final form of this PR.

@pregnor pregnor merged commit 5c78148 into master Apr 4, 2023
@pregnor pregnor deleted the openshift-support branch April 4, 2023 15:46
@panyuenlau panyuenlau mentioned this pull request Aug 29, 2023
2 tasks
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.

8 participants