fix(discovery): BUILTIN_DISCOVERY_DISABLED can be specified even when PLATFORM is set #1865
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @andrewazores! Add at least one of the required labels to this PR Required labels are : chore,ci,cleanup,docs,feat,fix,perf,refactor,style,test |
andrewazores
force-pushed
the
builtin-discovery-disable
branch
2 times, most recently
from
74bf2ed
to
698d100
Compare
|
/build_test |
|
Workflow started at 1/24/2024, 12:27:28 PM. View Actions Run. |
|
/build_test |
|
Workflow started at 1/24/2024, 12:36:11 PM. View Actions Run. |
To run smoketest: |
|
|
|
/build_test |
|
Workflow started at 1/24/2024, 1:27:31 PM. View Actions Run. |
To run smoketest: |
|
|
andrewazores
changed the title
andrewazores
force-pushed
the
builtin-discovery-disable
branch
from
b6d5af8
to
f22bfb7
Compare
|
/build_test |
|
Workflow started at 2/6/2024, 11:51:23 AM. View Actions Run. |
To run smoketest: |
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Welcome to Cryostat! 👋
Before contributing, make sure you have:
mainbranch[chore, ci, docs, feat, fix, test]To recreate commits with GPG signature
git fetch upstream && git rebase --force --gpg-sign upstream/mainFixes: #1864
Description of the change:
Removes
BUILTIN_DISCOVERY_DISABLEDenvironment variable from the platform detection selection logic. This is an unfortunate and very old design decision I made that the platform integration is coupled to both the default auth manager as well as platform-specific target discovery mechanism. This made sense when originally implemented because it never made sense for platform-specific discovery mechanisms to overlap, but with the advent of custom targets and later the pluggable discovery API, and then with support for both Podman and Docker API, this is clearly no longer a good assumption and no longer a good abstraction.This results in a bug where the variable used to control turning off all of these built-in discovery mechanisms is evaluated in the same place as where we determine which platform detection strategies to enable, which therefore also implicates the AuthManagers. We want to be able to do automatic platform auth (OpenShift is the only one that exists now) integration while also allowing users to turn off built-in discovery in case it causes problems or they intend not to use it.
The fix applied in this patch is to allow platform detection as a whole to either work by the usual automatic detection mechanisms, or to allow the user to use the
CRYOSTAT_PLATFORMenvironment variable to specify the list of platforms to enable explicitly. TheBUILTIN_DISCOVERY_DISABLEDvariable is not checked until later, after the platform strategies are selected. Once all the strategies are selected the DiscoveryStorage, which integrates all of the mechanisms into one data repository, turns each one on only if its newisEnabled()method returns true. For Custom Targets and Discovery Storage itself (the pluggable API) this is always true, but for the other built-in discovery mechanisms (Kubernetes API, Podman, Docker) the check depends onBUILTIN_DISCOVERY_DISABLEDnot being set totrue.How to manually test:
./mvnw clean package ; podman image prune -f ; podman tag quay.io/cryostat/cryostat:latest quay.io/$MY_QUAY_USERNAME/cryostat:pr-1865-1podman push quay.io/$MY_QUAY_USERNAME/cryostat:pr-1865-1cd cryostat-operatoroc new-project pr1865DEPLOY_NAMESPACE=$(oc project -q) CORE_IMG=quay.io/$MY_QUAY_USERNAME/cryostat:pr-1865-1 make deploymake create_cryostat_cr ; make sample_appoc patch -n $(oc project -q) cryostat/cryostat-sample -p '{"spec":{"targetDiscoveryOptions":{"builtInDiscoveryDisabled":true}}}'to flip the switch and turn off builtin discovery