-
Notifications
You must be signed in to change notification settings - Fork 33
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
consoleplugin reconciler #884
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #884 +/- ##
==========================================
+ Coverage 80.20% 81.18% +0.97%
==========================================
Files 64 115 +51
Lines 4492 7420 +2928
==========================================
+ Hits 3603 6024 +2421
- Misses 600 967 +367
- Partials 289 429 +140
Flags with carried forward coverage won't be shown. Click here to find out more.
|
58ddb25
to
75802d1
Compare
@@ -27,6 +27,10 @@ on: | |||
description: WASM Shim version | |||
default: latest | |||
type: string | |||
consolePluginImageURL: |
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.
@didierofrivia @maleck13 trying here complete URL instead of some "version". What are your thoughts about this?
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.
sorry guys, accepted by (answer) omission.
We can always change that later
Merged the docs |
29acec9
to
0e60b7a
Compare
LGTM |
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
…ycle Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
0e60b7a
to
e5bcefd
Compare
Makefile
Outdated
@@ -340,7 +340,7 @@ build: generate fmt vet ## Build manager binary. | |||
|
|||
run: export LOG_LEVEL = debug | |||
run: export LOG_MODE = development | |||
run: export OPERATOR_NAMESPACE = kuadrant-system | |||
run: export OPERATOR_NAMESPACE = $(shell kubectl config view --minify -o jsonpath='{..namespace}') |
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.
This gives an empty value for me for a kind and crc cluster, so it will panic for most people using make run
here unless it's just me 🤔
panic("namespace must be specified and can not be a blank string") |
{
"kind": "Config",
"apiVersion": "v1",
"preferences": {},
"clusters": [
{
"name": "api-crc-testing:6443",
"cluster": {
"server": "https://api.crc.testing:6443",
"certificate-authority-data": "DATA+OMITTED"
}
}
],
"users": [
{
"name": "developer/api-crc-testing:6443",
"user": {
"token": "REDACTED"
}
}
],
"contexts": [
{
"name": "/api-crc-testing:6443/developer",
"context": {
"cluster": "api-crc-testing:6443",
"user": "developer/api-crc-testing:6443"
}
}
],
"current-context": "/api-crc-testing:6443/developer"
}
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.
The command line kubectl config view --minify -o jsonpath='{..namespace}'
is pretty common way to parse the current context's namespace. I do not think there is anything wrong with it.
Your kubeconfig does not have namespace set on the current context. This is a portion of my kubeconfig
contexts:
- context:
cluster: mycluster:443
namespace: kuadrant-system
user: eguzki/mycluster:443
name: kuadrant-system/mycluster:443/eguzki
current-context: kuadrant-system/mycluster:443/eguzki
Maybe oc project kuadrant-system
will help??
I will try to enhance makefile command to handle error when the namespace is not available.
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.
added
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.
Hmm, yeah adding oc project kuadrant-system
will add the namespace
to the project, but without it there no namespace in the context
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.
Yeah, I don't know about this change tbh. I'm getting the following whenever I run make run
on a fresh kind cluster made using make local-env-setup
, so this will affect the make run
command once merged to main.
🚨 namespace could not be parsed from kubeconfig 💥
This does get set on a CRC cluster as I was interfacting with the cluster by following the verification steps. So I assume this gets set typically when interacting with an openshift type cluster unless I'm missing something 🤔
TBH the env var has a default now also so we probably don't need this set in the makefile also unless people want to explicitly change it
operatorNamespace = env.GetString("OPERATOR_NAMESPACE", "kuadrant-system") |
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.
Sorry, I wanted to read the namespace from the kubeconfig context, but turns out that in local env (kind), there is no namespace for the current context.
Reverting to previous state with the addition of adding OPERATOR_NAMESPACE
makefile variable to override default value (kuadrant-system
).
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.
Actually, I think that for make run
, the OPERATOR_NAMESPACE
should be:
- When
OPERATOR_NAMESPACE
makefile variable is set, it takes that value. Else - namespace from kubeconfig current context. If empty, then
- default from
KUADRANT_NAMESPACE
makefile variable, i.e.kuadrant-system
.
But we can leave this for other PR.
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.
cc @Boomatang ^^
if !topologyExists { | ||
utils.TagObjectToDelete(service) | ||
} | ||
err := r.ReconcileResource(ctx, &corev1.Service{}, service, reconcilers.CreateOnlyMutator) |
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.
I guess if we want to align with the conventions agreed, we do not want to use this r.ReconcileResource
function, since it performs a Get
from the api server. Instead we want to get this resource from the topology 🤔
Although not sure do we want to block this PR for this
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.
ReconcileResource
has some functionality we still does not have in the new approach. Do you mind letting this implementation go through? In exchange, I will open issue to address that as tech debt
BTW, I realize that the old issue of endless reconciliation loop for authconfig #354 we fixed with the dry run https://github.com/Kuadrant/kuadrant-operator/blob/main/pkg/library/reconcilers/base_reconciler.go#L156
desired.SetResourceVersion(obj.GetResourceVersion())
if err := b.Client().Update(ctx, desired, client.DryRunAll); err != nil {
return err
}
can now become an issue again with the topology being the source of truth.
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.
Yeah, I'm fine with leaving it to be addressed later 👍
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.
Created issue #916
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Verified steps on a local CRC cluster on Openshift version 4.16.7 👍 |
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
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.
Looks good to me!
What
Deployment of Kuadrant's Openshift dynamic plugin.
ConsolePlugin v1.0 available on OCP >= 4.12 https://docs.openshift.com/container-platform/4.12/rest_api/console_apis/consoleplugin-console-openshift-io-v1.html
The operator deploys the consoleplugin resources together with the service,deployment and configuration configmap in the operator's namespace
The lifecycle of the consoleplugin resource, together with the related deployment resources, is linked to the lifecycle of the topology configmap. When the topology configmap is created, so is the consoleplugin (and related resources). Same when deleting the topology configmap.
The consoleplugin will be deployed only on clusters where the CRDs are present, namely Openshift.
Fixes Kuadrant/kuadrant-console-plugin#46
TODO:
RELATED_IMAGE_CONSOLEPLUGIN
Lifecycle management: delete the plugin when the operator is uninstalled-> Left out for future implementation Removetopology
configmap on kuadrant operator uninstall #896Verification Steps
Verified on OpenShift v4.15
Follow user guide
Once istio is installed, we can deploy gateways and routes to be shown in the console plugin 🎨
Create
kuadrant-system
namespace. Currently it has to bekuadrant-system
as it is hardcoded in the consoleplugin service.To install the Kuadrant Operator, enter the following command to deploy catalog from this
46-consoleplugin
branch:Wait for the Kuadrant Operators to be installed as follows:
After some time, this command should return
complete
.Installation done 🚀 Let's try it out
It may take some time to load the plugin. The first time it will be Disabled. It needs to be enabled.
In
Home -> Overview
, the status box hasDynamic Plugins
. Click on it and then click on view all. Enable kuadrant-console plugin on the form as below:Once enabled, wait until the status says it is loaded, like in the snapshot below
Refresh the openshift's console and a new entry
Kuadrant
in the left menu will show upIf you check Overview and Policies it should be all empty (provided there is no gateway, httproute and kuadrant policy installed. Let's populate the cluster with some interesting resources.
Apply the Kuadrant custom resource
Add one gateway
Add one HTTPRoute
Add one RateLimitPolicy
After that, the topology graph looks like this:
You should get something like:
Cleanup