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.

Sign up for GitHub

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

Jump to bottom

Add missing roles for the helm watcher #1328

Merged
merged 5 commits into from
Jan 25, 2022
Merged

Add missing roles for the helm watcher #1328

merged 5 commits into from
Jan 25, 2022

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Jan 21, 2022 •
edited

Closes #1327

TODO: Add acceptance test for cross namespace access.

@Skarlso Skarlso added bug Something isn't working team/pitch-black labels Jan 21, 2022
@Skarlso Skarlso force-pushed the fix_watcher_permissions branch 2 times, most recently from 18b52ea to 0bf7ac2 Compare January 21, 2022 13:20
aclevername
aclevername reviewed Jan 21, 2022
View reviewed changes
manifests/manifests.go Outdated Show resolved Hide resolved
aclevername
aclevername previously requested changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@aclevername aclevername left a comment

Choose a reason for hiding this comment

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

can we get the acceptance test updated to create the HelmRepo in a different namespace to wego, to help cover this issue?

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 21, 2022

Testing

{"level":"info","ts":1642777817.7781508,"logger":"controller.helmrepository","msg":"starting put operation","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"podinfo","namespace":"wego-system","repository":"wego-system/podinfo"}
{"level":"info","ts":1642777817.7794151,"logger":"controller.helmrepository","msg":"finished put operation","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"podinfo","namespace":"wego-system","repository":"wego-system/podinfo"}
{"level":"info","ts":1642777817.7794402,"logger":"controller.helmrepository","msg":"cached data from repository","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"podinfo","namespace":"wego-system","repository":"wego-system/podinfo","url":"http://source-controller.wego-system.svc.cluster.local./helmrepository/wego-system/podinfo/index.yaml","name":"podinfo","number of profiles":1}
{"level":"info","ts":1642777817.789897,"logger":"controller.helmrepository","msg":"found the repository: ","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"cross-test","namespace":"cross-test","repository":"cross-test/cross-test","name":"cross-test"}
{"level":"info","ts":1642777818.185261,"logger":"controller.helmrepository","msg":"starting put operation","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"cross-test","namespace":"cross-test","repository":"cross-test/cross-test"}
{"level":"info","ts":1642777818.1867452,"logger":"controller.helmrepository","msg":"finished put operation","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"cross-test","namespace":"cross-test","repository":"cross-test/cross-test"}
{"level":"info","ts":1642777818.186769,"logger":"controller.helmrepository","msg":"cached data from repository","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"cross-test","namespace":"cross-test","repository":"cross-test/cross-test","url":"http://source-controller.wego-system.svc.cluster.local./helmrepository/cross-test/cross-test/index.yaml","name":"cross-test","number of profiles":1}

Seems to work fine now... implementing the acceptance test next.

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 22, 2022

Finally tested this in cluster. The above test run was a dev run. It doesn't work yet sadly. 😂

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 22, 2022

Fiiixed :)

time="2022-01-22T15:17:17Z" level=info msg="Serving on port 9001"
{"level":"info","ts":1642864638.228255,"logger":"controller-runtime.metrics","msg":"metrics server is starting to listen","addr":":9980"}
{"level":"info","ts":1642864638.2285573,"logger":"setup","msg":"starting manager"}
{"level":"info","ts":1642864638.229215,"msg":"starting metrics server","path":"/metrics"}
{"level":"info","ts":1642864638.2295551,"logger":"controller.helmrepository","msg":"Starting EventSource","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","source":"kind source: /, Kind="}
{"level":"info","ts":1642864638.2299635,"logger":"controller.helmrepository","msg":"Starting Controller","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository"}
{"level":"info","ts":1642864638.3306725,"logger":"controller.helmrepository","msg":"Starting workers","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","worker count":1}
{"level":"info","ts":1642864638.3429396,"logger":"controller.helmrepository","msg":"found the repository: ","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"podinfo","namespace":"wego-system","repository":"wego-system/podinfo","name":"podinfo"}
{"level":"info","ts":1642864638.90731,"logger":"controller.helmrepository","msg":"starting put operation","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"podinfo","namespace":"wego-system","repository":"wego-system/podinfo"}
{"level":"info","ts":1642864638.9083583,"logger":"controller.helmrepository","msg":"finished put operation","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"podinfo","namespace":"wego-system","repository":"wego-system/podinfo"}
{"level":"info","ts":1642864638.9084306,"logger":"controller.helmrepository","msg":"cached data from repository","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"podinfo","namespace":"wego-system","repository":"wego-system/podinfo","url":"http://source-controller.wego-system.svc.cluster.local./helmrepository/wego-system/podinfo/index.yaml","name":"podinfo","number of profiles":1}
{"level":"info","ts":1642864689.9879515,"logger":"controller.helmrepository","msg":"found the repository: ","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"cross-test","namespace":"cross-test","repository":"cross-test/cross-test","name":"cross-test"}
{"level":"info","ts":1642864690.431456,"logger":"controller.helmrepository","msg":"starting put operation","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"cross-test","namespace":"cross-test","repository":"cross-test/cross-test"}
{"level":"info","ts":1642864690.4392412,"logger":"controller.helmrepository","msg":"finished put operation","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"cross-test","namespace":"cross-test","repository":"cross-test/cross-test"}
{"level":"info","ts":1642864690.4394677,"logger":"controller.helmrepository","msg":"cached data from repository","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmRepository","name":"cross-test","namespace":"cross-test","repository":"cross-test/cross-test","url":"http://source-controller.wego-system.svc.cluster.local./helmrepository/cross-test/cross-test/index.yaml","name":"cross-test","number of profiles":1}

I was missing patch verb. :D

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 24, 2022

Running the acceptance test wasn't easy.

I made my own org ( for whatever reason, I couldn't get it to work with users ). Then created a kind launching script with a local registry and edited the deployment yaml file outlined in Jakes ticket here #1158.

Then, finally things worked. I had to edit a bunch of code to use the correct kind launching script.

@Skarlso Skarlso dismissed aclevername’s stale review January 24, 2022 16:02

Added acceptance test.

aclevername
aclevername reviewed Jan 24, 2022
View reviewed changes
manifests/wego-app/helm_watcher_role.yaml.tpl Outdated
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: helm-watcher-role
Copy link
Contributor

Choose a reason for hiding this comment

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

is this too ambiguous? Since its not namespaced maybe worth putting wego- prefix? Idk I don't feel that strongly but eh 🤷

aclevername
aclevername reviewed Jan 24, 2022
View reviewed changes
pkg/services/gitops/uninstall.go
Comment on lines +7 to -10
apierrors "k8s.io/apimachinery/pkg/api/errors"

"github.com/weaveworks/weave-gitops/cmd/gitops/version"
"github.com/weaveworks/weave-gitops/manifests"
"github.com/weaveworks/weave-gitops/pkg/kube"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Copy link
Contributor

Choose a reason for hiding this comment

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

gergely footprint 😆

aclevername
aclevername approved these changes Jan 24, 2022
View reviewed changes
Copy link
Contributor

@aclevername aclevername left a comment

Choose a reason for hiding this comment

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

lgtm couple nits 😄

@Skarlso Skarlso merged commit 1805f68 into main Jan 25, 2022
@Skarlso Skarlso deleted the fix_watcher_permissions branch January 25, 2022 09:34
joshri pushed a commit that referenced this pull request Jan 26, 2022
@Skarlso @joshri
Add missing roles for the helm watcher (#1328)
ec3e318 
* Add missing roles for the helm watcher

* Added missing verbs

* Added acceptnace test

* Renamed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bigkevmcd bigkevmcd bigkevmcd left review comments

@aclevername aclevername aclevername approved these changes

@yiannistri yiannistri Awaiting requested review from yiannistri

@foot foot Awaiting requested review from foot

@nikimanoledaki nikimanoledaki Awaiting requested review from nikimanoledaki

Assignees
No one assigned
Labels
bug Something isn't working team/pitch-black
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix missing cluster roles and service account for the helm watcher
3 participants
@Skarlso @bigkevmcd @aclevername