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

[POC] Can serverless-webhook be removed ? #17562

Closed
kwiatekus opened this issue May 24, 2023 · 3 comments
Closed

[POC] Can serverless-webhook be removed ? #17562

kwiatekus opened this issue May 24, 2023 · 3 comments
Assignees
Labels
area/serverless Issues or PRs related to serverless

Comments

@kwiatekus
Copy link
Contributor

kwiatekus commented May 24, 2023

Description

This exercise it to verify if we can get rid of serverless-webhook in favor of validation rules that can be defined in the Function CRD itself.
Find out which features of serverless webhook would be sacrified if only validation rules on CRD would be used.
Could we introduce the rules and cover the same validation/defaulting rules w/o api breaking changes?

Reasons

Webhooks are intercepting calls to apiserver and introduce another point of failure.
By getting rid of our webhook we can improve resiliency of serverless module and remove the problem of CA generation.
Also, atm user can apply functions CR only after webhook is up, and it takes it a while until its up.
If we get rid of webhooks we will enable true declarative way of defining function.

Attachments

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules

@kyma-bot
Copy link
Contributor

This issue or PR has been automatically marked as stale due to the lack of recent activity.
Thank you for your contributions.

This bot triages issues and PRs according to the following rules:

  • After 60d of inactivity, lifecycle/stale is applied
  • After 7d of inactivity since lifecycle/stale was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Close this issue or PR with /close

If you think that I work incorrectly, kindly raise an issue with the problem.

/lifecycle stale

@kyma-bot kyma-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 23, 2023
@kwiatekus kwiatekus removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 26, 2023
@pPrecel pPrecel removed their assignment Aug 1, 2023
@dbadura dbadura self-assigned this Aug 22, 2023
@dbadura
Copy link
Contributor

dbadura commented Aug 24, 2023

Our current validation rules for function CR:

  • user can choose resoruce profile or custom resources.
  • when nothing is passed, the default profile is set.
  • default profile in case of empty profile and resoruces.
We can replace this logic in controller and put the the used profile into status. Checking if one of profile or resouces is set and in case none of them is set, setting defualt value sounds very complicated
  • resoruce request has to be smaller than resource limits.
It's not possible because values are strings and comparing string is not the same as comparing the resources.

Opened ticket: kubernetes-sigs/controller-tools#827

  • restriced ENVs
It's possible to achieve by using regex and all() fn.
  • git url validation. It has to be http(s) or git.
  • runtime validation.
It can be an enum.
  • Reastrictied labels and annotations.
  • replicas has to be between ScaleConfig min and max if ScaleConfig is defined.
  • default profil for replicas.
Currently we don't have such field in spec. I would consider to remove it or use the same mechanism as in resource profile
  • secret mounts has to be k8s compatible (e.g.: size) and mountpath cannot me empty.

  • function is inline or git.

  • baseDir and reference cannot be empty in case of git function.

We can consider to use default values like `master` and `/`
  • gitAuth can be Key or Basic.
  • Secret reference with git credentials.
Similar validation to k8s secret

PoC PR: #18034

Webhook secret:
It's executed agains secret with label: "serverless.kyma-project.io/remote-registry": "config"
and extracts:

  • username
  • password
  • serverAddress
    then it create .dockerconfigjson and put it in secret.

Out secrer is created in chart and has .dockerconfigjson no matter if is internal or not. Consits of following fields:

  • registryAddress
  • serverAddress (it's used to create dockerconfigjson)

Our controller uses registryAddress to push by kaniko and set image in pod. dockerconfigjson is used to pull image by kubelet.

The distinction between serverAddress and registryAddress is because docker.hub is edge case. To pull images from dockerhub the https://index.docker.io/v1/ address is used and it's in dockerconfigjson. To pull images you have to type image name like e.g.: my-user/my-image.

User can create registry secret by kubectl:

  kubectl create secret docker-registry my-secret --docker-server=DOCKER_REGISTRY_SERVER --docker-username=DOCKER_USER
--docker-password=DOCKER_PASSWORD --docker-email=DOCKER_EMAIL

From out perspective this webhook is unnecesary if we point users to create registry secret using kubectl.

@kwiatekus
Copy link
Contributor Author

The following steps would allow us to remove serverless webhook

  • Check if build job can push to docker registry identified by secret of docker-registry type (w/o any additions like registry address)

  • For defaulting we could use controller logic. I.e if user doesnt provide runtime/buildtime profile, the controller would use the currently configured default profile and inform about used profile in the status.

  • Comparing if Limits > Requests could be dropped. Misconfigured function would show an error message in Deployed condition

  • Reject not allowed changes via CEL for

    • ENVs
    • git url
    • runtime (CEL preferred over enum)
    • labels & annotations
    • secret mounts ( secret name regexp & mountPath )
    • function source type ( inline/git)
    • git references, secret validation
  • Get rid of validation for spec.replicas :

  • between ScaleConfig.min/max values

  • Get rid of profile for replicas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/serverless Issues or PRs related to serverless
Projects
None yet
Development

No branches or pull requests

4 participants