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

Add --ingress-class-precedence to allow IngressClass taking precedence over annotation #857

Merged
merged 1 commit into from
Sep 30, 2021
Merged

Add --ingress-class-precedence to allow IngressClass taking precedence over annotation #857

merged 1 commit into from
Sep 30, 2021

Conversation

mkubaczyk
Copy link
Contributor

@mkubaczyk mkubaczyk commented Sep 23, 2021

When defined as true, --controller-class-precedence with give a precedence to controller class over kubernetes.io/ingress.class annotation. This will be especially useful while migrating from one approach to another having zero downtime in mind. It can be removed once the support of ingress annotation is fully disabled.

I am here referring to situation when both IngressClassName and ingress annotation are defined in Ingress which results in
W0923 07:50:41.725353 7 cache.go:715] ingress prometheus/prometheus has conflicting ingress class configuration, using annotation reference (false) in haproxy ingress controller logs.

@mkubaczyk
Copy link
Contributor Author

I haven't add reviewers so requesting a review from @jcmoraisjr :-)

@jcmoraisjr
Copy link
Owner

Thanks for the contribution and sorry about taking so long to comment.

I'd suggest you two main changes:

		ingressClassPrecedence = flags.Bool("ingress-class-precedence", false,
			`Defines if IngressClass resource should take precedence over
kubernetes.io/ingress.class annotation if both are defined and conflicting.`)

This is a suggested change in the name and description with two benefits: better fits the purpose of the functionality and also avoids to change the whole struct, which leads to conflict when merging or cherry-picking.

The other suggestion is to try to add the warning message if cfg is true - ie using the functionality will only change the precedence in the case of a conflict, but still warning when the conflict happens.

Please let me know if you need anything, I'll answer faster this time =)

@mkubaczyk mkubaczyk changed the title Add --controller-class-precedence to allow controller class taking precedence over ingress annotation WIP: Add --controller-class-precedence to allow controller class taking precedence over ingress annotation Sep 29, 2021
@mkubaczyk mkubaczyk changed the title WIP: Add --controller-class-precedence to allow controller class taking precedence over ingress annotation Add --controller-class-precedence to allow controller class taking precedence over ingress annotation Sep 29, 2021
@mkubaczyk
Copy link
Contributor Author

@jcmoraisjr sounds reasonable. I updated the code. Please take a look to make sure I have not skipped some condition. :-)

@jcmoraisjr
Copy link
Owner

jcmoraisjr commented Sep 29, 2021

I ended up with this piece of code, it'll only add a new logic, compared with the original code, if the config is true. Please check if it makes sense to you:

	// c.cfg.IngressClassPrecedence as `true` gives precedence to IngressClass
	// if both class and annotation are configured and they conflict
	if hasAnn {
		if hasClass && fromAnn != fromClass {
			if c.cfg.IngressClassPrecedence {
				c.logger.Warn("ingress %s/%s has conflicting ingress class configuration, "+
					"using ingress class reference because of --ingress-class-precedence enabled (%t)",
					ing.Namespace, ing.Name, fromClass)
				return fromClass
			}
			c.logger.Warn("ingress %s/%s has conflicting ingress class configuration, using annotation reference (%t)",
				ing.Namespace, ing.Name, fromAnn)
		}
		return fromAnn
	}

You can also add the starting version, this pr will be cherry-picked to v0.13:

| [`--ingress-class-precedence`](#ingress-class)          | [true\|false]              | `false`                 | v0.13.5 |

Adjust the alignment in the Configuration struct, both in controller (declaration) and launch (initialization) files.

And finally, try to squash all the resulting commits into a single one.

Thanks!

@mkubaczyk
Copy link
Contributor Author

@jcmoraisjr should be fine now

@jcmoraisjr
Copy link
Owner

Great, lgtm, thanks! Merging now.

@jcmoraisjr jcmoraisjr changed the title Add --controller-class-precedence to allow controller class taking precedence over ingress annotation Add --ingress-class-precedence to allow IngressClass taking precedence over annotation Sep 30, 2021
@jcmoraisjr jcmoraisjr merged commit 7937952 into jcmoraisjr:master Sep 30, 2021
@mkubaczyk mkubaczyk deleted the controller-class-precedence branch October 1, 2021 06:35
@mkubaczyk
Copy link
Contributor Author

@jcmoraisjr are you planing to release some new minor version that would also contain this change? :-)

@jcmoraisjr
Copy link
Owner

Hi, sorry about the delay in the response. v0.13.5 was just released and has this feature merged.

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

Successfully merging this pull request may close these issues.

2 participants