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

Initial plan for kubeapps backend API which can be used by 3rd parties. #1347

Merged
merged 4 commits into from
Dec 11, 2019

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Dec 5, 2019

Description of the change

Initial plan with stub handler for implementing a backend API which can be used for 3rd party integrations.

Benefits

See https://github.com/kubeapps/kubeapps/blob/master/docs/architecture/design-proposals/third-party-add-repository.md

Possible drawbacks

Until the helm3 work has settled, we don't have an obvious place for the backend API, so I'm including it in tiller-proxy for the moment, but using the frontend config to keep it independent (ie. we can easily move it later).

Applicable issues

Relates to: #1173

Additional information

There are a couple of questions I have regarding the direction, which I'll mention in-line.

@absoludity absoludity requested a review from andresmgot December 5, 2019 06:01
@@ -79,6 +79,18 @@ data:
{{- end }}
}

location ~* /api/backend {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the current config already defines things like /api/tillerdeploy and /api/chartsvc, so I need /api/<something> to maintain the same namespacing, but I'd actually prefer to just use /api rather than /api/backend.

Since this will be used by 3rd parties, prefixing with /backend doesn't really make sense or help. For example, this endpoint could/should just be /api/apprepositories. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the /backend part is not adding anything to the endpoint definition so I am okay using /api/apprepositories instead.

appreposHandler := handler.AppRepositories{}
backendAPIv1 := r.PathPrefix("/backend/v1").Subrouter()
backendAPIv1.Methods("POST").Path("/apprepositories").Handler(negroni.New(
authGate,
Copy link
Contributor Author

@absoludity absoludity Dec 5, 2019

Choose a reason for hiding this comment

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

I've used authGate here simply because it appears for all the other handlers, but I don't see why it's necessary here, as I'll be hitting the k8s API with the users credentials (with the id_token or access_token sent with the request), rather than something like tiller which runs with a service account. So I'll happily remove it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's probably not needed. I'd only see it useful if we do something (e.g. fetching credentials) before doing the actual request to the k8s API.

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.

I agree with your inline comments 👍

@@ -79,6 +79,18 @@ data:
{{- end }}
}

location ~* /api/backend {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the /backend part is not adding anything to the endpoint definition so I am okay using /api/apprepositories instead.

appreposHandler := handler.AppRepositories{}
backendAPIv1 := r.PathPrefix("/backend/v1").Subrouter()
backendAPIv1.Methods("POST").Path("/apprepositories").Handler(negroni.New(
authGate,
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's probably not needed. I'd only see it useful if we do something (e.g. fetching credentials) before doing the actual request to the k8s API.

@absoludity absoludity merged commit 24e83ae into vmware-tanzu:master Dec 11, 2019
@absoludity absoludity deleted the add-backend-handler branch December 11, 2019 01:29
SimonAlling added a commit to SimonAlling/kubeapps that referenced this pull request Dec 13, 2019
Suddenly, I couldn't get Kubeapps to start with `useHelm3: true`, making
me spend several hours trying various more or less random fixes. (I
thought I had messed something up with Docker or Minikube, so I didn't
think of bisecting or inspecting the YAML files.) The reason turned out
to be this unconditional proxy_pass directive introduced in vmware-tanzu#1347:

https://github.com/kubeapps/kubeapps/pull/1347/files#diff-b0b5b871c8bf5a72f95d417c97266ae0R100

I fixed it in vmware-tanzu#1382. This commit is intended to prevent something
similar from happening again; it's not at all obvious that the
proxy_pass directives must be conditional.
absoludity pushed a commit that referenced this pull request Dec 16, 2019
Suddenly, I couldn't get Kubeapps to start with `useHelm3: true`, making
me spend several hours trying various more or less random fixes. (I
thought I had messed something up with Docker or Minikube, so I didn't
think of bisecting or inspecting the YAML files.) The reason turned out
to be this unconditional proxy_pass directive introduced in #1347:

https://github.com/kubeapps/kubeapps/pull/1347/files#diff-b0b5b871c8bf5a72f95d417c97266ae0R100

I fixed it in #1382. This commit is intended to prevent something
similar from happening again; it's not at all obvious that the
proxy_pass directives must be conditional.
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