add container_index for service object

prahaladramji commented 3 years ago

The current helper only works for the first container, having this index hidden variable allows us to expose services from a second sidecar type container in the same pod.

dbarranco commented 3 years ago

I don't have any concern with this PR. Out of curiosity, which is your use case?

Thanks for the contribution! (let's wait until all reviewers accept this PR :) )

dbarranco commented 3 years ago

that's right. Maybe we should block this PR until some tests are added.

prahaladramji commented 3 years ago

Out of curiosity, which is your use case?

so we have a grafana deployment with a sidecar and that side car also exposes a http endpoint that we'd like to make available with a service definition.

grafana_sc_service: kube.Service($.name + '-sc') {
    metadata+: {
      namespace: $.namespace,
      labels+: {
        app: 'grafana',
        component: 'grafana-sc',
      },
    },
    target_pod: $.grafana.spec.template,
    container_index: 1,
    port: 80,
  },

in the deployment spec we use

...
containers_+: {
  grafana: kube.Container($.name) {....},
  'grafana-sc': kube.Container($name + '-sc') {...},
  'grafana-sc-dashboard': kube.Container($name + '-sc-dashboard') {...},
}

With the current implementation of kube-libsonnet the only way was to fully define the service block to choose the second container, because the index is hard coded to only use the first container.

This PR basically makes the index configurable so we can use the same helper to target a specific container when defining the service.

Not the most perfect solution, would be better to be able to target the service definition based on a named index. But i don't yet know enough of jsonnet to refactor that. If i did will make another pr.

prahaladramji commented 3 years ago

Thanks for the contrib!! SGTM, but can you add some tests to assert the code works?

Will do, Do you mean assert at the lib level? (something like test if it's numeric and less than the length of the number of containers)

Or do you mean additional test cases which runs the test for the repo. (this i'd have to spend some time understanding how it's done and can do so). But would need some help with direction.

I will have a look sometime later or in the weekend and update the pr.

prahaladramji commented 3 years ago

Hi, sorry it took a while, just following up on some help/direction on writing these tests specific to this change?

I can add asserts in the libsonnet to verify that the field passed is an int. Possibly assert if the provided index is greater than the available containers. (may be possible to check the length of the deployment spec.containers)
For adding test cases, do i just create a couple of pass/fail jsonnets to test this var? (i'll give it a crack but if you can point me to an example it'd be helpful).

Thanks.

jjo commented 3 years ago

Hi, sorry it took a while, just following up on some help/direction on writing these tests specific to this change?

I can add asserts in the libsonnet to verify that the field passed is an int. Possibly assert if the provided index is greater than the available containers. (may be possible to check the length of the deployment spec.containers)

It'd be good to have that assertion, but OTOH IMO whoever is overloading container_index does know what they're doing, thus a jsonnet "index out of range" error would be obvious for them. I'd rather focus on adding the test cases (more below).

For adding test cases, do i just create a couple of pass/fail jsonnets to test this var? (i'll give it a crack but if you can point me to an example it'd be helpful).

Below two test cases should serve as a good start (just remove the ingress stanzas, and adapt the pod/service there):

Thanks.

prahaladramji commented 3 years ago

Thanks @jjo, I've added 2 tests for selecting the container by index for service definition. Apologies for the delay.

prahaladramji commented 3 years ago

@jbianquetti-nami @dbarranco please review/merge when you get the chance. Thank you. once again apologies for the delay in finishing up this PR.

dbarranco commented 3 years ago

Thanks a lot for the effort! @prahaladramji

Will add this PR to my to-do list for this week, let's see if I have some time during this week :)

bfarayev commented 3 years ago

just following up to see if there's a chance to merge it at some point soon? :)

prahaladramji commented 2 years ago

Just following up/bumping this up again? Thanks @jbianquetti-nami @dbarranco

bitnami / kube-libsonnet

add container_index for service object #53