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: impresonation regression for service accounts #405

Conversation

rgruchalski-klarrio
Copy link
Contributor

Fixes service account impresonation regression discussed in #388 (comment).

@rgruchalski-klarrio rgruchalski-klarrio force-pushed the fix-impresonation-system-authenticated branch from c070bb5 to 9e32ee2 Compare March 2, 2024 20:00
@rgruchalski-klarrio rgruchalski-klarrio changed the title Fix impresonation regression for service accounts fix: impresonation regression for service accounts Mar 2, 2024
@rgruchalski-klarrio rgruchalski-klarrio force-pushed the fix-impresonation-system-authenticated branch 3 times, most recently from 8ad49e4 to 0f1187d Compare March 2, 2024 20:08
@rgruchalski-klarrio
Copy link
Contributor Author

Yay! The checks are passing! :)

@maxgio92
Copy link
Collaborator

maxgio92 commented Mar 3, 2024

Welcome to Capsule and thank you a lot @rgruchalski-klarrio!

Copy link
Collaborator

@maxgio92 maxgio92 left a comment

Choose a reason for hiding this comment

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

Looks good to me @rgruchalski-klarrio!
Just left a couple of minor points.

cc @prometherion PTAL

internal/request/http_test.go Show resolved Hide resolved
internal/request/http.go Outdated Show resolved Hide resolved
internal/request/http.go Outdated Show resolved Hide resolved
internal/request/http_test.go Outdated Show resolved Hide resolved
@rgruchalski-klarrio rgruchalski-klarrio force-pushed the fix-impresonation-system-authenticated branch from 079f33d to d79e4be Compare March 4, 2024 08:04
@maxgio92
Copy link
Collaborator

maxgio92 commented Mar 4, 2024

Thank you @rgruchalski-klarrio! LGTM

@oliverbaehler
Copy link
Collaborator

@rgruchalski-klarrio thanks for your contribution. Please fix the commit syntax. We have good documentation here: https://github.com/projectcapsule/capsule/blob/main/CONTRIBUTING.md#semantics

@oliverbaehler
Copy link
Collaborator

Also run make golint to get semantic passing for your code.

@rgruchalski-klarrio
Copy link
Contributor Author

rgruchalski-klarrio commented Mar 5, 2024

@maxgio92 what does this even mean?

/Users/radek/dev/golang/src/github.com/projectcapsule/capsule-proxy/bin/golangci-lint run -c .golangci.yml
internal/request/http_test.go:17: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/projectcapsule/capsule-proxy) (gci)
	"sigs.k8s.io/controller-runtime/pkg/client"

internal/request/http_test.go:20: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/projectcapsule/capsule-proxy) (gci)
	"k8s.io/apiserver/pkg/authentication/user"

@rgruchalski-klarrio rgruchalski-klarrio force-pushed the fix-impresonation-system-authenticated branch from d79e4be to 0058038 Compare March 5, 2024 14:26
@oliverbaehler
Copy link
Collaborator

@rgruchalski-klarrio That means your imports are not correctly ordered

  1. Core imports
  2. 3-P imports
  3. Project local Imports

@rgruchalski-klarrio
Copy link
Contributor Author

rgruchalski-klarrio commented Mar 5, 2024

Okay, thank you @oliverbaehler, this seems to be a puzzle I really don't want to think about, what's the correct order?

http.go

import (
	"fmt"
	h "net/http"
	"regexp"
	"strings"

	authenticationv1 "k8s.io/api/authentication/v1"
	authorizationv1 "k8s.io/api/authorization/v1"
	"k8s.io/apiserver/pkg/authentication/serviceaccount"
	"k8s.io/apiserver/pkg/authentication/user"
	"sigs.k8s.io/controller-runtime/pkg/client"
)

http_tets.go:

import (
	"context"
	"crypto/tls"
	"crypto/x509"
	"crypto/x509/pkix"
	"fmt"
	"net/http"
	"reflect"
	"regexp"
	"sort"
	"testing"

	authenticationv1 "k8s.io/api/authentication/v1"
	authorizationv1 "k8s.io/api/authorization/v1"
	"sigs.k8s.io/controller-runtime/pkg/client"

	"k8s.io/apiserver/pkg/authentication/serviceaccount"
	"k8s.io/apiserver/pkg/authentication/user"

	"github.com/projectcapsule/capsule-proxy/internal/request"
)

@oliverbaehler
Copy link
Collaborator

They should look like this, not really a puzzle:

import (
	"context"
	"crypto/tls"
	"crypto/x509"
	"crypto/x509/pkix"
	"fmt"
	"net/http"
	"reflect"
	"regexp"
	"sort"
	"testing"

	authenticationv1 "k8s.io/api/authentication/v1"
	authorizationv1 "k8s.io/api/authorization/v1"
	"k8s.io/apiserver/pkg/authentication/serviceaccount"
	"k8s.io/apiserver/pkg/authentication/user"
	"sigs.k8s.io/controller-runtime/pkg/client"

	"github.com/projectcapsule/capsule-proxy/internal/request"
)

@rgruchalski-klarrio rgruchalski-klarrio force-pushed the fix-impresonation-system-authenticated branch from 0058038 to e8fba6d Compare March 5, 2024 16:49
@rgruchalski-klarrio
Copy link
Contributor Author

Ah, thank you, alphabetical order within a group. All resolved and the CI is passing.

@oliverbaehler
Copy link
Collaborator

@rgruchalski-klarrio We only accept GPG signed commits, please sign your commit

@rgruchalski-klarrio
Copy link
Contributor Author

rgruchalski-klarrio commented Mar 5, 2024

omg... yes, let me configure gpg and have a look at this once again for you.

@rgruchalski-klarrio rgruchalski-klarrio force-pushed the fix-impresonation-system-authenticated branch 2 times, most recently from 7104e38 to 55b6bac Compare March 5, 2024 17:20
Signed-off-by: Radek Gruchalski <radek.gruchalski@klarrio.com>
@rgruchalski-klarrio rgruchalski-klarrio force-pushed the fix-impresonation-system-authenticated branch from 55b6bac to ccdbcb3 Compare March 5, 2024 17:22
@rgruchalski-klarrio
Copy link
Contributor Author

There we go. Should be much better now.

@oliverbaehler
Copy link
Collaborator

It's not for me, no need to get upset ☺️

@rgruchalski-klarrio
Copy link
Contributor Author

rgruchalski-klarrio commented Mar 5, 2024

Nobody got upset, just extremely busy and the ping ping kinda annoyed me. Glad we're done with it!

@oliverbaehler oliverbaehler merged commit 16deb06 into projectcapsule:main Mar 5, 2024
8 checks passed
@rgruchalski-klarrio rgruchalski-klarrio deleted the fix-impresonation-system-authenticated branch March 5, 2024 20:01
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.

4 participants