-
Notifications
You must be signed in to change notification settings - Fork 10
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
Wire up users controller and move internal clients package to pkg #236
Conversation
This PR does the following things: 1. It wires up the users controller to run in v2 mode. 2. It adds the CRDs to the kustomize manifest we tell users to point to when installing CRDs. 3. It moves the internal/client package to pkg/client in anticipation of utilizing factories internally in our acceptance tests 4. It adds a WithUser method on our factory to create clients who authenticate as a given SCRAM-based for authentication verification purposes in tests. 5. It upgrades common-go and helm-charts dependencies to include some fixes After this, the controller should be fully functional/runnable despite the acceptance tests not yet being merged. I will follow up with the acceptance test code that we can opine on the approach since it's introducing the first gherkin-based fully black-box tests.
@@ -93,7 +93,7 @@ func (s *Syncer) sync(ctx context.Context, principal string, rules []redpandav1a | |||
func (s *Syncer) ListACLs(ctx context.Context, principal string) ([]redpandav1alpha2.ACLRule, error) { | |||
describeResponse, err := s.listACLs(ctx, principal) | |||
if err != nil { | |||
return nil, err | |||
return nil, fmt.Errorf("listing ACLs: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I would drop that listing ACLs
as it is duplicated in https://github.com/redpanda-data/redpanda-operator/pull/236/files#diff-8c966a8be080a7f0e6924dd74d9d34d294fe4c3191ed06a0ab144ff8cafba012R75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Something went wrong. Operator v2 e2e tests are failing all over the place. |
So, not entirely sure how we can fix this without doing a helm release or hacking in some additional stuff for our v2 tests. They're failing due to the controllers not starting up properly because of permission issues on users:
The reason being that the test setup phase installs the operator with the latest released version of the helm chart: redpanda-operator/src/go/k8s/kuttl-v2-test.yaml Lines 18 to 21 in 7ac4262
that means that if RBAC permissions change in the helm chart (like they need to here), we can't pass CI until we release the chart. The RBAC PR already landed in the helm-charts repo in redpanda-data/helm-charts#1527, but since it's still unreleased the tests don't see the change. Ideally we'd be installing the helm chart from the tip of the main branch and/or locally (in a monorepo). |
This PR does the following things:
After this, the controller should be fully functional/runnable despite the acceptance tests not yet being merged. I will follow up with the acceptance test code that we can opine on the approach since it's introducing the first gherkin-based fully black-box tests.