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.

Sign up for GitHub

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

Jump to bottom

Ingress and Route generation refactor. #1730

Merged

rohanKanojia merged 2 commits into fabric8io:master from devang-gaur:ingress_corrections

Jan 3, 2020

Contributor

devang-gaur commented Oct 11, 2019

No description provided.

devang-gaur added the pr/wip label

devang-gaur requested a review from rohanKanojia

October 11, 2019 09:18

devang-gaur force-pushed the ingress_corrections branch from 3ad3504 to 6ba41b4 Compare

October 11, 2019 23:39

devang-gaur force-pushed the ingress_corrections branch 2 times, most recently from 7d79251 to cb35f85 Compare

November 8, 2019 14:12

devang-gaur force-pushed the ingress_corrections branch from cb35f85 to 3909e98 Compare

November 18, 2019 06:22

devang-gaur force-pushed the ingress_corrections branch 2 times, most recently from 6e1d279 to 3b34467 Compare

November 28, 2019 09:10

devang-gaur commented

View reviewed changes

core/src/main/java/io/fabric8/maven/core/config/ResourceConfig.java

@@ @@ -92,7 +92,10 @@ @@
                   @Parameter
                   private List<ServiceAccountConfig> serviceAccounts;
-                  private List<IngressRule> ingressRules;

Contributor Author

devang-gaur Nov 28, 2019

I added this wrongfully in my previous PR here: dc5b213#diff-9238e96d57160d3b61190b609607b2d8R95

hence, removing it

Member

rohanKanojia Dec 20, 2019

Remove import above too

devang-gaur force-pushed the ingress_corrections branch from 00e5a62 to c2412cb Compare

November 28, 2019 11:57

Devang Gaur added 2 commits

November 29, 2019 19:21


          Fixed IngressEnricher and RouteEnricher

05d6c2f


          Refactor ApplyMojo

cc5c946

devang-gaur force-pushed the ingress_corrections branch from c2412cb to cc5c946 Compare

November 29, 2019 13:52

devang-gaur requested a review from manusa

December 1, 2019 22:23

Contributor Author

devang-gaur commented Dec 1, 2019

@ro14nd @rohanKanojia Do we want the IngressEnricher to be included in the default profile for the default yaml manifests generation, like Route for OpenShift?

Or should there be a flag/mojo parameter for eg., enableIngress , expose = true to enable invokation of RouteEnricher and IngressEnricher ?

In simpler words, do we want Ingress.yaml to be generated by default for Kubernetes runtime mode?
@manusa

devang-gaur removed the pr/wip label

manusa reviewed

View reviewed changes

plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/ResourceMojo.java Show resolved Hide resolved

manusa reviewed

View reviewed changes

enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/IngressEnricher.java Show resolved Hide resolved

manusa reviewed

View reviewed changes

enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/openshift/RouteEnricher.java Show resolved Hide resolved

rohanKanojia requested changes

View reviewed changes

Member

rohanKanojia left a comment

Thanks, looks good to me.

It would be nice to add some documentation on how to generate Ingress just like we have for routes: http://maven.fabric8.io/#resource-route-generation . Maybe we can add some sample also which can demonstrate Ingress generation?
We are generating ingress even when there is no IngressController present. However, it's not the responsibility of plugin to add IngressController and should be done by cluster admins. But maybe we should at least check whether there is some IngressController present in the cluster or not, WDYT?

enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/IngressEnricher.java

@@ @@ -23,19 +23,24 @@ @@
               import io.fabric8.kubernetes.api.model.ServiceBuilder;
               import io.fabric8.kubernetes.api.model.ServicePort;
               import io.fabric8.kubernetes.api.model.ServiceSpec;
-              import io.fabric8.kubernetes.api.model.extensions.Ingress;
+              import io.fabric8.kubernetes.api.model.extensions.HTTPIngressPath;

Member

rohanKanojia Dec 20, 2019

Unused import

Contributor Author

devang-gaur Jan 2, 2020

Oh I thought intellij does that automatically. need to check my ide configuration

enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/IngressEnricher.java

                       }
                       return false;
                   }
