[fix] Replace docker-in-docker with notary client#22
Conversation
taejune
left a comment
taejune
left a comment
There was a problem hiding this comment.
전체적으로 handler에서 너무 많은 일을 해서 읽기가 쉽지 않아요.
pkg/server/handler.go
Outdated
| } | ||
|
|
||
| func (h *handler) findNotaryServer(registry string) string { | ||
| if registry == "docker.io" { |
There was a problem hiding this comment.
노터리 서버 주소를 구해오는 로직이 레지스트리 오퍼레이터 구현에 의존하고 있는데
나중에 오퍼레이터 구조가 변경되면 이 로직 자체가 수정되어야해서 힘들어질 것 같아요.
구해오는 로직을 구조체 초기화할 때 외부에서 함수로 넘기면 더 좋지 않을까 싶어요
There was a problem hiding this comment.
그렇게 하더라도 나중에 레지스트리 오퍼레이터 스펙이 변경되면 그 로직도 수정해야하는건 변하지 않지 않나요?
There was a problem hiding this comment.
모듈 외부에 맡기면 모듈 자체를 수정하지 않아도 되고 가져오는 방법에 대한 결정권이 모듈 사용자에게 있으니까 호출하는 맥락에 따라 유연하게 구현할 수 있으니까요
There was a problem hiding this comment.
현재 구조에서는 결국 handler 모듈의 유일한 사용자는 admission.go인데 admission에게 맡기자는 말씀이신가요?
그것도 이상하지 않나요?
There was a problem hiding this comment.
admission 입장에서 보면 이런 디펜던시가 모듈 안에 숨겨져 있는 것보다는 분명하게 드러나게하는게 좀 더 낫지 않을까 싶어요.
There was a problem hiding this comment.
우선 findNotaryServer 자체는 admission쪽으로 뺐습니다
나중에 리팩토링 하면서 추가적으로 정리하겠습니다
pkg/server/handler.go
Outdated
| func (h *handler) getSignature(img *image.Image) (*Signature, error) { | ||
| // Use notary client | ||
| tempDir := fmt.Sprintf("%s/notary/%s", os.TempDir(), randomString(10)) | ||
| not, err := trust.NewReadOnly(img, h.findNotaryServer(img.Host), tempDir) |
There was a problem hiding this comment.
메소드 이름만 봤을 때 외부통신을 할거라고 생각하지 않았어요.
외부에서 값을 가져와야한다면 메소드 밖에서 가져온 값 자체나 가져오는 방법에 대한 인터페이스를 인자로 넘겨서 사용해야할 것 같아요
There was a problem hiding this comment.
handler의 메소드로 넣는건 좀 이상한 것 같긴 하네요 차라리 외부 function으로 옮기겠습니다
There was a problem hiding this comment.
외부 fetchSignature 함수로 따로 뗐습니다
getter/fetcher를 interface로 받을까 생각도 해봤는데 어차피 Signature구조체도 그렇고 그 뒤의 프로세스도 notary에 강하게 의존적이라 굳이 여기서만 확장성을 부여하는건 의미 없다 판단해서 함수만 따로 뗐습니다
cqbqdd11519
force-pushed
the
fix/replace-dind
branch
from
f3957be to
1eb7103
Compare
- Removed docker daemon pod - Use notary client of registry-operator for getting image trust info
cqbqdd11519
force-pushed
the
fix/replace-dind
branch
from
1eb7103 to
7909428
Compare
2 participants
Changes
Submitter Checklist