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

Allow the user to set the ingress apiVersion #245

Closed
wants to merge 1 commit into from
Closed

Allow the user to set the ingress apiVersion #245

wants to merge 1 commit into from

Conversation

bhundven
Copy link

@bhundven bhundven commented Apr 24, 2021

In #205, newer ingress is broken by not using networking.k8s.io/v1,
but this may not be the case for all users.

Allow apiVersion to be set by the user.

Default to networking.k8s.io/v1, but add a note in the README.md about
older k8s or ingress-nginx to use networking.k8s.io/v1beta1.

Signed-off-by: Bryan Hundven [email protected]

@bhundven
Copy link
Author

@shanemcd lmk if you need any other changes. I'm happy to update this quickly.

@shanemcd
Copy link
Member

Unfortunately I don't think it will be so simple. When I tried to use this, I saw:

failed: [localhost] (item=tower_ingress) => changed=false
  ansible_loop_var: item
  error: 422
  item: tower_ingress
  msg: 'Failed to apply object: b''{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Ingress.extensions \\"awx-ingress\\" is invalid: spec.rules[0].http.paths[0].pathType: Required value: pathType must be specified","reason":"Invalid","details":{"name":"awx-ingress","group":"extensions","kind":"Ingress","causes":[{"reason":"FieldValueRequired","message":"Required value: pathType must be specified","field":"spec.rules[0].http.paths[0].pathType"}]},"code":422}\n'''
  reason: Unprocessable Entity
  status: 422

Per the k8s docs, it needs to look like this now:

spec:
  rules:
    - host: '{{ tower_hostname }}'
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: '{{ meta.name }}-service'
                port:
                  number: 80

At which point it will only work for networking.k8s.io/v1, so there's really no point to make it configurable.

@shanemcd
Copy link
Member

I should note that even when I made the above changes I still see the webhook error mentioned in #205

@bhundven
Copy link
Author

Well, we can update the template to switch the spec based on which apiVersion is set, but yea... we will have to wait for the next version of minikube.

@bhundven
Copy link
Author

I also removed the 'Closes' from the commit message and PR so it doesn't close 205

Copy link
Member

@shanemcd shanemcd left a comment

Choose a reason for hiding this comment

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

Nice work 😄

Copy link
Contributor

@tchellomello tchellomello left a comment

Choose a reason for hiding this comment

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

@bhundven thanks for great work.

I would say that would be nice to have this option also exposed as a hidden field at https://github.com/ansible/awx-operator/blob/devel/deploy/olm-catalog/awx-operator/manifests/awx-operator.clusterserviceversion.yaml so users can use configure it via UI as well.

Do you mind adding it for future use? You might need to extend the awxs.awx.ansible.com CDR as well.

But I'm ok if @shanemcd wants to merge like this since we can revisit this if needed in the future.

@bhundven
Copy link
Author

I will do that this evening.

@bhundven
Copy link
Author

@tchellomello Like that?

@bhundven
Copy link
Author

bhundven commented May 4, 2021

I can't see why test-local molecule scenario is failing. Could I get some help understanding what is going wrong? @shanemcd I think I fixed this later with the fourth commit: Add missing type for apiversion to awx_v1beta1_crd

@bhundven
Copy link
Author

bhundven commented May 4, 2021

Forgot to add type: string to tower_ingress_apiversion in awx_v1beta1_crd.yml. Also rebased against latest devel.
@tchellomello Would you review?

@bhundven
Copy link
Author

bhundven commented May 4, 2021

I dropped the commit to try and fix the molecule int to string conversion warning. I think it broke the pipeline.

@bhundven
Copy link
Author

I am not sure why this is looping and never coming up? Is there something in molecule that needs to be updated?

@bhundven
Copy link
Author

I ran the test locally, and got this during prepare:

TASK [Fetch the kubeconfig] ****************************************************
fatal: [kind-test-local]: FAILED! => {"changed": false, "file": "/root/.kube/config", "msg": "unable to calculate the checksum of the remote file"}

@bhundven
Copy link
Author

That was fixed with a rebase. Now getting Failed to find exact match for networking.k8s.io/v1.Ingress by [kind, name, singularName, shortNames] when set to extensions/v1beta1, and Failed to find exact match for .Ingress by [kind, name, singularName, shortNames] when set to networking.k8s.io/v1. So I'm trying to sort that out now.

@bhundven
Copy link
Author

Ok, I fixed that, this passed on my test-local setup.

In #205, newer ingress is broken by not using networking.k8s.io/v1,
but this may not be the case for all users.

Allow apiVersion to be set by the user.

Default to networking.k8s.io/v1, but add a note in the README.md about
older k8s or ingress-nginx to use networking.k8s.io/v1beta1.

Signed-off-by: Bryan Hundven <[email protected]>
@bhundven bhundven changed the title Allow tower_ingress_apiversion to be set by the user Allow the user to set the ingress apiVersion May 25, 2021
@bhundven bhundven closed this Jun 1, 2021
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.

3 participants