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

Skip API operations for diff run #793

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Skip API operations for diff run #793

merged 1 commit into from
Aug 8, 2023

Conversation

vixus0
Copy link
Contributor

@vixus0 vixus0 commented Aug 3, 2023

What this PR does / why we need it:

Diff run should be usable when the authorised RBAC role doesn't have
write permissions for resources in the cluster.

Diff run on app creation used the dry run feature of the Kubernetes
API, to avoid persisting kapp ConfigMap changes. However this still
checks RBAC permissions, and errored if the role did not have write
permissions for ConfigMaps.

The kapp ConfigMap was also being updated when doing a diff run for an
existing app.

This skips the API operations for updating the ConfigMap when doing a
diff run.

Which issue(s) this PR fixes:

Fixes #791

Does this PR introduce a user-facing change?

- Diff run will not check anymore for appropriate RBAC permissions.

Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


@vixus0
Copy link
Contributor Author

vixus0 commented Aug 3, 2023

Hmm, not sure why the new test TestDiffRunReadOnlyContext is failing. On my machine with minikube v1.31.1 (kubernetes v1.27.3):

=== RUN   TestDiffRunReadOnlyContext
Running 'kapp delete -a test-e2e-rbac-app -n kapp-test --yes'...
Running 'kapp delete -a diff-run-read-only -n kapp-test --yes'...
Running 'kapp deploy -a test-e2e-rbac-app -f - -n kapp-test --yes'...
Running 'kubectl get secret scoped-sa -o jsonpath={.data.token} -n kapp-test'...
Running 'kubectl config view --minify -o jsonpath={.clusters[].name} -n kapp-test'...
Running 'kubectl -redacted-'...
Running 'kubectl config set-context scoped-context --user=scoped-user --cluster=minikube'...
==> diff-run using scoped context
Running 'kapp deploy -f - -a diff-run-read-only --diff-run --kubeconfig-context=scoped-context -n kapp-test --into-ns kapp-test --yes'...
Running 'kubectl get configmap diff-run-read-only -n kapp-test -o yaml -n kapp-test'...
Running 'kubectl config delete-context scoped-context -n kapp-test'...
Running 'kubectl config delete-user scoped-user -n kapp-test'...
Running 'kapp delete -a test-e2e-rbac-app -n kapp-test --yes'...
Running 'kapp delete -a diff-run-read-only -n kapp-test --yes'...
--- PASS: TestDiffRunReadOnlyContext (0.52s)

The other two tests (TestFormattedError, TestIgnoreFailingAPIServices) seem to be failing on the develop branch also, but I can investigate further.

@praveenrewar
Copy link
Member

Hey @vixus0! Thank you so much for the PR. Like you mentioned the other 2 tests are failing on develop as well, most probably because of some changes in the latest version of Kubernetes. I will fix them as soon as I get some time.
Regarding the new test, I will have to take a closer look to be sure what the issue is. Is it consistently working for you on your machine?

@vixus0
Copy link
Contributor Author

vixus0 commented Aug 4, 2023

Yeah, I can consistently run the test on my machine. It seems in CI the initial kapp deploy step for actually setting up the scoped role is failing:

kapp: Error: waiting on reconcile secret/scoped-sa (v1) namespace: kapp-test:
  Errored:
    Getting resource secret/scoped-sa (v1) namespace: kapp-test:
      API server says: secrets "scoped-sa" not found (reason: NotFound)

Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

I re ran the tests and the test seems to be passing now (except the other 2, I have created a PR to fix them), I am guessing it was a one time thing, although I am not sure how that error could possible come.
Left some comments.

pkg/kapp/app/recorded_app.go Outdated Show resolved Hide resolved
test/e2e/diff_test.go Outdated Show resolved Hide resolved
test/e2e/diff_test.go Show resolved Hide resolved
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

Looking really good now 🚀
Just left some minor comments.

test/e2e/diff_test.go Outdated Show resolved Hide resolved
test/e2e/diff_test.go Outdated Show resolved Hide resolved
test/e2e/diff_test.go Outdated Show resolved Hide resolved
@praveenrewar
Copy link
Member

The PR to fix the other 2 tests is merged, so you can rebase on those changes now for the tests to pass.

Diff run should be usable when the authorised RBAC role doesn't have
write permissions for resources in the cluster.

Diff run on app creation used the dry run feature of the Kubernetes
API, to avoid persisting kapp ConfigMap changes. However this still
checks RBAC permissions, and errored if the role did not have write
permissions for ConfigMaps.

The kapp ConfigMap was also being updated when doing a diff run for an
existing app.

This skips the API operations for updating the ConfigMap when doing a
diff run.

Fixes #791.

Signed-off-by: Anshul Sirur <[email protected]>
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much for the PR @vixus0 🚀

@praveenrewar praveenrewar merged commit 1725a44 into carvel-dev:develop Aug 8, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

kapp deploy --diff-run should not require updating the existing ConfigMap
2 participants