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

(#5635) Add TLS configuration in the ingress trait #5650

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

romain-pfund
Copy link
Contributor

Add

  • tlsHosts
  • tlsSecretName
    to configure TLS in the ingress trait

Release Note

Ingress Trait: Add tlsHosts and tlsSecretName to configure the TLS ingress spec

pkg/trait/ingress.go Outdated Show resolved Hide resolved
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the work. We need also to add the outcome of make generate though.

@squakez squakez linked an issue Jun 17, 2024 that may be closed by this pull request
@romain-pfund
Copy link
Contributor Author

I haven't had the time to fully check with a working certificat

but the kubect get ingress xxx -o yaml seams fine when comparing to an internal working ingress (not showing here for confidentiality reason):

image

image
image

@romain-pfund
Copy link
Contributor Author

romain-pfund commented Jun 17, 2024

LGTM, thanks for the work. We need also to add the outcome of make generate though.

ok, so I run

make
make generate

and commit/push ?

@squakez
Copy link
Contributor

squakez commented Jun 17, 2024

I haven't had the time to fully check with a working certificat

but the kubect get ingress xxx -o yaml seams fine when comparing to an internal working ingress (not showing here for confidentiality reason):

Sure, the important work here is to map into the configuration expected by Kubernetes.

@romain-pfund romain-pfund marked this pull request as ready for review June 17, 2024 14:23
@squakez
Copy link
Contributor

squakez commented Jun 17, 2024

make
make generate

and commit/push ?

Yes, you should see several autogenerated resources. They must be added to the PR, you can git commit --amend and force push this. Also, you can see there is some minor lint error due to codestyle formatting. The check is failing with:

pkg/trait/ingress.go:22  goimports  File is not `goimports`-ed

You can perform a gofmt -w pkg/trait/ingress.go and later a make lint to verify that is solved afterward. And again, commit the change (likely some space format).

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, remove this one from the commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@romain-pfund romain-pfund force-pushed the feature/trait-ingress-tls branch from ba9fc58 to f5b2836 Compare June 17, 2024 14:40
@romain-pfund
Copy link
Contributor Author

Some builds are failing
Did i missed something ?

@squakez
Copy link
Contributor

squakez commented Jun 18, 2024

Some builds are failing Did i missed something ?

Install check is failing because of #5632 (I'm working on a fix). The rest should be working normally. I gave it another shot.

@squakez squakez merged commit 39bdbd1 into apache:main Jun 18, 2024
13 of 14 checks passed
@squakez
Copy link
Contributor

squakez commented Jun 18, 2024

Thanks for the contribution!

@romain-pfund romain-pfund deleted the feature/trait-ingress-tls branch June 18, 2024 15:39
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.

Add TLS configuration in the ingress trait
2 participants