+                  /**
+                   * Should we try to create an external URL for the given service?

Member

rohanKanojia Dec 20, 2019

Either complete this javadoc or remove it.

enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/IngressEnricher.java

+                   */
+                  private boolean shouldCreateExternalURLForService(ServiceBuilder service) {
+                      String serviceName = service.getMetadata().getName();
+                      if ("kubernetes".equals(serviceName) || "kubernetes-ro".equals(serviceName)) {

Member

rohanKanojia Dec 20, 2019

Why are we comparing Service names here like this? Do we even generate kubernetes service while resource processing? What's with this kubernetes-ro service?

Contributor Author

devang-gaur Jan 8, 2020

I think these are the prefixes for the potential services generated by kubernetes system itself. I took it from the apply mojo.

enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/IngressEnricher.java

+                      }
+                      ServiceSpec spec = service.getSpec();
+                      List<ServicePort> ports = spec.getPorts();
+                      log.debug("Service " + serviceName + " has ports: " + ports);

Member

rohanKanojia Dec 20, 2019

Is dumping whole object into log statement fine?

[DEBUG] F8: fmp-ingress: Service fabric8-maven-sample-spring-boot has ports: [ServicePort(name=http, nodePort=null, port=8080, protocol=TCP, targetPort=IntOrString(IntVal=8080, Kind=0, StrVal=null, additionalProperties={}), additionalProperties={})]

Contributor Author

devang-gaur Jan 8, 2020

I thought its appropriate for log.debug.

enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/openshift/RouteEnricher.java

-                  protected boolean isExposedService(ServiceBuilder serviceBuilder) {
-                      Service service = serviceBuilder.build();
-                      return isExposedService(service);
+                  private ObjectMeta removeExposeLabel(ObjectMeta metadata) {

Member

rohanKanojia Dec 20, 2019

I've seen this method in IngressEnricher also. Could we please avoid this code duplication?

plugin/src/main/java/io/fabric8/maven/plugin/enricher/EnricherManager.java

                   /**
                    * Allow enricher to add Metadata to the resources.
-                   *
+                   * @param platformMode

Member

rohanKanojia Dec 20, 2019

incomplete javadoc

core/src/main/java/io/fabric8/maven/core/config/ResourceConfig.java

@@ @@ -92,7 +92,10 @@ @@
                   @Parameter
                   private List<ServiceAccountConfig> serviceAccounts;
-                  private List<IngressRule> ingressRules;

Member

rohanKanojia Dec 20, 2019

Remove import above too

plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/ResourceMojo.java Show resolved Hide resolved

manusa approved these changes

View reviewed changes

Member

manusa left a comment

LGTM

rohanKanojia merged commit 818b653 into fabric8io:master

Contributor Author

devang-gaur commented Jan 6, 2020

Contributor Author

devang-gaur commented Jan 8, 2020

We need to deduce a logic for finding/detecting kubernetes-controllers inside the cluster, as they are not a namespaced resource. In minikube, the default nginx-ingress-controller deployment is in the kube-system namespace but is it a globally followed arrangement?

devang-gaur added the jkube/pending label

rohanKanojia mentioned this pull request

Fix #184: IngressEnricher seems to be broken eclipse-jkube/jkube#213

Merged

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request


          Fix eclipse-jkube#184: IngressEnricher seems to be broken

Ported PR fabric8io/fabric8-maven-plugin#1730

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request


          Fix eclipse-jkube#184: IngressEnricher seems to be broken

97f8e22

Ported PR fabric8io/fabric8-maven-plugin#1730

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request


          Fix eclipse-jkube#184: IngressEnricher seems to be broken

ca6c7a3

Ported PR fabric8io/fabric8-maven-plugin#1730

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request


          Fix eclipse-jkube#184: IngressEnricher seems to be broken

19c800c

Ported PR fabric8io/fabric8-maven-plugin#1730

rohanKanojia added a commit to rohanKanojia/jkube that referenced this pull request


          Fix eclipse-jkube#184: IngressEnricher seems to be broken

d3f6e45

Ported PR fabric8io/fabric8-maven-plugin#1730

manusa pushed a commit to eclipse-jkube/jkube that referenced this pull request


          Fix #184: IngressEnricher seems to be broken

e11392a

Ported PR fabric8io/fabric8-maven-plugin#1730

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment