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

Ingress Hostname and Path collision at Tenant level #366

Merged
merged 16 commits into from
Aug 12, 2021
Merged

Conversation

prometherion
Copy link
Member

Closes #358.

On CapsuleConfiguration API side, the collision flags have been removed since this is going to be managed at Tenant level, also for the Tenant allowed hostnames.

Now the collision check is taking care also of the path, allowing creating several Ingress but with different path since it's a quite common pattern due to some limitations of specific Ingress Controller.

Last but not least, on v1beta a new IngressOptions struct has been implemented, grouping allowed IngressClass, allowed hostnames, and the new scope feature (backported to v1alpha1 too with the annotation ingress.capsule.clastix.io/hostname-collision-scope accepting one of the enum values Cluster, Tenant, Namespace, Disabled, this latter one the default).

@prometherion prometherion requested a review from bsctl August 1, 2021 13:23
@prometherion prometherion changed the title Ingress Hostname and Path collision Ingress Hostname and Path collision at Tenant level Aug 1, 2021
@prometherion prometherion force-pushed the issues/358 branch 3 times, most recently from ec8b798 to 09f183e Compare August 1, 2021 13:40
@MaxFedotov
Copy link
Collaborator

Hi Dario! Thanks a lot for the great work - this ability to check also for path in collisions is what we are looking for! The only thing I want to mention, so we won't forget it when this will be merged - we had to update https://github.com/clastix/capsule-proxy/tree/master/internal/modules/ingressclass, as tenant spec changed

@prometherion
Copy link
Member Author

Thanks @MaxFedotov, I totally forgot it: should be easy to achieve, BTW.

Going to open an issue on proxy side referencing your comment.

@prometherion prometherion requested a review from MaxFedotov August 4, 2021 11:39
Copy link
Collaborator

@MaxFedotov MaxFedotov left a comment

Choose a reason for hiding this comment

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

lgtm!

@prometherion prometherion force-pushed the issues/358 branch 3 times, most recently from 1ec5eaf to 9b098b4 Compare August 12, 2021 10:41
Copy link
Member

@bsctl bsctl left a comment

Choose a reason for hiding this comment

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

@prometherion
noted that when collision is detected, the error message does not return the colliding ingress, "STDIN" is reported instead.

Error from server (hostname web.uno.fastcloud.it is already used across the cluster: please, reach out to the system administrators): error when creating "STDIN": admission webhook "ingress.capsule.clastix.io" denied the request: hostname web.uno.fastcloud.it is already used across the cluster: please, reach out to the system administrators

Also the collision events are not recorded in kubectl describe tnt <tenant>

@prometherion
Copy link
Member Author

We had a bug in the Event recording, fixed with last commit.

@bsctl
Copy link
Member

bsctl commented Aug 12, 2021

Events are now emitted and collected

Events:
  Type     Reason                    Age                From               Message
  ----     ------                    ----               ----               -------
  Normal   NamespaceCreationWebhook  12m                tenant-webhook     Namespace due-production has been assigned to the desired Tenant
  Normal   due-production            11m (x4 over 12m)  tenant-controller  Ensuring Namespace metadata
  Normal   due-production            11m (x4 over 12m)  tenant-controller  Ensuring Capsule RoleBinding namespace:admin
  Normal   due-production            11m (x4 over 12m)  tenant-controller  Ensuring Capsule RoleBinding namespace-deleter
  Normal   NamespaceCreationWebhook  11m                tenant-webhook     Namespace due-development has been assigned to the desired Tenant
  Normal   due-development           11m (x2 over 11m)  tenant-controller  Ensuring Namespace metadata
  Normal   due-development           11m (x2 over 11m)  tenant-controller  Ensuring Capsule RoleBinding namespace:admin
  Normal   due-development           11m (x2 over 11m)  tenant-controller  Ensuring Capsule RoleBinding namespace-deleter
  Warning  IngressHostnameCollision  10m                tenant-webhook     Ingress due-production/example hostname is colliding

@bsctl
Copy link
Member

bsctl commented Aug 12, 2021

Unfortunately, still see the Ingress referenced as STDIN in the error message:

Error from server (hostname web.fastcloud.it is already used across the cluster: please, reach out to the system administrators): error when creating "STDIN": admission webhook "ingress.capsule.clastix.io" denied the request: hostname web.fastcloud.it is already used across the cluster: please, reach out to the system administrators

@bsctl bsctl self-requested a review August 12, 2021 17:09
@prometherion
Copy link
Member Author

The STDIN Is the Input for kubectl when posting the payload, it's something unrelated to Capsule.

Copy link
Member

@bsctl bsctl left a comment

Choose a reason for hiding this comment

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

@prometherion LGTM, great work!

@prometherion prometherion merged commit a2fda44 into master Aug 12, 2021
@prometherion prometherion deleted the issues/358 branch August 12, 2021 17:30
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.

Ingress hostname collision check at Tenant level
3 participants