Add HTTPS port in generated containers by Kubernetes #30302
Conversation
|
I'll review this one closely tomorrow |
This comment has been minimized.
This comment has been minimized.
|
The test failure does seem related |
Yep, it seems to be caused by a bug in Dekorate that I have just reported: dekorateio/dekorate#1123. What puzzles me is that the issue was random. Anyway, I already have a fix for Quarkus, so I will update this PR soon. |
|
PR updated |
This comment has been minimized.
This comment has been minimized.
|
Spotted this new issue in Dekorate related to the Route port: dekorateio/dekorate#1124. |
|
Should we perhaps wait to get a Dekorate release? |
Taking into account that this is very usual, I would say better to work around the issues and when bumping Dekorate with the fixes, remove the workarounds. Otherwise, it would delay many features in Quarkus. |
What do you have in mind? |
No, I meant other changes in these Quarkus extensions. That this is what I used to do when I found blockers. |
|
I would prefer we fix the issues in Dekorate first as this is a new feature so there is no real rush to get it in, but I won't object, as this is definitely useful work. |
I will be working on the issues in Dekorate. If we address them quickly, I'm also favored to wait for the next Dekorate release including these fixes and remove the workarounds from this pull request. |
|
Thanks! |
This comment has been minimized.
This comment has been minimized.
|
Pull request that bumps Dekorate: #30351 |
|
PR updated using the latest Dekorate release with the mentioned issues fixed. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@iocanel any chance you can take a look into this, please? |
|
Sure |
There was a problem hiding this comment.
Looks good to me.
I only have a minor naming concern on whether the property should be called target-port or target-port-name. In dekorate we went with the former, however I am having second thoughts, as in quarkus there are areas we are going to have one or the other (e.g. #30547).
I'm a bit reluctant of renaming the property to "target-port-name" because:
In case we want to also support port numbers, we can always add a new property "target-port-number" in a separated pull request (this should be done first in Dekorate, and also for routes). |
|
ping @iocanel |
Most importantly, it seems that in the kubernetes context |
These changes will add an additional container port HTTPS in the generated manifests:
```yaml
containers:
- image: ...
imagePullPolicy: IfNotPresent
name: kubernetes-kind
ports:
- containerPort: 8080
name: http
protocol: TCP
- containerPort: 8443
name: https
protocol: TCP
```
By default, the Ingress and Route resources will use the "http" port. However, as part of these changes, I've added a new property to select between "https" or "http", or any other that user might have added as part of the configuration. Example:
```
quarkus.kubernetes.ingress.target-port=https
quarkus.openshift.route.target-port=https
```
Finally, note that the https container won't be added for the Knative resources since there seems to be a limitation at Knative side.
Fix quarkusio#29999
asd
|
🙈 The PR is closed and the preview is expired. |
|
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. |
These changes will add an additional container port HTTPS in the generated manifests:
By default, the Ingress and Route resources will use the "http" port. However, as part of these changes, I've added a new property to select between "https" or "http", or any other that user might have added as part of the configuration. Example:
Finally, note that the https container won't be added for the Knative resources since there seems to be a limitation at Knative side where only one port can be used.
Fix #29999