-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Gh17060 gateway single host kube #17557
Gh17060 gateway single host kube #17557
Conversation
Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
.../workspace/infrastructure/kubernetes/server/external/TraefikGatewayRouteConfigGenerator.java
Outdated
Show resolved
Hide resolved
...rg/eclipse/che/workspace/infrastructure/kubernetes/server/external/GatewayServerExposer.java
Show resolved
Hide resolved
StringWriter sw = new StringWriter(); | ||
|
||
try { | ||
YAMLGenerator generator = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about using YAMLGenerator
. We could use ObjectMapper
, but then we would need to create all objects in configuration yaml or use multiple levels of Map<String, Map<String, Map<...
.
So I don't think it's so bad... Or any better idea?
Signed-off-by: Michal Vala <mvala@redhat.com>
...rg/eclipse/che/workspace/infrastructure/kubernetes/server/external/GatewayServerExposer.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Michal Vala <mvala@redhat.com>
...rg/eclipse/che/workspace/infrastructure/kubernetes/server/external/GatewayServerExposer.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Michal Vala <mvala@redhat.com>
...rg/eclipse/che/workspace/infrastructure/kubernetes/server/external/GatewayServerExposer.java
Outdated
Show resolved
Hide resolved
setting it back to WIP as getting workspace namespace into |
Signed-off-by: Michal Vala <mvala@redhat.com>
I've updated the PR with creating Gateway route configmaps in che namespace. There are lot of stuff missing, but I'd like to know opinion about general idea here, especially Biggest part missing is cleanup of "che" namespace. I'm afraid I'll have to again create Thoughts? Ideas for cleaner solution? |
.endMetadata() | ||
.withData(gatewayRouteConfigGenerator.generate()); | ||
|
||
ConfigMap routeConfigMap = cheNamespace.configMaps().create(configMapBuilder.build()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I afraid you will face the permission problem accessing che namespace, which i have faced during search of CA configmap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. I'm afraid we would have to somehow create that configmap in namespace with gateway pod. If we won't have permissions for that, we would have to add it. I'll look what permissions we have and if we need more. Alternatively, we could configure configbump to watch configmaps on all namespaces, or namespaces with running workspaces, which would require higher permissions on configbump SA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a big problem to require the che SA to be able to crud configmaps in the che namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works with current helm install (we use cluster-admin
). With operator, we currently can't use different namespaces anyway because lack of permissions and in one namespace it works fine.
So last option is OpenShift OAuth. I didn't try it, but this task is about Kubernetes so I would leave this potential permissions issue to #17061
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Biggest part missing is cleanup of "che" namespace. I'm afraid I'll have to again create KubernetesNamespace for "che" namespace and workspaceId and do cleanup on it.
Should we create something like CheServerKubernetesNamespace
to encapsulate logic that might be needed che server to work with k8s object on his namespace?
I think direction is ok. I would like to hear @metlos and @sleshchenko option before the merge too.
.../main/java/org/eclipse/che/workspace/infrastructure/kubernetes/GatewayRouterProvisioner.java
Outdated
Show resolved
Hide resolved
.../workspace/infrastructure/kubernetes/server/external/TraefikGatewayRouteConfigGenerator.java
Outdated
Show resolved
Hide resolved
I think it's worth to have |
Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
...main/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesInternalRuntime.java
Outdated
Show resolved
Hide resolved
...ain/java/org/eclipse/che/workspace/infrastructure/kubernetes/CheKubernetesClientFactory.java
Outdated
Show resolved
Hide resolved
… and move method to check che namespace annotation into util class Signed-off-by: Michal Vala <mvala@redhat.com>
Signed-off-by: Michal Vala <mvala@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the overall approach is good. My only comment would be that the gateway provisioning is not encapsulated too well, because the fact that we're using configmaps to "materialize" the gateway config is omnipresent. I would imagine that KubernetesInternalRuntime
would be completely unwaware of what the individual provisioners "do" to the environment yet we assume there that some configmaps might be deployed to the che namespace, which seems to me like a quite concrete assumption.
That said, I don't consider the above to be a blocker - we might abstract this if/when the need arises.
Yes, but |
# Conflicts: # infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesEnvironmentProvisionerTest.java
What does this PR do?
Implements Traefik based Gateway configuration on Kubernetes.
❗ This PR introduces one breaking change. This class https://github.com/eclipse/che/pull/17557/files#diff-8855fac8bb68d106dd25037d440a3e22 (previously was
OpenShiftCheInstallationLocation
https://github.com/eclipse/che/blob/master/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java) now fails when neitherKUBERNETES_NAMESPACE
orPOD_NAMESPACE
is set. Previously it returned null. The reason for it is that we need to know che installation namespace here and can't work without it so we should fail fast (at startup). The installers (che-operator, helm) are responsible to set one of these env variables. If there will be any failures, it should be fixed on their side.What issues does this PR fix or reference?
#17060
Release Notes
Docs PR