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

Discovery Selectors #797

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Conversation

jmazzitelli
Copy link
Contributor

@jmazzitelli jmazzitelli commented Jul 23, 2024

Implement the new discovery selector support in the operator.

part of: kiali/kiali#7546
part of KEP: https://github.com/kiali/kiali/blob/master/design/KEPS/namespace-discovery/proposal.md

server PR: kiali/kiali#7592
operator PR: #797
helm chart PR: kiali/helm-charts#274
docs PR: kiali/kiali.io#807

@jmazzitelli jmazzitelli added enhancement New feature or request requires server PR A PR requires additional changes in the backend code. requires helm chart PR labels Jul 23, 2024
@jmazzitelli jmazzitelli self-assigned this Jul 23, 2024
@jmazzitelli
Copy link
Contributor Author

jmazzitelli commented Jul 23, 2024

Some things I know I still need to get the operator to do (UPDATE: this is done)

  • attempt to auto-detect istio configmap
  • set the discovery_selectors in the configmap to select only those namespaces that the operator gave kiali access to

@jmazzitelli
Copy link
Contributor Author

jmazzitelli commented Jul 24, 2024

As of right now, I can confirm the following molecule tests pass (minus the server-side testing - mainly calls into the server's namespaces API):

accessible-namespaces-test
affinity-tolerations-resources-test
cluster-wide-access-test
config-values-test
default
default-namespace-test
instance-name-test
null-cr-values-test
only-view-only-mode-test
os-console-links-test
read-certs-secrets-test
roles-test
rolling-restart-test

Next up the server has to be refactored to move away from accessible_namespaces and go use discovery selectors. Once the server is ready, the following molecule tests then need to be tested (along with the ones above, since they were not testing server-side functionality at the time I got them to pass).

grafana-test
header-auth-test
jaeger-test
metrics-test
openid-test
token-test

@jmazzitelli
Copy link
Contributor Author

Two more things added to this PR.

First, when discovery selectors are defined AND cluster wide access is FALSE, the operator will set discovery_selectors.default in the ConfigMap to be a list of simple match_labels, one for each namespace. They will match on the namespace name label kubernetes.io/metadata.name. So, in other words, a user can provide a complex set of selectors in the Kiali CR - the operator will find all namespaces that match those selectors at the time the Kiali CR is reconciled. The operator will then replace the discovery_selectors.default field in the ConfigMap - it won't have those complex set of selectors; instead, it will have a list of simple match_labels selectors - one kubernetes.io/metadata.name selector per namespace. This way the server will be ensured to select those namespaces (and only those namespaces) in which it was given permission to see (the operator's Roles will ensure permissions are set up correctly for those namespaces, but only those namespaces).

Note that if cluster_wide_access is True, the operator simply leaves the ConfigMap's discovery selectors to those complex set of selectors the user put in the Kiali CR. This is because Kiali is given permission to see any and all namespaces in the cluster, so if new namespaces are created and match those selectors, the Kiali Server will see them (assuming it queries for namespaces again using the selectors). There is no need to fix the Kiali Server on a set list of namespaces. This behavior is the same as the old accessible_namespaces of ["**"].

Secondly, the operator will attempt to find the Istio ConfigMap (regardless of its name) in the istio control plane namespace. It will look at all ConfigMaps, and the first it finds that has "data.mesh" field, that will be used as the Istio ConfigMap. It will then look inside to see if there are any discoverySelectors for the operator to use. Note that the operator only ever looks for the Istio ConfigMap if there is no discovery_selectors.default setting in the Kiali CR.

@jmazzitelli
Copy link
Contributor Author

reminder to me to document the labels that the operator creates. things like kiali.io/kiali.home and other labels.

