Add switch to enable/disable operators support #4051
Conversation
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
| {{- if not $.Values.featureFlags.operators }} | ||
|
|
||
| # Operators support disabled | ||
| location ~* /api/clusters/{{ .name }}/.*operators.coreos.com.* { |
There was a problem hiding this comment.
With this, should be enough for Nginx to avoid calls to URLs related to operators mgmt.
There was a problem hiding this comment.
Interesting... I was surprised to see this. I guess I'll be replacing this once my PRs land to instead remove any location for /api/clusters (see line 78 below) when the operators flag is false, whereas you needed this to explicitly ensure no calls to the api operator endpoints were made?
There was a problem hiding this comment.
Exactly, this piece should be temporary until we get rid of the direct calls to k8s API.
| // Ignore APIs for operators if specified | ||
| const operatorsEnabled = getStore().config.featureFlags?.operators === true; | ||
| if (!operatorsEnabled) { | ||
| groups = groups.filter( |
There was a problem hiding this comment.
When operators disabled and OLM is existing in the cluster, groups are still containing stuff related to operators.
There was a problem hiding this comment.
Hmm, when operators are disabled, there should be no need to call getResourceKinds at all, I don't think? They used to be used by the AppView to determine general resource kinds (whether they were namespaced, what the plural was etc.), but this was replaced by the resources plugin which does the work (with a RESTMapper) in the apis server. Did you find an error when not calling getResourceKinds at all? Perhaps I've missed something, let me know.
There was a problem hiding this comment.
Didn't try to not calling getResourceKinds, as I assumed if it was there, it was for some reason other than operators.
I'm totally fine with giving it a try and removing it. Is everything related to resources plugin already in master or should I create an issue to track this later?
There was a problem hiding this comment.
I believe you already tackled this here?
| <PrivateRouteContainer key={route} exact={true} path={route} component={component} /> | ||
| ))} | ||
| {!this.props.featureFlags?.operators && | ||
| Object.entries(unsupportedRoutes).map(([route]) => ( |
There was a problem hiding this comment.
If no operators supported, warning message is shown.
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
castelblanque
changed the title
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
| {{- if not $.Values.featureFlags.operators }} | ||
|
|
||
| # Operators support disabled | ||
| location ~* /api/clusters/{{ .name }}/.*operators.coreos.com.* { |
There was a problem hiding this comment.
Interesting... I was surprised to see this. I guess I'll be replacing this once my PRs land to instead remove any location for /api/clusters (see line 78 below) when the operators flag is false, whereas you needed this to explicitly ensure no calls to the api operator endpoints were made?
| // Ignore APIs for operators if specified | ||
| const operatorsEnabled = getStore().config.featureFlags?.operators === true; | ||
| if (!operatorsEnabled) { | ||
| groups = groups.filter( |
There was a problem hiding this comment.
Hmm, when operators are disabled, there should be no need to call getResourceKinds at all, I don't think? They used to be used by the AppView to determine general resource kinds (whether they were namespaced, what the plural was etc.), but this was replaced by the resources plugin which does the work (with a RESTMapper) in the apis server. Did you find an error when not calling getResourceKinds at all? Perhaps I've missed something, let me know.
| menu.simulate("click"); | ||
| wrapper.update(); | ||
| expect(wrapper.find(".dropdown")).toHaveClassName("open"); | ||
| // It render links for AppRepositories and operators |
There was a problem hiding this comment.
Copy-n-pasto? :)
| // It render links for AppRepositories and operators | |
| // It render links for AppRepositories but not operators |
There was a problem hiding this comment.
Well, actually, when operators are enabled, it renders both links. Will rewrite it, though.
| } as const; | ||
|
|
||
| const unsupportedRoutes = { | ||
| "/c/:cluster/ns/:namespace/operators*": "Operators support has been disabled for Kubeapps.", |
There was a problem hiding this comment.
Perhaps: "Operators support has been disabled by default for Kubeapps. Enable it in your values.yaml" (or something better, that sounds awkward, but something communicating that it can be enabled).
| <PrivateRouteContainer key={route} exact={true} path={route} component={component} /> | ||
| ))} | ||
| {!this.props.featureFlags?.operators && | ||
| Object.entries(unsupportedRoutes).map(([route]) => ( |
| @@ -198,6 +198,7 @@ installOrUpgradeKubeapps() { | |||
| --set apprepository.initialRepos[0].url=http://chartmuseum-chartmuseum.kubeapps:8080 | |||
| --set apprepository.initialRepos[0].basicAuth.user=admin | |||
| --set apprepository.initialRepos[0].basicAuth.password=password | |||
| --set featureFlags.operators=true | |||
There was a problem hiding this comment.
We will probably need to have a switch (env var) for CI so that we can run CI with or without the operator support and tests, but doesn't need to be done here and now, just noting.
There was a problem hiding this comment.
Absolutely, by now we will have CI running with operators to check that we keep on supporting it 😄
There was a problem hiding this comment.
Wow, only now I see your comments below, forget my comment above this one 😃
There was a problem hiding this comment.
FWIW, I've gone ahead and done two of the small points I've mentioned here:
- updated the ci script so that operators can be enabled or disabled via env,
- no fetching of resources happens at all from the dashboard if operators are not enabled
in my own PR at Test running without k8s api proxying. #4054, so I'd be able to test running on CI without the api proxying in the frontend.
Feel free to land this PR here as is and I'll update my PR to target master, or fine if you want to change anything here (I'll just wait until your PR lands before I take my PR out of draft).
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Description of the change
Adds a flag in the Kubeapps Helm chart to control if Operators support must be enabled or disabled.
By default is set to disabled. Hopefully with this changes + the huge job that @absoludity has done (#3896), Kubeapps shouldn't be directly calling to K8s APIs.
Changes here prevent both dashboard and Nginx from interacting with Operators. But still the OLM framework can live and function in the cluster as usual, of course.
Benefits
Operators can be ignored from the Kubeapps perspective, and no direct calls to K8s apis are done.
Possible drawbacks
Existing Kubeapps installation that work with Operators will need to set the new flag to true when updating in order to keep working. Breaking change?
Applicable issues
Additional information
N/A