Skip to content

integrations/operator: propagate revision + fix tests #34265

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

Merged
hugoShaka merged 2 commits into from
Nov 7, 2023
Merged

integrations/operator: propagate revision + fix tests #34265

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
40929f5
operator: propagate revision info + fix tests
hugoShaka Nov 6, 2023
bb54a43
fixup! operator: propagate revision info + fix tests
hugoShaka Nov 6, 2023
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
8 changes: 8 additions & 0 deletions integrations/operator/apis/resources/v1/loginrule_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,11 @@ func (l *LoginRuleResource) SetOrigin(origin string) {
func (l *LoginRuleResource) GetMetadata() types.Metadata {
return *l.LoginRule.Metadata
}

func (l *LoginRuleResource) GetRevision() string {
return l.LoginRule.Metadata.GetRevision()
}

func (l *LoginRuleResource) SetRevision(rev string) {
l.LoginRule.Metadata.SetRevision(rev)
}
3 changes: 3 additions & 0 deletions integrations/operator/controllers/resources/role_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,9 @@ func (r *RoleReconciler) Upsert(ctx context.Context, obj kclient.Object) error {
}
}

if existingResource != nil {
teleportResource.SetRevision(existingResource.GetRevision())
}
r.AddTeleportResourceOrigin(teleportResource)

// If an error happens we want to put it in status.conditions before returning.
Expand Down
7 changes: 7 additions & 0 deletions integrations/operator/controllers/resources/teleport_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ type TeleportResource interface {
GetName() string
SetOrigin(string)
GetMetadata() types.Metadata
GetRevision() string
SetRevision(string)
}

// TeleportKubernetesResource is a Kubernetes resource representing a Teleport resource
Expand Down Expand Up @@ -133,6 +135,11 @@ func (r TeleportResourceReconciler[T, K]) Upsert(ctx context.Context, obj kclien

teleportResource.SetOrigin(types.OriginKubernetes)

// Propagate revision as required by opportunistic locking
if exists {
teleportResource.SetRevision(existingResource.GetRevision())
Copy link
Copy Markdown
Contributor

@tigrato tigrato Nov 7, 2023

Choose a reason for hiding this comment

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

Shouldn't we use Upsert instead of create/update?
Upsert doesn't care about the revision which is more or less what we need here

Copy link
Copy Markdown
Contributor Author

@hugoShaka hugoShaka Nov 7, 2023

Choose a reason for hiding this comment

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

I asked @rosstimothy, and we likely want to propagate revision even when Upserting. IIRC Update and Upsert have different behaviours for User, and both endpoints are not implemented for all resources. We might want to support both endpoints with the same generic controller.

I don't see an issue with propagating the revision. Worst case scenario, it is not used. In the best case scenario, this prevents a conflict. For some resources we are propagating information from the existing resource, the whole process is not 100% stateless. This happens in the optional Mutate() function. For example, the "CreatedBy" field of the user spec. In this case, opportunistic locking makes sense.

Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy Nov 7, 2023

Choose a reason for hiding this comment

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

Using Upsert directly could lead to the operator overwriting any manual changes made to a resource, I'm not sure how likely that scenario is. It is generally preferred to use Create/Update where possible.

Copy link
Copy Markdown
Contributor

@marcoandredinis marcoandredinis Nov 7, 2023

Choose a reason for hiding this comment

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

I think we must improve documentation.
When using IaC tools, it's not clear if users can change using other tools (web, tctl).

If they can't change, then the upsert is, imo, a better approach.
Even if they change the resource just to test something out, they can always go back to IaC flow.

If they can change the resource, the update will fail and they need to manually change the resource to merge any potential diff.

Have we decided on this?

Copy link
Copy Markdown
Contributor Author

@hugoShaka hugoShaka Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If they can change the resource, the update will fail and they need to manually change the resource to merge any potential diff.

With the current implementation, if a conflict happens, the operator update/upsert will fail and the resource reconciliation will be requeued. The operator will perform the update in the next reconciliation cycle, a few milliseconds later. The second reconciliation should succeed.

When using IaC tools, it's not clear if users can change using other tools (web, tctl).

We do have an origin label we are not using enough. The UI/tctl could display a warning if you are attempting to edit a resource whose origin is IaC. Things become even more confusing with resources like AccessLists where some of the information can and should be edited by users in the UI even if some parts of the resource are managed via IaC.

If they can't change, then the upsert is, imo, a better approach.
Even if they change the resource just to test something out, they can always go back to IaC flow.

AFAIK, if you do a change, TF/operator will erase it the next time, which is the intended behaviour.
My biggest issue today is not caused by users editing resources managed by IaC but caused by Teleport updating fields in the spec (which should not happen according to Kube's design, the status is here for computed values). Regardless if we use update or upsert, we need to propagate those dynamic fields that appear in the spec, or else we lose information.

Have we decided on this?

I don't think and can create an issue for this. AFAIK this PR doesn't change the current state of things, it only propagates the revision.

}

// We apply resource-specific mutations.
if mutator, ok := r.resourceClient.(TeleportResourceMutator[T]); ok {
mutator.Mutate(teleportResource, existingResource)
Expand Down
15 changes: 12 additions & 3 deletions integrations/operator/controllers/resources/user_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ import (
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/gravitational/trace"
"github.com/mitchellh/mapstructure"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
kerrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -347,7 +348,7 @@ func TestUserUpdate(t *testing.T) {
require.NoError(t, err)

// TeleportUser was updated with new roles
return assert.ElementsMatch(t, tUser.GetRoles(), []string{"x", "z"})
return compareRoles([]string{"x", "z"}, tUser.GetRoles())
})

// Updating the user in K8S
Expand All @@ -373,7 +374,7 @@ func TestUserUpdate(t *testing.T) {
require.NoError(t, err)

// TeleportUser updated with new roles
return assert.ElementsMatch(t, tUser.GetRoles(), []string{"x", "z", "y"})
return compareRoles([]string{"x", "y", "z"}, tUser.GetRoles())
})
require.Equal(t, setup.OperatorName, tUser.GetCreatedBy().User.Name, "createdBy has not been erased")
}
Expand Down Expand Up @@ -419,3 +420,11 @@ func getUserStatusConditionError(object map[string]interface{}) []metav1.Conditi
}
return conditionsWithError
}

func compareRoles(expected, actual []string) bool {
return cmp.Diff(
expected,
actual,
cmpopts.SortSlices(func(a, b string) bool { return a < b }),
) == ""
}