@jmazzitelli jmazzitelli force-pushed the discovery-selectors branch 5 times, most recently from aced14a to 28f494e Compare August 3, 2024 06:44
@jmazzitelli jmazzitelli marked this pull request as ready for review August 3, 2024 16:24
@jmazzitelli jmazzitelli requested a review from nrfox August 3, 2024 19:29
@jmazzitelli jmazzitelli force-pushed the discovery-selectors branch 4 times, most recently from c1612f5 to 9669cc1 Compare August 5, 2024 20:29
@jmazzitelli jmazzitelli force-pushed the discovery-selectors branch 2 times, most recently from 975f736 to 391cd4a Compare August 14, 2024 18:57
@jmazzitelli
Copy link
Contributor Author

jmazzitelli commented Aug 14, 2024

Just documenting the labels and annotations that are used by the operator

namespace label (created when the operator makes the namespace accessible to the kiali server):

  • kiali.io/<instance-name>.home == <kiali deployment.namespace>

remote cluster secret annotation (created by script that creates remote cluster secrets):

  • kiali.io/cluster == <cluster name>

kiali deployment annotation (created/updated by the operator when it restarts the kiali server pod)

  • operator.kiali.io/last-updated == <date/time of last update>

ossmconsole deployment annotation (created/updated by the operator when it restarts the ossmconsole server pod)

  • ossmconsole.kiali.io/last-updated == <date/time of last update>

@jmazzitelli jmazzitelli force-pushed the discovery-selectors branch 2 times, most recently from 47e833c to ad7cdef Compare August 19, 2024 19:54
nrfox
nrfox previously approved these changes Aug 29, 2024
@jmazzitelli
Copy link
Contributor Author

jmazzitelli commented Aug 30, 2024

I made a change - now that the server supports multiple "values" for a matchExpressions, I change the operator so it creates those instead of multiple matchLabels (makes reading the configmap easier and probably more efficient on the server side).

Before, the operator was putting accessible namespaces in the discovery selectors within the ConfigMap like this:

discovery_selectors:
  default:
  - matchLabels:
      kubernetes.io/metadata.name: istio-system
  - matchLabels:
      kubernetes.io/metadata.name: my-namespace
  - matchLabels:
      kubernetes.io/metadata.name: my-second-namespace

Now it will be:

discovery_selectors:
  default:
  - matchExpressions:
    - key: kubernetes.io/metadata.name
      operator: In
      values:
      - istio-system
      - my-namespace
      - my-second-namespace

Molecule tests all pass after making this change:

=====================
=== TEST RESULTS: ===
=====================

              accessible-namespaces-test... success [6m 36s]
     affinity-tolerations-resources-test... success [3m 20s]
                cluster-wide-access-test... success [7m 30s]
                      config-values-test... success [2m 23s]
                  default-namespace-test... success [1m 46s]
                            grafana-test... success [3m 8s]
                        header-auth-test... success [1m 37s]
                      instance-name-test... success [2m 10s]
                             jaeger-test... success [2m 33s]
                            metrics-test... success [3m 31s]
                     null-cr-values-test... success [1m 33s]
                only-view-only-mode-test... success [2m 20s]
                             openid-test... success [1m 36s]
                   os-console-links-test... skipped
          ossmconsole-config-values-test... skipped
                 read-certs-secrets-test... success [1m 33s]
                              roles-test... success [4m 46s]
                    rolling-restart-test... success [2m 15s]
                              token-test... success [1m 53s]

Copy link
Contributor

@jshaughn jshaughn left a comment

Choose a reason for hiding this comment

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

Change is fine by me, more efficient and easier to look at.

@jmazzitelli jmazzitelli merged commit f95678a into kiali:master Aug 30, 2024
1 check passed
@jmazzitelli jmazzitelli deleted the discovery-selectors branch August 30, 2024 14:08
@jshaughn jshaughn added the test: molecule Molecule test infrastructure and tests themselves label Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request requires helm chart PR requires server PR A PR requires additional changes in the backend code. test: molecule Molecule test infrastructure and tests themselves
Projects
Development

Successfully merging this pull request may close these issues.

3 participants