search

brigadecore / brigade

Event-driven scripting for Kubernetes
https://brigade.sh/
Apache License 2.0
2.4k stars 247 forks source link

Fix setting secrets on a non-existing project tells you're unauthorized #1900

Closed AnuragThePathak closed 2 years ago

AnuragThePathak commented 2 years ago

Signed-off-by: Anurag Pathak anuragpathak911@gmail.com Fixes #1893

What this PR does / why we need it: While setting a secret, instead of directly checking for PROJECT_ADMIN role we'll now be checking if the user is a READER followed by checking if the project exists so that we can avoid getting unauthorized error when the project doesn't exist. Special notes for your reviewer:

If applicable:

netlify[bot] commented 2 years ago

Deploy Preview for brigade-docs ready!

Name Link
Latest commit b52bc6e13954c980cc923cca0f42084b00a90ed9
Latest deploy log https://app.netlify.com/sites/brigade-docs/deploys/62491c8f59de9400083d9365
Deploy Preview https://deploy-preview-1900--brigade-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

AnuragThePathak commented 2 years ago

Should we add unit tests as well?

krancour commented 2 years ago

Should we add unit tests as well?

They haven't run yet, but I'm pretty sure they'll fail if they're not amended.

AnuragThePathak commented 2 years ago

Should I create a new file for the tests of secrets.go file? It doesn't exist right now ig.

Moreover, I was not able to able to run unit tests using make test-unit-go (because of network issue or proxy issue) yesterday. But when I ran tests directly in my system all tests passed. I have tested it locally using make test-unit-go, all test cases passed. So I believe there are no tests for the secrets.

krancour commented 2 years ago

Should I create a new file for the tests of secrets.go file? It doesn't exist right now ig.

Oooh. That's a gap in our testing. Thank you for calling my attention to it.

I'll add those tests today, including ones for the new stuff you've added in this PR and we'll combine our commits together.

AnuragThePathak commented 2 years ago

Should I create the PR from a different branch? I remembered that after committing only so created the PR from main only.

krancour commented 2 years ago

@AnuragThePathak nope. You did everything right.

krancour commented 2 years ago

@AnuragThePathak I opened #1903 to add the missing tests. After that's merged, we can rebase this PR and you can make the incremental changes necessary to the tests to validate the changes you've already made.

Happy to help if you need help rebasing or anything like that.

AnuragThePathak commented 2 years ago

@krancour No problem. Btw does brigade installation fails when we run make hack while using proxy?

I am using kind and have to use proxy because internet speed in the place where my college is located is very poor and LAN connection doesn't work without a proxy.

krancour commented 2 years ago

@AnuragThePathak afaik, Kind and our whole make hack process should work fine as long as Docker is working correctly.

The only thing I highly suggest (even for folks with fast internet) is to use a local Docker registry when you're hacking away. make hack-new-kind-cluster will actually set one up for you and connect the KinD cluster to it, but you do need to set an env var for make hack to use it:

$ export DOCKER_REGISTRY=localhost:5000

If you do that, your internet connection will only ever slow you down the first time base images are pulled and 100% of everything else that happens will happen locally.

Edit: I really need to write some kind of primer about how to develop efficiently on Brigade. Hope to get to it soon.

AnuragThePathak commented 2 years ago

export DOCKER_REGISTRY=localhost:5000

Yeah I did that but still brigade release installation fails with the following statements

Error: timed out waiting for the condition make: *** [Makefile:521: hack-deploy] Error 1

I'm not really sure everything is fine because of that proxy issue.

krancour commented 2 years ago

@AnuragThePathak what state are all the pods in?

$ kubectl get pods -n brigade
AnuragThePathak commented 2 years ago

I can see that the problem is occurring because of not being able to pull local images.

krancour commented 2 years ago

@AnuragThePathak how did you start the KinD cluster?

If you just did kind create cluster, it will lack the necessary configuration to connect to a local registry.

We provide a script that sets everything up correctly. You can kick it off with make hack-new-kind-cluster.

Again... I really need to cook up some kind of primer on all of this, so thank you for your patience.

AnuragThePathak commented 2 years ago

I created the kind cluster using make hack-new-kind-cluster as mentioned in the docs.

It was working perfectly when I was at my home and working without proxy, not sure if it's some proxy issue.

AnuragThePathak commented 2 years ago

I've figured out the issue it was occurring as brigade-dev-registry was not in no proxy. Now again I got stuck in the github OAuth login as localhost is no proxy ig, hence not able to connect to github servers.

krancour commented 2 years ago

@AnuragThePathak I'm not completely understanding your proxy issues. Specifically, I don't understand why individual containers, for instance, would need any special configuration to work with your proxy. Is it not the case that your whole system is configured to use the proxy for all outbound traffic?

Also, you don't need to use OAuth for hacking on Brigade. It's a non-production cluster, so root is enabled. Just log in as root.

AnuragThePathak commented 2 years ago

Is it not the case that your whole system is configured to use the proxy for all outbound traffic?

I think the proxy is not limited to outbound traffic only as we need to add localhost and a few other services to no proxy list.

Also, you don't need to use OAuth for hacking on Brigade. It's a non-production cluster, so root is enabled. Just log in as root.

But I need to do so because of the following reason

This is only true if you're logged in as a real user and not as root.

krancour commented 2 years ago

I think the proxy is not limited to outbound traffic only as we need to add localhost and a few other services to no proxy list.

This sounds strange to me, but I can't get bogged down in troubleshooting your proxy.

But I need to do so because of the following reason

This is only true if you're logged in as a real user and not as root.

Ah. Right.

This is where the beauty and utility of the unit tests comes in. Unit tests exist precisely so you can validate the functionality of a small piece of code without being forced to run the entire system.

AnuragThePathak commented 2 years ago

This is where the beauty and utility of the unit tests comes in. Unit tests exist precisely so you can validate the functionality of a small piece of code without being forced to run the entire system.

Cool. Btw I have updated the tests to which was required with the fix of this issue.

krancour commented 2 years ago

I have updated the tests to which was required with the fix of this issue.

Awesome! Just took a look.

I left one small comment, but all of this looks fantastic.

Once again, thank you so much for this and other recent contributions!

AnuragThePathak commented 2 years ago

Once again, thank you so much for this and other recent contributions!

You're welcome. I personally like this project not only from developer perspective but also from user perspective.

krancour commented 2 years ago

/brig run