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

MAISTRA-1956 Stabilize accessible_namespaces in the Kiali resource #699

Merged
merged 1 commit into from
Apr 13, 2021

Conversation

luksa
Copy link
Contributor

@luksa luksa commented Apr 12, 2021

Previously, the operator reset the accessible_namespaces field in the
Kiali resource to only the control plane namespace every time the SMCP
was updated. Now, the Helm chart for the Kiali resource no longer
contains the accessible_namespaces field. This allows the
ServiceMeshMemberRoll controller to be the only component that updates
this field and ensures that the list of namespaces reflects the list
of member namespaces at all times.

Additionally, the list of namespaces now contains all the member
namespaces (either configured in SMMR.spec.members or those that
contain a ServiceMeshMember object). Previously, it only contained
configured member namespaces, which caused namespaces to be removed
from the list every time the SMCP was updated (until the namespace
was configured according to the new SMCP version).

Kiali does not require the control plane namespace to be listed
in accessible_namespaces, as it adds it automatically. Kiali also
does not need the member namespaces to be configured in any way,
so it's safe to include all member namespaces.

@@ -290,6 +290,7 @@ func (r *MemberRollReconciler) reconcileObject(ctx context.Context, roll *maistr
}

// 5. check each ServiceMeshMember object to see if it's configured or terminating
allKnownMembers := sets.NewString(roll.Spec.Members...).Insert(getNamespaces(members)...).Delete(meshNamespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also filter out namespaces that do not exist. Personally, I don't think we should allow configuration of members that don't exist. Maybe we can leave this as is, and move the discussion to member roll validation (I think there's an existing issue).

Copy link
Contributor

Choose a reason for hiding this comment

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

can't we leave that validation to kiali?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on my tests, Kiali seems to handle missing namespaces by itself.

I do agree that we should probably not allow users to add nonexistent namespaces to the SMMR. We don't even need to add any kind of validation to the SMMR. We can simply remove the .spec.members field. Actually, adding validation wouldn't even work properly, as users could delete the namespace and we'd end up with an SMMR pointing to a nonexistent namespace again.

Mesh admins can create a member namespace, create a ServiceMeshMember resource inside it, and then grant admin access to the namespace to a particular mesh user. This way, there's no way of somebody else gaining access to the mesh simply by creating the namespace that they know is specified in the SMMR.

Previously, the operator reset the accessible_namespaces field in the
Kiali resource to only the control plane namespace every time the SMCP
was updated. Now, the Helm chart for the Kiali resource no longer
contains the accessible_namespaces field. This allows the
ServiceMeshMemberRoll controller to be the only component that updates
this field and ensures that the list of namespaces reflects the list
of member namespaces at all times.

Additionally, the list of namespaces now contains all the member
namespaces (either configured in SMMR.spec.members or those that
contain a ServiceMeshMember object). Previously, it only contained
configured member namespaces, which caused namespaces to be removed
from the list every time the SMCP was updated (until the namespace
was configured according to the new SMCP version).

Kiali does not require the control plane namespace to be listed
in accessible_namespaces, as it adds it automatically. Kiali also
does not need the member namespaces to be configured in any way,
so it's safe to include all member namespaces.
@luksa
Copy link
Contributor Author

luksa commented Apr 13, 2021

/test istio-operator_unittests

@maistra-bot maistra-bot merged commit bec3f82 into maistra:maistra-2.1 Apr 13, 2021
shamsher31 pushed a commit to shamsher31/istio-operator that referenced this pull request Jun 4, 2021
…aistra#699)

Previously, the operator reset the accessible_namespaces field in the
Kiali resource to only the control plane namespace every time the SMCP
was updated. Now, the Helm chart for the Kiali resource no longer
contains the accessible_namespaces field. This allows the
ServiceMeshMemberRoll controller to be the only component that updates
this field and ensures that the list of namespaces reflects the list
of member namespaces at all times.

Additionally, the list of namespaces now contains all the member
namespaces (either configured in SMMR.spec.members or those that
contain a ServiceMeshMember object). Previously, it only contained
configured member namespaces, which caused namespaces to be removed
from the list every time the SMCP was updated (until the namespace
was configured according to the new SMCP version).

Kiali does not require the control plane namespace to be listed
in accessible_namespaces, as it adds it automatically. Kiali also
does not need the member namespaces to be configured in any way,
so it's safe to include all member namespaces.
@luksa luksa deleted the MAISTRA-1956 branch September 3, 2021 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants