Added support for falling back to native ingress if not on Openshift

[shannon]

This PR adds support for falling back to native kubernetes ingress objects if not on openshift. It determines this based on whether the 'route.openshift.io/v1' api is present.

I'm not very proficient in Ruby so please review carefully.

[akostadinov]

Looks reasonable on quick look. Are you still interested to move this forward?

[shannon]

@akostadinov Yea I think so. This would be helpful to be able to provide a fully automated certificate configuration using CertManager.

[akostadinov]

Shouldn't we better perform this call on demand instead of during initialization?

def use_openshift_route?
  @use_openshift_route ||= client.api_groups.include?('route.openshift.io/v1')
end

[akostadinov]

Also we could allow users to choose an ingress regardless of route availability. It better be done as a separate PR if desired.

[shannon]

Shouldn't we better perform this call on demand instead of during initialization?

def use_openshift_route?
  @use_openshift_route ||= client.api_groups.include?('route.openshift.io/v1')
end

@akostadinov well I had assumed that the environment wouldn't change. I can't imagine a scenario where zync would be running in kubernetes and then the openshift API route.openshift.io/v1 would be added after. So I figured once at init time would be sufficient. Or am I misunderstanding what you mean?

[akostadinov]

The point is not to change value later. The ``||=` operator will prevent that in either case. The point is not to do operations at object initialization, rather at the time that operation is needed.

At present it may not make much difference. But as a matter of good design. Also one can mock a method much more easily in testing.

[shannon]

I updated this, but I'm still not sure I understand the correct way to do this in Ruby classes. I kept having issues using @use_openshift_route so I just changed it to return the value. I also added an env variable to force native ingresses because it was simple enough to add it right there.

[akostadinov]

Better move environment setup to a before all block. As far as I can trust my eyes the environment for both test cases is identical.

[shannon]

@akostadinov You are correct, they are the same and I understand your point. However, I am not really familiar with writing tests in Ruby. Is something like this sufficient?

class Integration::KubernetesServiceTest < ActiveSupport::TestCase
  include Base64

  def before_setup
    @_env = ENV.to_hash
    super
  end

  def after_teardown
    ENV.replace(@_env)
    super
  end

  before do
    ENV['KUBERNETES_NAMESPACE'] = 'zync'
    ENV['KUBE_TOKEN'] = strict_encode64('token')
    ENV['KUBE_SERVER'] = 'http://localhost'
    ENV['KUBE_CA'] = encode64 <<~CERTIFICATE
      -----BEGIN CERTIFICATE-----
      MIIBZjCCAQ2gAwIBAgIQBHMSmrmlj2QTqgFRa+HP3DAKBggqhkjOPQQDAjASMRAw
      DgYDVQQDEwdyb290LWNhMB4XDTE5MDQwNDExMzI1OVoXDTI5MDQwMTExMzI1OVow
      EjEQMA4GA1UEAxMHcm9vdC1jYTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABGG2
      NDgiBuXNVWVVxrDNVjPsKm14wg76w4830Zn3K24u03LJthzsB3RPJN9l+kM7ryjg
      dCenDYANVabMMQEy2iGjRTBDMA4GA1UdDwEB/wQEAwIBBjASBgNVHRMBAf8ECDAG
      AQH/AgEBMB0GA1UdDgQWBBRfJt1t0sAlUMBwfeTWVv2v4XNcNjAKBggqhkjOPQQD
      AgNHADBEAiB+MlaTocrG33AiOE8TrH4N2gVrDBo2fAyJ1qDmjxhWvAIgPOoAoWQ9
      qwUVj52L6/Ptj0Tn4Mt6u+bdVr6jEXkZ8f0=
      -----END CERTIFICATE-----
    CERTIFICATE
  end
  ...

It's a bit unclear to me how before do works in relation to say def before_setup. And which would be more appropriate here.

[akostadinov]

Should work. Also changing env in before_setup should also be fine. It will do once for all tests in the class. But make sure to put it after @_env = ENV.to_hash.

[shannon]

It's been so long and I need to rebase from master anyways at this point. It looks like that particular block is in a setup do now. So I will do that now.

[shannon]

Yea that merge didn't go the way I wanted it to. I think I am going to restart from master and just change the two files independently.

[shannon]

@akostadinov one thing I am noticing as I test this in my new environment is that the Kubernetes Nginx Ingress controller now requires a ingress class. Before it would just chose the default. See here. This wasn't required at the time I originally created this PR. So I think we should probably have an environment variable to specify the ingress class. WDYT?

[mayorova]

I'm wondering if we could create two separate subclasses: one for OpenShift service, and another one for plain Kubernetes, and override the methods where the if use_openshift_route? check is currently done.
Then maybe we could select the correct service in DiscoverIntegrationService...

[thalesmiguel]

I agree with @mayorova.
Adding this use_openshift_route option forced us to add ifs on multiple methods that that now have to have a different behaviour depending on this new option.
Rather than doing that, since there are multiple changes to be done, we could define a new class with all the behaviour we expect when we talk about OpenShift routes and leave this one to care about Kubernetes.
With this, we'd have different points in code responsible for each scenario.

[shannon]

I had originally intended to do it this way but it required me to touch more of the code base than I was comfortable with. @mayorova Could you maybe elaborate or give an example of how I would select Openshift vs Kubernetes inside DiscoverIntegrationService?