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

Add namespaces support for REST #668

Conversation

martindekov
Copy link
Contributor

Adding namespaces support for the REST endpoints
of the operator. Currently only the function objects
are spawned without the actual pods running the function
themselves

Signed-off-by: Martin Dekov [email protected]

Add function CRD creation across namespaces for the operator's REST controller

Description

Enable rest controller for the operator to spawn function objects across namespaces. The functions yet don't spawn pods, only the function CRD.

Motivation and Context

  • I have raised an issue to propose this change (required)

Part of #616

How Has This Been Tested?

Tested by creating resource, will publish logs once we discuss this draft

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Adding namespaces support for the REST endpoints
of the operator. Currently only the function objects
are spawned without the actual pods running the function
themselves

Signed-off-by: Martin Dekov <[email protected]>
@@ -73,7 +73,7 @@ oauth2Plugin:
securityContext: true

faasnetes:
image: openfaas/faas-netes:0.10.5
image: martindekov/faas-netes:0.12.0-rc1
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 will revert, generally the change is in this image

Name: item.Name,
}
secrets = append(secrets, secret)
handler := handlers.SecretsHandler{
Copy link
Member

Choose a reason for hiding this comment

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

It isn't clear to me why we make SecretsHandler public instead of using the already public factory method MakeSecretHandler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah Lucas, now I can see that due to your feedback, when I wrote this I didn't thought that actually secrets are generic openfaas feature, and not specific for the operator. Will switch to that 👍

@@ -13,9 +13,22 @@ import (
glog "k8s.io/klog"
)

func makeDeleteHandler(namespace string, client clientset.Interface) http.HandlerFunc {
func makeDeleteHandler(defaultNamespace string, client clientset.Interface) http.HandlerFunc {
Copy link
Member

Choose a reason for hiding this comment

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

I am going to make a rather big suggestion that will require another refactor. I think we should have a FunctionClient interface with the needed create/update/delete/list methods. We can then create two implementation one based on the clientset and one based on the informer/lister and even one based on the CRD. This allows the logic in the handlers to be separated from the implementation. This will simplify the handlers and allow us to easily swap the implementation over time. This also means the server implementation can be unified and reused between both faas-netes and the operator because the important distinct "CRD based FunctionClient" vs "API based FunctionClient" is wrapped into the specific client implementation

Copy link
Contributor Author

@martindekov martindekov Jul 9, 2020

Choose a reason for hiding this comment

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

From my understanding on the proposal, you mean using only one types.FaaSHandlers instead of having almost duplicate code for the operator and faas-netes. This FunctionClient will internally switch between the details of the two (if there are any differences between them). So essentially is this some sort of a Facade pattern over the types.FaaSHandlers? I can ping you to discuss this further to gather more context on the proposed refactoring. Also this change is missing the actual creation of functions, it currently creates only the Function CRD. I was also hoping to get some insight what I should be changing in order to make this work e2e.

@martindekov
Copy link
Contributor Author

Full change is here: #673

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.

2 participants