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

fix(apis): remove deprecated flightrecorder/recording apis #438

Merged
merged 23 commits into from
Aug 31, 2022

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Aug 11, 2022

Fixes #436
Fixes #387
Related to #430
Depends on https://github.com/cryostatio/cryostat/pull/1042
Depends on #440

Removed deprecated flight/recording apis and their resources.
Updated CSV (i.e. remove deprecated apis and bump operator-sdk version to 1.22.2).

@tthvo tthvo added the fix label Aug 11, 2022
@tthvo
Copy link
Member Author

tthvo commented Aug 15, 2022

Run into:

WARNING: HTTP 401: HTTP Authorization Failure
io.vertx.ext.web.handler.HttpException: Unauthorized
Caused by: java.util.concurrent.ExecutionException: java.util.concurrent.ExecutionException: io.cryostat.net.PermissionDeniedException: Requesting client in namespace "default" cannot get recordings.operator.cryostat.io: scopes [user:check-access role:cryostat-operator-oauth-client:default] prevent this action
	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
	at io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler.handle(AbstractAuthenticatedRequestHandler.java:92)
	at io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler.handle(AbstractAuthenticatedRequestHandler.java:69)
	at io.vertx.ext.web.impl.BlockingHandlerDecorator.lambda$handle$0(BlockingHandlerDecorator.java:48)
	at io.vertx.core.impl.ContextImpl.lambda$null$0(ContextImpl.java:159)
	at io.vertx.core.impl.AbstractContext.dispatch(AbstractContext.java:100)
	at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$1(ContextImpl.java:157)
	at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.util.concurrent.ExecutionException: io.cryostat.net.PermissionDeniedException: Requesting client in namespace "default" cannot get recordings.operator.cryostat.io: scopes [user:check-access role:cryostat-operator-oauth-client:default] prevent this action
	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
	at io.cryostat.net.openshift.OpenShiftAuthManager.validateToken(OpenShiftAuthManager.java:263)
	at io.cryostat.net.openshift.OpenShiftAuthManager.validateHttpHeader(OpenShiftAuthManager.java:359)
	at io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler.validateRequestAuthorization(AbstractAuthenticatedRequestHandler.java:120)
	... 11 more
Caused by: io.cryostat.net.PermissionDeniedException: Requesting client in namespace "default" cannot get recordings.operator.cryostat.io: scopes [user:check-access role:cryostat-operator-oauth-client:default] prevent this action
	at io.cryostat.net.openshift.OpenShiftAuthManager.lambda$validateAction$4(OpenShiftAuthManager.java:337)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.HashMap$KeySpliterator.forEachRemaining(HashMap.java:1707)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
	at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:276)
	at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
	at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1845)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
	at io.cryostat.net.openshift.OpenShiftAuthManager.validateToken(OpenShiftAuthManager.java:260)
	... 13 more

Looking into cryostat OAuth setup for check its rbac resource requirements.

@ebaron
Copy link
Member

ebaron commented Aug 15, 2022

@tthvo oh right. This ends up being blocked by https://github.com/cryostatio/cryostat/issues/979. We'll have to come up with some alternative resource mapping to permissions that do not use these APIs being removed.

@tthvo
Copy link
Member Author

tthvo commented Aug 15, 2022

Oh yehh make senses! I can try looking into that and trace back how we setup OAuth to get a better idea then :D

@github-actions
Copy link

This PR/issue depends on:

@tthvo tthvo force-pushed the remove-deprecated-api branch from 91ac92f to 4af11c4 Compare August 28, 2022 23:02
@ebaron ebaron changed the title fix(apis): remove deprecated flighrecorder/recording apis fix(apis): remove deprecated flightrecorder/recording apis Aug 29, 2022
@tthvo tthvo force-pushed the remove-deprecated-api branch from 4af11c4 to 76604af Compare August 29, 2022 16:57
@tthvo tthvo requested a review from ebaron August 29, 2022 19:03
@tthvo tthvo marked this pull request as ready for review August 29, 2022 19:04
@tthvo tthvo requested a review from andrewazores August 29, 2022 19:04
@tthvo
Copy link
Member Author

tthvo commented Aug 29, 2022

Images available for testing:

quay.io/thvo/cryostat-operator-bundle:0.0.0-depr-api and quay.io/thvo/cryostat-operator:0.0.0-depr-api

@tthvo
Copy link
Member Author

tthvo commented Aug 29, 2022

I believe this PR can also resolve #387?

@ebaron
Copy link
Member

ebaron commented Aug 30, 2022

I believe this PR can also resolve #387?

That's correct.

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Hey @tthvo, looks good at a glance. I'm going to try it out and see if I can test upgrading the bundle to make sure nothing breaks.

internal/controllers/common/finalizer_utils.go Outdated Show resolved Hide resolved
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Finally managed to do an upgrade test and it succeeded. I ended up creating a catalog image with 2.1.1 and this PR, but OLM didn't attempt an upgrade. I think this might be because 2.1.1 is in the stable channel and this (and all development versions) is in the alpha channel. Using 2.1.2-dev and then this PR (2.2.0-dev), I was able to get the upgrade to work.

If you're interested in how I did it, I made the bundle image with:

make catalog-build CATALOG_IMG=quay.io/ebaron/cryostat-operator-catalog:latest \
BUNDLE_IMGS='quay.io/cryostat/cryostat-operator-bundle:2.1.2-dev,quay.io/ebaron/cryostat-operator-bundle:latest'

You can then install the 2.1.2-dev version specifically using these instructions:
https://docs.openshift.com/container-platform/4.10/operators/user/olm-installing-operators-in-namespace.html#olm-installing-specific-version-cli_olm-installing-operators-in-namespace

@tthvo
Copy link
Member Author

tthvo commented Sep 1, 2022

Great! Thanks @ebaron a lot for showing me how to do this test. I tried out and worked out nicely!!

  1. Built the controller image (tagged 2.2.0-dev) and bundle image (tagged latest).
  2. Built catalog image with cryostat-operator 2.1.2-dev and my above image.
  3. All images pushed to my quay.io/thvo registry in public mode.
  4. Created a CatalogSource pointing to that index image and channel set to alpha,
  5. Created a OperatorGroup with targetNamespace to default.
  6. Created a Subscription with Manual approval plan in default namespace and startingCSV to 2.1.2-dev.

From there, I could see the InstallPlan awaiting approval. I could change its spec to spec.approved to true. On Openshift console, I saw a upgrade-available notification. After approval, operator is installed sucessfully with latest version (2.2.0-dev).

Source:

@ebaron
Copy link
Member

ebaron commented Sep 1, 2022

Glad it worked for you, and thanks for documenting these steps in more detail!

@tthvo tthvo deleted the remove-deprecated-api branch September 5, 2022 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
2 participants