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

Allow extra manually added pull secrets to managed SA #418

Merged
merged 11 commits into from
Sep 25, 2023

Conversation

eryalito
Copy link
Contributor

@eryalito eryalito commented Aug 31, 2023

This PR fixes #417

Checklist

  • I have signed the CLA
  • I have updated/added any relevant documentation

Description

What's the goal of this PR?

Change the default behaviour of service account pull secrets to support adding extra ones, supporting global pull secrets on OKD/OCP and the ones manually added

What changes did you make?

The SA matching logic is updated so it matches SAs with the same metadata and if all of the desired pull secrets are present on the existing SA.

What alternative solution should we consider, if any?

@CLAassistant
Copy link

CLAassistant commented Aug 31, 2023

CLA assistant check
All committers have signed the CLA.

@eryalito eryalito changed the title Sa pull secret Allow extra manually added pull secrets to managed SA Sep 5, 2023
Copy link
Member

@sudermanjr sudermanjr left a comment

Choose a reason for hiding this comment

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

First glance looks good, need to test more thoroughly. Would you consider adding some tests to the end-to-end testing as well?

@eryalito
Copy link
Contributor Author

eryalito commented Sep 7, 2023

Hey @sudermanjr, I've rearranged the e2e tests a bit so I think it kinda make more sense now there are multiple tests. Also I take the liberty to bump the yq version up so I can use it into the e2e test to analyze the SA manifests.

This is my first time doing this kind of e2e test, so if there are any further adjustments needed or if something isn't quite right, please just let me know so I can fix it.

@eryalito
Copy link
Contributor Author

eryalito commented Sep 8, 2023

forgot to commit the yq fix on the pre script, sorry

@eryalito
Copy link
Contributor Author

I think now it should be fixed for good, but I can not run the whole e2e tests. Is there any way to run this locally to not be waiting for the CI to be enabled?

@sudermanjr
Copy link
Member

sudermanjr commented Sep 18, 2023

You can definitely run the tests using Venom against a local cluster. I don't remember exactly how these get run, so you might have to check the scripts in the e2e folder to figure out how.

Apparently these tests are a much older format that is just a bash script. Seems like you would have to spin up a kind cluster, built the rbac-manager image, and then use kind load to put that image in the cluster. Then you can run the e2e/test.sh file

@sudermanjr
Copy link
Member

Seems like the e2e/rbacdefinition folder isn't getting copied to the "command-runner" container in CircleCI. I think you'll need an additional docker cp command in the e2e/pre.sh

@eryalito
Copy link
Contributor Author

yeah, you're right! I was thinking it was the BASE_DIR variable but it was properly resolved

@sudermanjr
Copy link
Member

sudermanjr commented Sep 19, 2023

That got a lot closer. Looks like just a couple tweaks to the yq commands now

looks like the version of yq in the command runner is being overwritten somewhere:

root@e2e-control-plane:/rbacdefinition# yq --version
yq version 4.8.0
root@e2e-control-plane:/rbacdefinition# which yq
/usr/local/bin/yq

Edit: looks like we're only installing yq in the pre.sh, which is done before the script that runs on the command-runner. The version we see in the comand runner comes from rok8s-scripts. Possibly just an update to change the rok8s reference in the circle config to @13 and a ci-images change to v13 will help. https://github.com/FairwindsOps/rok8s-scripts/blob/b1bfc862c5ad0516c71d27452f257dbc19286c0b/bin/install-rok8s-requirements#L151

Happy to do that in a separate PR, but if you want to do it we can just lump all this together.

@eryalito
Copy link
Contributor Author

I've updated the versions. I'm not sure that this is the problem now, but it's fine by me to do it in this PR

@sudermanjr
Copy link
Member

LGTM. Thanks!

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.

SA creating-deleting loop on OpenShift/OKD
3 participants