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

fix(trait): controller strategy default service port name #5411

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

squakez
Copy link
Contributor

@squakez squakez commented Apr 24, 2024

Using a func that determine the default port name based on the controller strategy adopted.

Close #5409

Release Note

fix(trait): controller strategy default service port name

@squakez squakez added the kind/bug Something isn't working label Apr 24, 2024
@squakez
Copy link
Contributor Author

squakez commented Apr 24, 2024

@lburgazzoli do you know if this change may create any problem for other deployment strategy or other clusters (ie, Openshift) by any chance?

@squakez squakez force-pushed the fix/prometheus_fail branch from 9739592 to 1458d8e Compare April 24, 2024 08:50
@lburgazzoli
Copy link
Contributor

In general if you use a service, it should not be an issue, however it may break customer deployment if they were using such name in any external tool for any reason i.e. in prometheus' PodMonitor

apiVersion: monitoring.coreos.com/v1
kind: PodMonitor
metadata:
  name: example-app
  labels:
    team: frontend
spec:
  selector:
    matchLabels:
      app: example-app
  podMetricsEndpoints:
  - port: http

I wonder if we should make this change knative only at least for now and eventually failing the integration if the use changes the port name for a Knative deployment.

@lburgazzoli
Copy link
Contributor

As side note, this would probably cause all the integration Pods to be restarted when the operator gets upgraded

@squakez
Copy link
Contributor Author

squakez commented Apr 24, 2024

In general if you use a service, it should not be an issue, however it may break customer deployment if they were using such name in any external tool for any reason i.e. in prometheus' PodMonitor

Yes, if the monitoring is external, then, we cannot control that. I'll move this change to apply exclusively to Knative, however, there are several places where this is happening, so we need to make more verification about it.

@squakez squakez force-pushed the fix/prometheus_fail branch from 1458d8e to 16476ba Compare April 25, 2024 10:06
@squakez squakez changed the title fix(trait): harmonize service port name fix(trait): controller strategy default service port name Apr 25, 2024
@squakez
Copy link
Contributor Author

squakez commented Apr 25, 2024

Should be good to go now. I've worked to define a more generic approach and let any trait requiring to know the port name to use a function that will return the default based on the controller strategy used. It should be compatible with the existing Deployment as will still return http conversely to the previous commit.

@squakez squakez force-pushed the fix/prometheus_fail branch 2 times, most recently from 5a6889c to b71cae3 Compare April 25, 2024 14:24
Using a func that determine the default port name based on the controller strategy adopted

Close apache#5409
@squakez squakez force-pushed the fix/prometheus_fail branch from b71cae3 to 3d16f28 Compare April 26, 2024 08:36
@squakez squakez merged commit 698b470 into apache:main Apr 26, 2024
14 checks passed
@squakez squakez deleted the fix/prometheus_fail branch April 26, 2024 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Knative accepted port names
2 participants