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.

Sign up for GitHub

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

registration-service: new Service to /metrics #1051

Merged
merged 7 commits into from
Jul 2, 2024
Merged

registration-service: new Service to /metrics #1051

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
69b594f
registration-service: new Service to `/metrics`
xcoulon Jun 25, 2024
7e58490
remove unneeded change
xcoulon Jun 25, 2024
1f68b9f
Merge branch 'master' into regsvc_metrics_service
xcoulon Jun 27, 2024
d0b8564
Merge branch 'master' into regsvc_metrics_service
xcoulon Jul 2, 2024
ce07425
shorten port name
xcoulon Jul 2, 2024
c152198
fix tests
xcoulon Jul 2, 2024
c2aefe5
Merge branch 'master' into regsvc_metrics_service
xcoulon Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions controllers/toolchainconfig/toolchainconfig_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func TestReconcile(t *testing.T) {
Exists().
HasConditions(
toolchainconfig.ToSyncComplete(),
toolchainconfig.ToRegServiceDeploying("updated resources: [ServiceAccount: registration-service Role: registration-service RoleBinding: registration-service Deployment: registration-service Service: registration-service Route: registration-service Service: api Route: api Service: proxy-metrics-service]")).
toolchainconfig.ToRegServiceDeploying("updated resources: [ServiceAccount: registration-service Role: registration-service RoleBinding: registration-service Deployment: registration-service Route: registration-service Service: registration-service Service: registration-service-metrics Route: api Service: api Service: proxy-metrics-service]")).
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to your changes - I'm a bit confused with those conditions, not sure what are those for 🤔

HasNoSyncErrors()

// check member1 config
Expand Down Expand Up @@ -274,7 +274,7 @@ func TestReconcile(t *testing.T) {
Exists().
HasConditions(
toolchainconfig.ToSyncFailure(),
toolchainconfig.ToRegServiceDeploying("updated resources: [ServiceAccount: registration-service Role: registration-service RoleBinding: registration-service Deployment: registration-service Service: registration-service Route: registration-service Service: api Route: api Service: proxy-metrics-service]")).
toolchainconfig.ToRegServiceDeploying("updated resources: [ServiceAccount: registration-service Role: registration-service RoleBinding: registration-service Deployment: registration-service Route: registration-service Service: registration-service Service: registration-service-metrics Route: api Service: api Service: proxy-metrics-service]")).
HasSyncErrors(map[string]string{"missing-member": "specific member configuration exists but no matching toolchaincluster was found"})
})
})
Expand Down
82 changes: 57 additions & 25 deletions deploy/registration-service/registration-service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,12 @@ objects:
- name: registration-service
image: ${IMAGE}
ports:
- containerPort: 8080
- containerPort: 8081
- containerPort: 8082
- containerPort: 8080 # registration service
- containerPort: 8081 # proxy
- containerPort: 8082 # proxy metrics
name: metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

optional - maybe we could rename this to proxy-metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, indeed, although I did not want to bring too many changes here, in case it's used by Konflux 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

- containerPort: 8083 # registration service metrics
name: registration-service-metrics
command:
- registration-service
imagePullPolicy: IfNotPresent
Expand Down Expand Up @@ -140,24 +142,8 @@ objects:
requests:
cpu: "50m"
memory: "100M"
- kind: Service
apiVersion: v1
metadata:
name: registration-service
namespace: ${NAMESPACE}
labels:
provider: codeready-toolchain
run: registration-service
spec:
ports:
- name: "8080"
protocol: TCP
port: 80
targetPort: 8080
selector:
run: registration-service
type: ClusterIP
sessionAffinity: null

# route for the registration service
- kind: Route
apiVersion: v1
metadata:
Expand All @@ -168,6 +154,7 @@ objects:
namespace: ${NAMESPACE}
spec:
host: ''
path: /api # we don't want to expose anything else (eg: Prometheus metrics)
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a route for the registration service.
but still, do we really need to limit it? isn't it already limited/isolated using services and ports?
apart from that - would it also allow requests on the /apis path? it's used in openshift as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhh, that's a mistake, I thought I reverted that change! 🤦‍♂️

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 in 7e58490

port:
targetPort: "8080"
to:
Expand All @@ -177,24 +164,48 @@ objects:
tls:
termination: edge
wildcardPolicy: None

# service associated with the registration service route
- kind: Service
apiVersion: v1
metadata:
name: api
name: registration-service
namespace: ${NAMESPACE}
labels:
provider: codeready-toolchain
run: registration-service
spec:
ports:
- name: "8081"
- name: "8080"
protocol: TCP
port: 80
targetPort: 8081
targetPort: 8080
selector:
run: registration-service
type: ClusterIP
sessionAffinity: null

# internal service for the registration service, used by Prometheus to scrape the metrics
- kind: Service
apiVersion: v1
metadata:
name: registration-service-metrics
namespace: ${NAMESPACE}
labels:
provider: codeready-toolchain
run: registration-service
spec:
ports:
- name: registration-service-metrics
protocol: TCP
port: 80
targetPort: registration-service-metrics
selector:
run: registration-service
type: ClusterIP
sessionAffinity: null

# route for the proxy
- kind: Route
apiVersion: v1
metadata:
Expand All @@ -206,7 +217,6 @@ objects:
name: api
namespace: ${NAMESPACE}
spec:
host: ''
port:
targetPort: "8081"
to:
Expand All @@ -216,6 +226,28 @@ objects:
tls:
termination: edge
wildcardPolicy: None

# service associated with the proxy route
- kind: Service
apiVersion: v1
metadata:
name: api
namespace: ${NAMESPACE}
labels:
provider: codeready-toolchain
run: registration-service
spec:
ports:
- name: "8081"
protocol: TCP
port: 80
targetPort: 8081
selector:
run: registration-service
type: ClusterIP
sessionAffinity: null

# internal service for the proxy, used by Prometheus to scrape the metrics
- kind: Service
apiVersion: v1
metadata:
Expand Down
Loading