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

Enable the admission webhook #460

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

alshabib
Copy link
Contributor

This enables the admission webhook on kne created namespace allowing the said webhook to manipulate pod specs.

@alexmasi
Copy link
Contributor

@alexmasi
Copy link
Contributor

/gcbrun

topo/topo.go Outdated
@@ -543,6 +543,9 @@ func (m *Manager) push(ctx context.Context) error {
ns := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: m.topo.Name,
Labels: map[string]string{
"admission-webhook": "enabled",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: i think something like "kne-topology": m.topo.Name. or "kne-topology": "true would be generally useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting an extra label here?

Copy link
Collaborator

@DanG100 DanG100 Nov 15, 2023

Choose a reason for hiding this comment

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

i was thinking instead of the "admission-webhook" label. You can operate on all the topologies namespaces by doing

namespaceSelector:
    matchExpressions:
      - key: kne-topology
        operator: Exists

to me its better to have kne say this namesapace is kne topo instead of allow any webhooks here

Copy link
Contributor

Choose a reason for hiding this comment

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

I like Daniel's idea here, I think there is value to attaching a label to the namespace to show it represents a topology. "kne-topology": true should suffice. Then you webhook can check for the "kne-topology" key to determine what to mutate. In the future we may add other tools that interact only with kne topo ns so I think its a better pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok - got it. Thanks makes sense, I'll change this to what you are suggesting.

@alshabib
Copy link
Contributor Author

Can you also change https://github.com/openconfig/kne/blob/main/kind/kind-no-cni.yaml?

What did you have in mind? We don't need any extra config here.

@alexmasi
Copy link
Contributor

Can you also change https://github.com/openconfig/kne/blob/main/kind/kind-no-cni.yaml?

What did you have in mind? We don't need any extra config here.

Oh nice, I thought there was a change needed to the kind settings.

@alexmasi
Copy link
Contributor

/gcbrun

@alexmasi alexmasi merged commit 44f0257 into openconfig:main Nov 15, 2023
5 checks passed
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.

3 participants