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

fix(labels): propagates labels to Deployment's spec.template #236

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(labels): propagates labels to Deployment's spec.template #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe08b87
fix(labels): propagates labels to Deployment's spec.template
bartoszmajsak Jan 9, 2025
edae875
fix(test): asserts spect.template label propagation
bartoszmajsak Jan 9, 2025
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/authorino_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func (r *AuthorinoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
} else if existingDeployment == nil {
// Creates a new deployment resource to deploy the new authorino instance
newDeployment := r.buildAuthorinoDeployment(authorinoInstance)
if err := r.Client.Create(context.TODO(), newDeployment); err != nil {
if err := r.Client.Create(ctx, newDeployment); err != nil {
return ctrl.Result{}, r.wrapErrorWithStatusUpdate(
logger, authorinoInstance, r.setStatusFailed(statusUnableToCreateDeployment),
fmt.Errorf("failed to create %s Deployment resource, err: %v", newDeployment.Name, err),
Expand Down Expand Up @@ -259,7 +259,7 @@ func (r *AuthorinoReconciler) buildAuthorinoDeployment(authorino *api.Authorino)
volumes = append(volumes, authorinoResources.GetTlsVolume(authorinoTlsCertVolumeName, secretName))
}

// if an external OIDC server is enable mounts a volume to the container
// if an external OIDC server is enabled mounts a volume to the container
// by using the secret with the certs
if enabled := authorino.Spec.OIDCServer.Tls.Enabled; enabled == nil || *enabled {
secretName := authorino.Spec.OIDCServer.Tls.CertSecret.Name
Expand Down
5 changes: 3 additions & 2 deletions controllers/authorino_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,9 @@ var _ = Describe("Authorino controller", func() {
image := DefaultAuthorinoImage
existContainer := false

Expect(deployment.Spec.Replicas).Should(Equal(&replicas))
Expect(deployment.Labels).Should(Equal(map[string]string{"thisLabel": "willPropagate"}))
Expect(deployment.Spec.Replicas).To(Equal(&replicas))
Expect(deployment.Labels).To(Equal(map[string]string{"thisLabel": "willPropagate"}))
Expect(deployment.Spec.Template.Labels).Should(HaveKeyWithValue("thisLabel", "willPropagate"))
for _, container := range deployment.Spec.Template.Spec.Containers {
if container.Name == authorinoContainerName {
Expect(container.Image).Should(Equal(image))
Expand Down
3 changes: 3 additions & 0 deletions pkg/resources/k8s_deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import (
func GetDeployment(name, namespace, saName string, replicas *int32, containers []k8score.Container, vol []k8score.Volume, labels map[string]string) *k8sapps.Deployment {
objMeta := getObjectMeta(namespace, name, labels)
authorinoLabels := labelsForAuthorino(name)
for key, value := range labels {
authorinoLabels[key] = value
}
Comment on lines 10 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

This works well for the inital creation of an Authorino CR but there's a few things missing that require some further consideration for this to work for if an existing Authorino CR labels is updated also:

  1. The comparison logic for labels to determine if the Authorino CR has changed is currently missing and needs to be added. Currently, the labels is not compared so it will not be updated if the labels are changed.
  1. Once 1) is addressed and updating labels is allowed, the current logic will also update the deployment.spec.matchLabels which are immutable once set. Because these labels are immutable, the update will never succeed with an error from the API server

  • authorino-operator/pkg/resources/k8s_deployment.go

    Lines 17 to 19 in a92b462

    Selector: &v1.LabelSelector{
    MatchLabels: authorinoLabels,
    },
  • Not sure do we want a separate podLabels map of labels that contains the hardcoded authorinoLabels and the Authorino CR labels and use this in the deployment pod template. The matchLabel should always just use the hardcoded labels 🤔
  1. There is a possibilty that the labels defined in the Authorino CR can overwrite the hardcoded labels in labelsForAuthorino function, which then hit into 2) where the we attempt to update the deployment.spec.matchLabels. The hardcoded labels should take precedence in this case the prevent this 🤔

Other considerations around upgrade are:

  1. This currently do not respect any user added labels to deployment.spec.template.labels 🤔. In this case, if the deployment is upgraded, any user added labels will be lost since only the labels currently present on the Authorino CR and the hardcoded labels is kept. We should do a "smart" merge of labels where:
  • Known hardcoded labels take precedence
  • Merge Authorino CR labels
  • Keep any other user added labels already added to the deployment pod template.

This will however mean there's a wider scope of change as the desired deployment will need awareness of the existing deployment pod template labels

@eguzki Let me know if I've missed anything or if you have any thoughts on these suggestions.

@bartoszmajsak What are your thought on the above?

Copy link
Contributor Author

@bartoszmajsak bartoszmajsak Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

These are all valid points. Thanks for your thorough analysis.

Do you want me to expand this PR based on this discussion or is something to be done as a follow-up? Happy to work on it either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I think it would be nicer to exapnd this PR based on this discussion, especially for points 1 - 3 for accounting for updates.

However, If you feel the PR change would get too big though, I think the smart merge bit for upgrades can be done as follow-up/seperate PR. Let me know if you prefer this. Thanks for taking on this work 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KevFan some of it definitely belongs to this PR, so I will dig into it in the 2nd half of the week. I came purely from "create CR" angle and didn't consider all the other "what-ifs" :)


return &k8sapps.Deployment{
ObjectMeta: objMeta,
Expand Down
Loading