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

feat(kuma-cp) Validation synced resources on K8S #919

Merged
merged 3 commits into from
Jul 24, 2020

Conversation

jakubdyszkiewicz
Copy link
Contributor

Summary

This PR introduces validation on K8S that prevents users to apply policies on Remote by accident.
It relies on annotation k8s.kuma.io/synced: "true"

Documentation

  • Internal feature only

@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner July 22, 2020 09:20
apiVersion: admissionregistration.k8s.io/v1beta1
kind: ValidatingWebhookConfiguration
metadata:
name: kuma-validating-webhook-configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Another one to keep in sync with the helm chart.

- containerPort: 5443
{{- if ne .KumaCpMode "global" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed!

@@ -75,11 +79,27 @@ func (s *KubernetesStore) Create(ctx context.Context, r core_model.Resource, fs
return nil
}

func markAsSynced(obj k8s_model.KubernetesObject) {
annotations := obj.GetAnnotations()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This functionality seems very internal. I think it's more of an annotation

Allowed: false,
Result: &metav1.Status{
Status: "Failure",
Message: "You are trying to apply policy on Remote CP. In multicluster setup, policies should be only applied on Global CP and synced to Remote CPs.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Good!

@@ -57,6 +65,12 @@ func ModifiedAt(modificationTime time.Time) UpdateOptionsFunc {
}
}

func UpdateSynced() UpdateOptionsFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need UpdateSynced? If some resource was created with CreateSynced then it should be staying synced for the whole lifetime scope, right? So it is a permanent characteristic of the resource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically yes, but there is an exception - Mesh object. Remote can create default Mesh and then it's updated from the remote.

pkg/plugins/runtime/k8s/webhooks/validation.go Outdated Show resolved Hide resolved
@@ -72,6 +102,28 @@ func (h *validatingHandler) Handle(ctx context.Context, req admission.Request) a
return admission.Allowed("")
}

func (h *validatingHandler) validateSync(resType core_model.ResourceType, obj k8s_model.KubernetesObject) admission.Response {
if resType == core_mesh.MeshType && len(obj.GetSpec()) == 0 { // skip validation for the default mesh
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment says that we skip default mesh, but it skips every mesh with empty spec, isn't it? And why do we have to skip default mesh? Is it created by defaultingWebhook?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because Remote can create default Mesh and if we let the logic pass, it will fail because normally you should not create meshes on Remotes.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest I thought that since webhook disabled on Remote, mesh object always comes from Global, but I see that only ValidationWebhook was disabled. Maybe we can disable mesh-defaulter for Remote to have unified way of managing resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know... mesh defaulter does not make sense on remote 🤔 it's only useful when you apply a mesh with some settings, but it will be defaulted in global

@jakubdyszkiewicz jakubdyszkiewicz merged commit 4a6b020 into master Jul 24, 2020
@jakubdyszkiewicz jakubdyszkiewicz deleted the feat/sync-resources-validation branch July 24, 2020 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants