Skip to content
This repository was archived by the owner on May 24, 2023. It is now read-only.

Add default name to IngressClass and tidy SCC handling #130

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

soneillf5
Copy link
Contributor

@soneillf5 soneillf5 commented Jul 5, 2021

Proposed changes

Fixes deletion bug, using the default name for the ingress class for deletion and creation.

Minor refactor to SCC resource handling.

*** UPDATED ***

I've removed the code that deletes the ingress class. This resource shouldn't be deleted when an IC is deleted, it should be removed when the operator is deleted.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@github-actions github-actions bot added the enhancement Pull requests for new features/feature enhancements label Jul 5, 2021
@soneillf5 soneillf5 requested review from lucacome and ciarams87 July 6, 2021 13:18
Copy link
Member

@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

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

Actually after testing it out, this approach may not work - deleting the default ingressClass (a global resource) on a single ingress controller deletion will lead to difficulty when we have several ICs sharing the same resource.

@lucacome
Copy link
Contributor

lucacome commented Jul 6, 2021

I'm a little bit confused. What's the deletion bug?

@soneillf5 soneillf5 requested a review from ciarams87 July 7, 2021 09:12
@soneillf5
Copy link
Contributor Author

soneillf5 commented Jul 7, 2021

@lucacome Sorry for the lack of description, the deletion bug is the following sequence:

  • An IngressController is created
  • 'controllers/prerequisites.go:18' checkPrerequisites is called before it's created
  • this function creates an IngressClass if it doesn't exist. (L61)
  • If the name of the IngressClass is not specified, it uses the default name 'nginx'
  • A finaliser is added to CR resources, in this case an IngressController
  • The IC is deleted
  • In the reconcile loop, we see a finaliser needs to be addressed so 'finalizeNginxIngressController' is called
  • This tries to delete the IngressClass but it fails as it can't find it an IngressClass with no name. (BUG HERE)
  • The failed deletion causes the finaliser to exit with an error
  • the reconcile loop ignores the error, leaving the finalizer
  • the reconcile loop is Called again and fails to delete the resource ( hence the loop)

Essentially, the deletion code isn't add the default value for the IngressClass name that the addition code did.

@soneillf5 soneillf5 requested a review from vepatel July 7, 2021 09:24
@soneillf5 soneillf5 force-pushed the feature/ingressclass-bug branch from 882ebd1 to 922eae5 Compare July 7, 2021 11:49
@soneillf5 soneillf5 enabled auto-merge (rebase) July 7, 2021 11:50
@soneillf5 soneillf5 merged commit 607365b into master Jul 7, 2021
@soneillf5 soneillf5 deleted the feature/ingressclass-bug branch July 7, 2021 11:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants