Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

Use of the kubeless http trigger instead of a custom created ingress #206

Merged
merged 12 commits into from
May 19, 2020
Merged

Conversation

faxioman
Copy link
Contributor

@faxioman faxioman commented May 5, 2020

This pull request enables the support for the Kubeless http trigger.
The code related to the creation/deletion of a custom ingress is removed.
All the options of the previous release of the serverless-kubeless plugin are preserved with this exceptions:

  • the ingress.tlsConfig option is replaced by a ingress.tlsSecretName option, same as the --tls-secret flag when using the kubeless cli
  • a new option ingress.enableTlsAcme is added, same as the --enableTLSAcme flag when using the kubeless cli
  • a new option ingress.basicAuthSecretName is added, same as the --basic-auth-secret flag when using the kubeless cli
  • a new boolean option ingress.cors is added, same as the --cors-enable flag when using the kubeless cli

This pull request includes a workaround for the issue vmware-archive/kubeless#1124

@andresmgot
Copy link
Contributor

Thanks @faxioman, I will review this shortly. It also seems that the CI is failing in the test that creates the ingress/http trigger:

    http-custom-path
      1) should return a "hello world" in a subpath

We probably need to update that, I will let you know once I have more info.

@faxioman
Copy link
Contributor Author

faxioman commented May 6, 2020

Thanks @andresmgot. I was checking the failing integration test ... I could check it out. Does that work for you?

@andresmgot
Copy link
Contributor

sure, that'd be great

@faxioman
Copy link
Contributor Author

faxioman commented May 7, 2020

Ok, I also need to patch the get-info.js module.

@faxioman
Copy link
Contributor Author

faxioman commented May 8, 2020

@andresmgot I made some fixes and now all the integration tests are passing.
I have a problem in a unit test though. The test is "should return the URL in case a path is specified". The URL field is now available for an http trigger when executing "serverless info"... so my problem is the test :)
I really can't figure out how to fix this test in order to pass it. Can you help me? ;)
Thanks!



// Mock call to get.ingress
_.each(functions, f => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this the cause of the issue you are facing with the test. This is just a mock that returns the ingress if requested, why did you remove it?

Copy link
Contributor Author

@faxioman faxioman May 11, 2020

Choose a reason for hiding this comment

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

I removed it because the ingress is now created by kubeless ... so I think the serverless-kubeless plugin is no more responsible for the ingress creation. Right?

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 mean, the kubeless plugin is unaware of the ingress created by kubeless.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, well, the result at the end of the day is the same. An Ingress with the name of the function is created so that's what this mock is doing.

If you want to remove this, you need to create the mock to return the HTTP trigger instead and change the getInfo code to request the HTTP trigger instead. I think that is more complicated to do so, that's why I suggest to just leave this as it was but if you want to run the extra mile and adapt that code, that's also fine ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok ;)
We'll catch later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

“Later” is the new “next week” 😏 Sorry For the delay @andresmgot. I will work on this pr next monday.

@faxioman
Copy link
Contributor Author

@andresmgot now the getInfo method is based on the created http trigger.
I see an error in travis log ... but I think is unrelated to my changes.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Great job @faxioman! Can you bump the version of the package so this gets released? It's a feature release so let's use 0.10.0. Thank you!

I see an error in travis log ... but I think is unrelated to my changes

It's indeed unrelated.

'basic-auth-secret': options.ingress.basicAuthSecretName || '',
'cors-enable': options.ingress.cors || false,
'function-name': name,
gateway: options.ingress.class || 'nginx', // not working: https://github.com/kubeless/kubeless/issues/1124
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 is incorrect, right? it now works with these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no. The gateway option is ignored by the kubeless controller, as reported in this issue: vmware-archive/kubeless#1124
I added the gateway option so when the fix will be relased it will works as expected but until then, I added a workaround that adds the ingress class annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, right, that was an issue in the controller

@faxioman
Copy link
Contributor Author

@andresmgot version bumped

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants